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)
Firefox for Android Graveyard
Data Providers
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 | ||
Updated•8 years ago
|
Assignee: nobody → ahunt
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40913/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40913/
Attachment #8731902 -
Flags: review?(s.kaspari)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40915/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40915/
Attachment #8731903 -
Flags: review?(s.kaspari)
Comment 3•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8731903 -
Flags: review?(s.kaspari) → review+
Comment 4•8 years ago
|
||
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.
https://hg.mozilla.org/integration/fx-team/rev/95c8cb6f1515 https://hg.mozilla.org/integration/fx-team/rev/ca8d1e1caeef
Assignee | ||
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Comment 7•8 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/787e4d0b72b84a59086bf3ebe7cbedbd5d6773db Bug 1257667 - Try to catch stale cursor usage using LeakCanary r=sebastian
Assignee | ||
Comment 8•8 years ago
|
||
(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...)
Comment 9•8 years ago
|
||
bugherder |
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•