Closed Bug 1210243 Opened 4 years ago Closed 4 years ago

ReadingListPanel.markAsRead can crash on a null return from getActivity()

Categories

(Firefox for Android Graveyard :: Reading List, defect)

defect
Not set

Tracking

(firefox42 unaffected, firefox43 verified, firefox44 verified)

VERIFIED FIXED
Firefox 44
Tracking Status
firefox42 --- unaffected
firefox43 --- verified
firefox44 --- verified

People

(Reporter: mfinkle, Assigned: mfinkle)

References

Details

Crash Data

Attachments

(2 files)

While running some monkey tests (bug 1206071) I found a crash that happened because the Context returned from getActivity() was null. That can happen if using Fragments and the fragment gets detached. That could be happening here.

Crash:
https://crash-stats.mozilla.com/report/index/dc74c18d-a92e-41d7-aa8f-f888c2150930

Line:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/ReadingListPanel.java#116

via:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/ReadingListPanel.java#94
Looks like we saw this in Top Sites too for the List (bug 930160) and the grid (bug 1096958) but never moved the pattern to other panels.
Except the fixes described in comment 1 don't work in this case. If I load about:home, go to Reading List and tap an article, everything works. If I tap on the URL bar to cause the home panels to open, go to Reading List and tap an article, it crashes.

I'll need a specific fix here.
This patch just adds the same listener cleanup added to Top Sites. It's just some defensive code.
Assignee: nobody → mark.finkle
Attachment #8668243 - Flags: review?(margaret.leibovic)
This patch fixes the actual crash. If we don't have a Context, we just bail. Curiously, by moving the Context accessor to the beginning of the method, we don't get a null Context and we can markAsRead without crashing anymore.

I suspect the race had to do with the mUrlOpenListener.onUrlOpen(...) call, which probably takes some non-trivial time.

In any case, we check for a null context, but don't seem to fail anymore: win-win.
Attachment #8668244 - Flags: review?(margaret.leibovic)
Comment on attachment 8668244 [details] [diff] [review]
readingpanel-context-check v0.1

Review of attachment 8668244 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by-review (I wrote markAsRead): r+

::: mobile/android/base/home/ReadingListPanel.java
@@ +114,5 @@
>          });
>          registerForContextMenu(mList);
>      }
>  
> +    private void markAsRead(final long id, final Context context) {

Super NIT: I think Context should always go first. But I'm having a hard time rationalizing this (Order of importance? Being aligned with other methods taking Context arguments?). So just ignore if you don't agree. :)
Attachment #8668244 - Flags: review+
Attachment #8668244 - Flags: review?(margaret.leibovic)
(In reply to Sebastian Kaspari (:sebastian) from comment #5)
> Comment on attachment 8668244 [details] [diff] [review]
> readingpanel-context-check v0.1
> 
> Review of attachment 8668244 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Drive-by-review (I wrote markAsRead): r+
> 
> ::: mobile/android/base/home/ReadingListPanel.java
> @@ +114,5 @@
> >          });
> >          registerForContextMenu(mList);
> >      }
> >  
> > +    private void markAsRead(final long id, final Context context) {
> 
> Super NIT: I think Context should always go first. But I'm having a hard
> time rationalizing this (Order of importance? Being aligned with other
> methods taking Context arguments?). So just ignore if you don't agree. :)

Done. I took your review for the crasher. Margaret can review the sanity check later.
Comment on attachment 8668243 [details] [diff] [review]
fragment-sanity-check v0.1

Review of attachment 8668243 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for being slow to review. It could be worth making a mailing list post to alert people to this potential pitfall.
Attachment #8668243 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/ada7566c1f9d
https://hg.mozilla.org/mozilla-central/rev/be9f3f152aae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Reproduced the issue on Firefox for Android 44.0a1 (2015-09-28) after adding a page to reading list and went to private browsing to the reading list panel and tap the page added to reading list.
Verified as fixed on Firefox for Android 44.0a1 (2015-10-04) using Samsung Galaxy Note 5 (Android 5.1.1)

Still reproducible the crash on latest Aurora.
Comment on attachment 8668244 [details] [diff] [review]
readingpanel-context-check v0.1

Approval Request Comment
[Feature/regressing bug #]: bug 1084062
[User impact if declined]: Crash. It's currently #13 on Fx43
[Describe test coverage new/current, TreeHerder]: Crash has disappeared from Fx44 in the last few days.
[Risks and why]: Low
[String/UUID change made/needed]: None
Attachment #8668244 - Flags: approval-mozilla-aurora?
Blocks: 1212998
Comment on attachment 8668244 [details] [diff] [review]
readingpanel-context-check v0.1

Crash fix, works well on m-c. Approved for uplift to aurora.
Attachment #8668244 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
setting the flag for mark :)
Verified as fixed on Firefox for Android 43.0a2 (2015-10-26) using Samsung Galaxy Note 5 (Android 5.1.1)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.