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

Status: VERIFIED FIXED

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

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: fragment-sanity-check v0.1

This patch just adds the same listener cleanup added to Top Sites. It's just some defensive code.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: 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.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

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. :)

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ada7566c1f9d

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

(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 8

[:Margaret Leibovic]

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.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/be9f3f152aae

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ada7566c1f9d
https://hg.mozilla.org/mozilla-central/rev/be9f3f152aae

Comment 11

[Teodora Vermesan (:TeoVermesan)]

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 12

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8668244 [details] [diff] [review]
readingpanel-context-check v0.1

Crash fix, works well on m-c. Approved for uplift to aurora.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/68f959011549

Comment 15

[Carsten Book [:Tomcat]]

setting the flag for mark :)

Comment 16

[Teodora Vermesan (:TeoVermesan)]

Verified as fixed on Firefox for Android 43.0a2 (2015-10-26) using Samsung Galaxy Note 5 (Android 5.1.1)