Closed Bug 1257667 Opened 8 years ago Closed 8 years ago

Ensure we've swapped cursors in CursorLoaderCallbacks.onLoadFinished using LeakCanary

Categories

(Firefox for Android Graveyard :: Data Providers, defect)

defect
Not set
normal

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: ahunt, Assigned: ahunt)

References

Details

Attachments

(2 files)

CursorLoader (or our own SimpleCursorLoader implementation) will close the old cursor during deliverResult (if it exists) once CursorLoaderCallbacks.onLoadFinished has completed. We can use LeakCanary to make sure that we're not holding on to this cursor anywhere.

This wouldn't catch our topsites crash (see Bug 1254468), since that only "leaks" the cursor for the duration of the awesomescreen transition (tenths of a second), whereas LeakCanary has a default leak-checking delay of 5s, however it might help catch other cursor leaks.

It might be worth looking into checking whether we can force immediate leak checking with LeakCanary?
Assignee: nobody → ahunt
Comment on attachment 8731902 [details]
MozReview Request: Bug 1257667 - Pre: Allow retrieving GeckoApplication's RefWatcher for leak-spotting r?sebastian

https://reviewboard.mozilla.org/r/40913/#review37591

Nice!
Attachment #8731902 - Flags: review?(s.kaspari) → review+
Attachment #8731903 - Flags: review?(s.kaspari) → review+
Comment on attachment 8731903 [details]
MozReview Request: Bug 1257667 - Try to catch stale cursor usage using LeakCanary r?sebastian

https://reviewboard.mozilla.org/r/40915/#review37593

Cool! Let's see if we can catch something.

::: mobile/android/base/java/org/mozilla/gecko/home/SimpleCursorLoader.java:77
(Diff revision 1)
> +            // Trying to read from the closed cursor will cause crashes, hence we should make
> +            // sure that no adapters/LoaderCallbacks are holding onto this cursor.
> +            GeckoApplication.getRefWatcher(getContext()).watch(oldCursor);
> +
>              oldCursor.close();

NIT: I would move this after closing the cursor. Just because LeakCanary uses timers afaik and we shouldn't continue interacting with the object: In the worst case close() could take very long and LeakCanary would detect a leak because the reference is still not cleared.
(In reply to Pulsebot from comment #5)
> https://hg.mozilla.org/integration/fx-team/rev/95c8cb6f1515
> https://hg.mozilla.org/integration/fx-team/rev/ca8d1e1caeef

Oh no, I managed to land the unfixed version, I'll try to backout and reland.
https://hg.mozilla.org/integration/fx-team/rev/787e4d0b72b84a59086bf3ebe7cbedbd5d6773db
Bug 1257667 - Try to catch stale cursor usage using LeakCanary r=sebastian
(In reply to Andrzej Hunt :ahunt from comment #6)
> (In reply to Pulsebot from comment #5)
> > https://hg.mozilla.org/integration/fx-team/rev/95c8cb6f1515
> > https://hg.mozilla.org/integration/fx-team/rev/ca8d1e1caeef
> 
> Oh no, I managed to land the unfixed version, I'll try to backout and reland.

Fixed now (probably got too distracted the first time round since it was my first push using git...)
https://hg.mozilla.org/mozilla-central/rev/95c8cb6f1515
https://hg.mozilla.org/mozilla-central/rev/787e4d0b72b8
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.