Closed Bug 1356693 Opened 7 years ago Closed 5 years ago

[infer] Fix all RESOURCE_LEAK's

Categories

(Firefox for Android Graveyard :: General, defect, P3)

Product:
Firefox for Android Graveyard
Component:
General
Type:
defect

Tracking

(firefox55 affected)

Status:
RESOLVED INACTIVE
Tracking Flags:
Tracking Status
firefox55 --- affected

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

Attachments

(6 files, 1 obsolete file)

Bug 1356693 - Pre: use IOUtils.safeStreamClose() in IOUtils
59 bytes, text/x-review-board-request
walkingice
: review+
Details
Bug 1356693 - Pre: add comment explaining why MergeCursor can't handle null cursors
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1356693 - infer: fix RESOURCE_LEAK's in base
59 bytes, text/x-review-board-request
walkingice
: review+
Details
Bug 1356693 - infer: fix RESOURCE_LEAK's in services
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 1356693 - infer: fix RESOURCE_LEAK's in geckoview
59 bytes, text/x-review-board-request
walkingice
: review+
Details
Bug 1356693 - infer: fix RESOURCE_LEAK's in stumbler
59 bytes, text/x-review-board-request
walkingice
: review+
Details 
This is a catch-all bug for all RESOURCE_LEAK's found by infer. Please leave-open until all issues are addressed.

I'm getting 35 reported instances (plus a few more I've already fixed locally) using:
grep -c "RESOURCE_LEAK" infer-out/bugs.txt
I've got patches tackling most of the app resource leaks (I'm ignoring tests for now).

Other than tests, we still have:
- A few MergeCursor reports, which I think are false positives (infer doesn't seem to recognise that the MergeCursor now owns the Cursors going into it, I should investigate that more)
- Stream being closed on a background thread, which is probably valid given the background thread could end up never being run. That might need a slight architecture change?
- Stream being returned from a method in Logger, I still need to dig into that more - it might be a false positive since Logger does close its Streams.
Assignee: nobody → ahunt
Comment on attachment 8858520 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in base

https://reviewboard.mozilla.org/r/130488/#review133902

::: mobile/android/base/java/org/mozilla/gecko/ANRReporter.java:183
(Diff revision 2)
>                      Log.d(LOGTAG, "empty getprop result");
>                  }
>              } finally {
>                  propProc.destroy();
> +
> +                if (buf != null) {

I should use IOUtils.safeStreamClose() here
I've discovered one resource leak (using infer) in our webrtc code, in:
media/webrtc/trunk/webrtc/modules/video_render/android/java/src/org/webrtc/videoengine/ViESurfaceRenderer.java

This looks like upstream code that we've imported, so I'm wondering if I should try to upstream this patch. However the current upstream doesn't contain anything under webrtc/modules/video_render anymore, so this fix probably isn't even relevant there. Do you have any opinions on this?

This leak looks like its in debug only code, so I'm not too concerned about actually fixing it. Our options seem to be:
- Land this fix in mozilla-central?
- Try to upstream (but upstream no longer has this class, I've got no idea if the code in question is gone or just moved)?
- Ignore all errors in media/rtc for the purposes of infer (this is the only issue it found)?

(My current draft patch which fixes the leak is at: https://reviewboard.mozilla.org/r/130492/diff/3#index_header )
Flags: needinfo?(rjesup)
Attachment #8858518 - Flags: review?(walkingice0204)
Attachment #8858519 - Flags: review?(gkruglov)
Attachment #8858520 - Flags: review?(walkingice0204)
Attachment #8858521 - Flags: review?(gkruglov)
Attachment #8858523 - Flags: review?(walkingice0204)
Attachment #8858524 - Flags: review?(walkingice0204)
(In reply to Andrzej Hunt :ahunt from comment #23)
> This looks like upstream code that we've imported, so I'm wondering if I
> should try to upstream this patch. However the current upstream doesn't
> contain anything under webrtc/modules/video_render anymore, so this fix
> probably isn't even relevant there. Do you have any opinions on this?

It's likely not relevant anymore.  I'm about to land webrtc.org version 57, which doesn't have that file - what exact code has the leak?

> This leak looks like its in debug only code, so I'm not too concerned about
> actually fixing it. Our options seem to be:
> - Land this fix in mozilla-central?
> - Try to upstream (but upstream no longer has this class, I've got no idea
> if the code in question is gone or just moved)?
> - Ignore all errors in media/rtc for the purposes of infer (this is the only
> issue it found)?

Well, on my update to 57 the code will be very different, so checking there is useful at that point.  And we have no code that can ever call savebitmaptojpeg that I know of, even in debug.
Flags: needinfo?(rjesup)
Comment on attachment 8858519 [details]
Bug 1356693 - Pre: add comment explaining why MergeCursor can't handle null cursors

https://reviewboard.mozilla.org/r/130486/#review135924

OK.
Attachment #8858519 - Flags: review?(gkruglov) → review+
Comment on attachment 8858521 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in services

https://reviewboard.mozilla.org/r/130490/#review135928

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/Utils.java:505
(Diff revision 3)
>        }
>        return sb.toString();
>      } catch (Exception e) {
>        return null;
>      } finally {
> -      if (isr != null) {
> +      IOUtils.safeStreamClose(br);

Does .close on `BufferedReader` cacsade to nested streams?

I guess from the docs it does: "Closes the stream and releases any system resources associated with it"
Attachment #8858521 - Flags: review?(gkruglov) → review+
Comment on attachment 8858518 [details]
Bug 1356693 - Pre: use IOUtils.safeStreamClose() in IOUtils

https://reviewboard.mozilla.org/r/130484/#review136588

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/IOUtils.java:93
(Diff revision 2)
>  
>              return new ConsumedInputStream(bPointer + 1, buffer);
>          } catch (IOException e) {
>              Log.e(LOGTAG, "Error consuming input stream.", e);
>          } finally {
> -            try {
> +            IOUtils.safeStreamClose(iStream);

should we print log in `safeStreamClose` as well?
Attachment #8858518 - Flags: review?(walkingice0204) → review+
Comment on attachment 8858520 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in base

https://reviewboard.mozilla.org/r/130488/#review136596

::: mobile/android/base/java/org/mozilla/gecko/updater/UpdateService.java:630
(Diff revision 3)
>              mDownloading = false;
>  
>              if (mWifiLock.isHeld()) {
>                  mWifiLock.release();
>              }
> +

should we also append same comments here? ("conn isn't guaranteed to be an HttpURLConnection.....")
Attachment #8858520 - Flags: review?(walkingice0204) → review+
Comment on attachment 8858523 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in geckoview

https://reviewboard.mozilla.org/r/130494/#review136598

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1195
(Diff revision 3)
>  
>      private static void EnumerateGeckoProcesses(GeckoProcessesVisitor visiter) {
>          int pidColumn = -1;
>          int userColumn = -1;
>  
> +        java.lang.Process ps = null;

is "java.lang." Necessary?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1265
(Diff revision 3)
>      public static void listOfOpenFiles() {
>          int pidColumn = -1;
>          int nameColumn = -1;
>  
> -        try {
> -            String filter = GeckoProfile.get(getApplicationContext()).getDir().toString();
> +        String filter = GeckoProfile.get(getApplicationContext()).getDir().toString();

I am not quite sure, will it be safe if move this line out of try-catch?
Comment on attachment 8858524 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in stumbler

https://reviewboard.mozilla.org/r/130496/#review136606
Attachment #8858524 - Flags: review?(walkingice0204) → review+
(In reply to Randell Jesup [:jesup] from comment #24)
> (In reply to Andrzej Hunt :ahunt from comment #23)
> > This looks like upstream code that we've imported, so I'm wondering if I
> > should try to upstream this patch. However the current upstream doesn't
> > contain anything under webrtc/modules/video_render anymore, so this fix
> > probably isn't even relevant there. Do you have any opinions on this?
> 
> It's likely not relevant anymore.  I'm about to land webrtc.org version 57,
> which doesn't have that file - what exact code has the leak?

The leak happens in saveBitmapToJPEG(): the "FileOutputStream output" is leaked if output.write() throws an Exception. This isn't tragic since it's debug code, even more so since its unused.
> 
> 
> Well, on my update to 57 the code will be very different, so checking there
> is useful at that point.  And we have no code that can ever call
> savebitmaptojpeg that I know of, even in debug.

Good to know! I'll just scrap my patch - since this is an external dependency I might just completely disable static analysis for now (we have plenty of pre-existing static analysis issues in the main Firefox for Android codebase, so there's not much value in looking at external issues for now - especially since the only webrtc issue seems to be a non-issue).
Comment on attachment 8858523 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in geckoview

https://reviewboard.mozilla.org/r/130494/#review136724

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java:1195
(Diff revision 3)
>  
>      private static void EnumerateGeckoProcesses(GeckoProcessesVisitor visiter) {
>          int pidColumn = -1;
>          int userColumn = -1;
>  
> +        java.lang.Process ps = null;

Nope - it looks like this class uses a lot of java.lang.Foo for some reason and I never noticed...
Attachment #8858522 - Attachment is obsolete: true
Comment on attachment 8858523 [details]
Bug 1356693 - infer: fix RESOURCE_LEAK's in geckoview

https://reviewboard.mozilla.org/r/130494/#review136794
Attachment #8858523 - Flags: review?(walkingice0204) → review+
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ca9044bb930
Pre: use IOUtils.safeStreamClose() in IOUtils r=walkingice
https://hg.mozilla.org/integration/autoland/rev/76c560e8d908
Pre: MergeCursor can handle null cursors r=Grisha
https://hg.mozilla.org/integration/autoland/rev/8b672583d57c
infer: fix RESOURCE_LEAK's in base r=walkingice
https://hg.mozilla.org/integration/autoland/rev/6d26ad68f31f
infer: fix RESOURCE_LEAK's in services r=Grisha
https://hg.mozilla.org/integration/autoland/rev/731479637eda
infer: fix RESOURCE_LEAK's in stumbler r=walkingice
https://hg.mozilla.org/integration/autoland/rev/1fd99a71f607
infer: fix RESOURCE_LEAK's in geckoview r=walkingice
backed out for test failures/crashes like https://treeherder.mozilla.org/logviewer.html#?job_id=94810397&repo=autoland
Flags: needinfo?(ahunt)
Sorry about landing that. I forgot to add robocop tests on my final try run (I'd previously tried to address an issue that robocop tests had found, which didn't succeed in my last draft set of patches).

The cause is that MergeCursor can't actually handle null Cursors well: if the first Cursor in the list is null, and you try to read columns (Cursor.getColumnIndexOrThrow()), then the Cursor will throw. CursorAdapter always tries to read the "_id" column when swapping a Cursor, which guarantees a crash if the first Cursor is null. I've updated that patch to instead add a comment explaining why we need to make sure to put no nulls in a MergeCursor (other than that, MergeCursor does have logic to handle null Cursors).
Flags: needinfo?(ahunt)
All green on try including robocop tests, except for an x86 build failure in unrelated c++ code (Unified_cpp_js_src18.cpp ?), appears to be safe to push again.
Pushed by ahunt@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/90b14b6c3e22
Pre: use IOUtils.safeStreamClose() in IOUtils r=walkingice
https://hg.mozilla.org/integration/autoland/rev/f7c98bd5fa0b
Pre: add comment explaining why MergeCursor can't handle null cursors r=Grisha
https://hg.mozilla.org/integration/autoland/rev/b61dedcb6e38
infer: fix RESOURCE_LEAK's in base r=walkingice
https://hg.mozilla.org/integration/autoland/rev/e21c60bf9e3a
infer: fix RESOURCE_LEAK's in services r=Grisha
https://hg.mozilla.org/integration/autoland/rev/57f27e3e8a4d
infer: fix RESOURCE_LEAK's in stumbler r=walkingice
https://hg.mozilla.org/integration/autoland/rev/9ca2a0410bf6
infer: fix RESOURCE_LEAK's in geckoview r=walkingice
Julian, it looks like this one can be closed now? It wasn't closed automatically because of the "leave-open" keyword.
Flags: needinfo?(walkingice0204)
I ran `infer run -- ./gradlew assembleLocalPhotonDebug` and got 12 RESOURCE_LEAK. It is likely we need to keep this bug open.
Flags: needinfo?(walkingice0204)
[triage] Bulk edit from title: this is a non-critical issue or [meta] bug.
Priority: -- → P3

No action for a while, closing as we should probably open a new bug now anyway for this.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → INCOMPLETE
Resolution: INCOMPLETE → INACTIVE
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: