LogFileManager.getLogString leaking Closeable during SessionReportingCoordinator.addLogsCustomKeysAndEventKeysToEvent #7403

OOS93 · 2025-09-22T21:57:07Z

Refactoring QueueFile.java to avoid holding a leakable reference for a RandomAccessFile instance, instead a reference for the underlying file is kept. See #7341 for further context.

…a RandomAccessFile instance, instead a reference for the underlying file is kept. See firebase#7341 for further context.

gemini-code-assist · 2025-09-22T21:57:11Z

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

OOS93 · 2025-09-24T21:56:00Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors QueueFile.java to address a potential resource leak by no longer holding a persistent RandomAccessFile instance. While the overall approach is sound, the implementation introduces several critical bugs that could lead to NullPointerException and ClosedChannelException at runtime. I've identified issues in the QueueFile constructor, the initialize method, and the expandIfNecessary method, and have provided code suggestions to resolve them. The related changes in QueueFileLogStore.java are correct and align with the intended refactoring.

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java

…ion).

…sable' into crashlytics_strictmode_leakedclosable

OOS93 · 2025-09-24T22:28:36Z

/gemini review

gemini-code-assist

Code Review

This pull request successfully refactors QueueFile to avoid holding a long-lived RandomAccessFile instance, which resolves a potential resource leak. The new approach involves opening and closing the file on-demand for each operation. While this fixes the leak, it introduces a critical bug and several significant performance regressions where file operations are performed repeatedly, such as in loops or for stream-based reading. I have provided detailed comments on these issues with suggestions for how to resolve them while maintaining the leak-safe design.

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java

OOS93 · 2025-09-25T01:37:05Z

/gemini review

gemini-code-assist

Code Review

This pull request effectively addresses a potential resource leak in QueueFile by refactoring it to not hold a persistent RandomAccessFile instance. Instead, file operations are wrapped in a new openAndExecute helper that manages the RandomAccessFile lifecycle using try-with-resources. This is a solid improvement.

My review includes a high-severity point about a potential issue with nested file access that could arise from the new implementation of setLength, and a suggestion to improve encapsulation for the clear method. Overall, the changes are in the right direction to make the class more robust.

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java

OOS93 · 2025-09-25T01:44:07Z

/gemini review

gemini-code-assist

Code Review

This pull request effectively addresses a potential resource leak in QueueFile by changing the strategy for handling RandomAccessFile. Instead of holding a reference to an open file, the implementation now opens and closes the file for each operation, ensuring resources are properly managed using try-with-resources. This is a solid improvement for robustness. While this approach prioritizes correctness and leak prevention, it introduces a performance trade-off due to the overhead of repeated file open/close operations, which I've noted in a specific comment. The changes in QueueFileLogStore are consistent with the QueueFile refactoring. Overall, this is a good change that improves the reliability of the logging mechanism.

gemini-code-assist · 2025-09-25T01:45:19Z

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java

  private final byte[] buffer = new byte[16];

  /**
   * Constructs a new queue backed by the given file. Only one {@code QueueFile} instance should
   * access a given file at a time.
   */
  public QueueFile(File file) throws IOException {
+    this.rafFile = file;
    if (!file.exists()) {
      initialize(file);
    }
-    raf = open(file);
-    readHeader();
-  }
-
-  /** For testing. */
-  QueueFile(RandomAccessFile raf) throws IOException {
-    this.raf = raf;
    readHeader();
  }


This constructor, which was marked for testing, was very useful for injecting a mock RandomAccessFile to unit test the class's logic without touching the file system. Its removal makes unit testing more difficult, as tests will now likely need to interact with real files. While the new design with openAndExecute makes this kind of injection harder, it might be worth considering if there's an alternative way to maintain the same level of testability, perhaps by allowing injection of a RandomAccessFile factory.

gemini-code-assist · 2025-09-25T01:45:20Z

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java

+  private void openAndExecute(RandomAccessFileCallable rafCallable) throws IOException {
+    try (RandomAccessFile raf = open(rafFile)) {
+      rafCallable.run(raf);
+    }
+  }


This new openAndExecute pattern successfully prevents resource leaks by ensuring the RandomAccessFile is always closed. However, it's worth noting the performance trade-off. Opening and closing a file for every operation (like add, peek, remove) can introduce significant overhead compared to keeping the file handle open. For the use case of crash logging, this is likely an acceptable trade-off for improved robustness, but it's an important design consideration to be aware of if this class were to be used in a high-throughput scenario.

gemini-code-assist · 2025-09-25T01:45:20Z

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java

+      int readByte;
      raf.seek(position);
-      int b = raf.read();
+      readByte = raf.read();


The declaration of readByte can be combined with its assignment for better conciseness.

Suggested change

int readByte;

raf.seek(position);

int b = raf.read();

readByte = raf.read();

raf.seek(position);

int readByte = raf.read();

Refactoring QueueFile.java to avoid holding a leakable reference for …

9471eed

…a RandomAccessFile instance, instead a reference for the underlying file is kept. See firebase#7341 for further context.

Merge branch 'main' into crashlytics_strictmode_leakedclosable

9fa7441

gemini-code-assist bot reviewed Sep 24, 2025

View reviewed changes

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java Show resolved Hide resolved

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java Outdated Show resolved Hide resolved

OOS93 added 2 commits September 24, 2025 22:15

Addressing Code Review (possible NPE and mistaken file at initializat…

3a091bd

…ion).

Merge remote-tracking branch 'origin/crashlytics_strictmode_leakedclo…

74e5b92

…sable' into crashlytics_strictmode_leakedclosable

gemini-code-assist bot reviewed Sep 24, 2025

View reviewed changes

OOS93 added 3 commits September 24, 2025 22:32

NIT on Method Signature.

5056a8f

Aim for the right RAF method in order to handle FileChannel instance.

a820d31

Optimizing the use of RandomAccessFile with the help of local variables.

9801199

gemini-code-assist bot reviewed Sep 25, 2025

View reviewed changes

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java Outdated Show resolved Hide resolved

...e-crashlytics/src/main/java/com/google/firebase/crashlytics/internal/metadata/QueueFile.java Outdated Show resolved Hide resolved

Refining the use of RandomAccessFile with the help of local variables.

1969758

gemini-code-assist bot reviewed Sep 25, 2025

View reviewed changes

LogFileManager.getLogString leaking Closeable during SessionReportingCoordinator.addLogsCustomKeysAndEventKeysToEvent #7403

Are you sure you want to change the base?

LogFileManager.getLogString leaking Closeable during SessionReportingCoordinator.addLogsCustomKeysAndEventKeysToEvent #7403

Uh oh!

Conversation

OOS93 commented Sep 22, 2025

Uh oh!

gemini-code-assist bot commented Sep 22, 2025

Uh oh!

OOS93 commented Sep 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

OOS93 commented Sep 24, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OOS93 commented Sep 25, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

OOS93 commented Sep 25, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!