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

VERIFIED FIXED in Firefox 43

Status

()

Firefox for Android
Reading List
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

(Blocks: 2 bugs)

unspecified
Firefox 44
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 unaffected, firefox43 verified, firefox44 verified)

Details

(crash signature)

Attachments

(2 attachments)

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.
Created attachment 8668243 [details] [diff] [review]
fragment-sanity-check v0.1

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)
Created attachment 8668244 [details] [diff] [review]
readingpanel-context-check v0.1

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+
(Assignee)

Updated

3 years ago
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.
(Assignee)

Updated

3 years ago
Keywords: leave-open

Comment 8

3 years ago
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+
(Assignee)

Updated

3 years ago
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/ada7566c1f9d
https://hg.mozilla.org/mozilla-central/rev/be9f3f152aae
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
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.
status-firefox42: --- → unaffected
status-firefox43: --- → affected
status-firefox44: fixed → verified
(Assignee)

Updated

3 years ago
Blocks: 1084062
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?

Updated

3 years ago
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 :)
status-firefox43: affected → fixed
Verified as fixed on Firefox for Android 43.0a2 (2015-10-26) using Samsung Galaxy Note 5 (Android 5.1.1)
Status: RESOLVED → VERIFIED
status-firefox43: fixed → verified
You need to log in before you can comment on or make changes to this bug.