935542 - crash in java.lang.NullPointerException: at org.mozilla.gecko.home.TopSitesPage$EditPinnedSiteListener.onSiteSelected(TopSitesPage.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-7d9bd778-d336-43a6-9e0a-ff93e2131105.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.home.TopSitesPage$EditPinnedSiteListener.onSiteSelected(TopSitesPage.java:403)
	at org.mozilla.gecko.home.PinSiteDialog$2.onKey(PinSiteDialog.java:115)
	at android.view.View.dispatchKeyEvent(View.java:7085)
	at android.view.ViewGroup.dispatchKeyEvent(ViewGroup.java:1361)

Comment 1

[:Margaret Leibovic]

Regression from bug 925082?

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to :Margaret Leibovic from comment #1)
> Regression from bug 925082?

Doesn't seem so. From the looks of it, it seems the NPE is caused by a null mOnSiteSelectedListener or maybe a null activity once onSiteSelected is called. The lines to exactly match the code anymore.

Comment 3

[Aaron Train [:aaronmt]]

This is dominating crash-stats on 28.0b1 as far as I can tell

Comment 4

[Lucas Rocha (:lucasr)]

Attachment: PinSiteDialog fragment should be owned by TopSitesPanel (r=mfinkle)

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Unset site selection listener when PinSiteDialog gets destroyed (r=mfinkle)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8374833 [details] [diff] [review]
PinSiteDialog fragment should be owned by TopSitesPanel (r=mfinkle)

One the issues here is that the dialog fragment is owned by the activity which means the TopSitesPanel might get detached from the activity and the dialog could still stick around (enough time to catch a key event and trigger the OnSiteSelectedListener).

This patch corrects this by managing PinSiteDialog in the TopSitePanel's fragment manager.

Comment 7

[Lucas Rocha (:lucasr)]

Comment on attachment 8374834 [details] [diff] [review]
Unset site selection listener when PinSiteDialog gets destroyed (r=mfinkle)

Extra safety measure. Ensure that the OnSiteSelectedListener will not be triggered after the PinSiteDialog is destroyed.

FYI: this bug is among the top crashers across all our channels.

Comment 8

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

Comment on attachment 8374833 [details] [diff] [review]
PinSiteDialog fragment should be owned by TopSitesPanel (r=mfinkle)

I had to read-up on getChildFragmentManager. Looks like the right approach.

Comment 9

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/fbd6ad52ace7
https://hg.mozilla.org/integration/fx-team/rev/6e3808d2f605

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8374833 [details] [diff] [review]
PinSiteDialog fragment should be owned by TopSitesPanel (r=mfinkle)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probably the new about:home stuff released in Fx26. 
User impact if declined: One of top crashers in Fx28 right now. 
Testing completed (on m-c, etc.): No easy way to reproduce it so hard to tell if this is actually going to fix the bug. This is a very educated guess though. Let's land this in nightly, and gradually land in aurora and then beta to see how crashstats looks. 
Risk to taking this patch (and alternatives if risky): Fairly low, small patches with no functional change. 
String or IDL/UUID changes made by this patch: n/a

Comment 11

[Lucas Rocha (:lucasr)]

Comment on attachment 8374834 [details] [diff] [review]
Unset site selection listener when PinSiteDialog gets destroyed (r=mfinkle)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Probably the new about:home stuff released in Fx26. 
User impact if declined: One of top crashers in Fx28 right now. 
Testing completed (on m-c, etc.): No easy way to reproduce it so hard to tell if this is actually going to fix the bug. This is a very educated guess though. Let's land this in nightly, and gradually land in aurora and then beta to see how crashstats looks. 
Risk to taking this patch (and alternatives if risky): Fairly low, small patches with no functional change. 
String or IDL/UUID changes made by this patch: n/a

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fbd6ad52ace7
https://hg.mozilla.org/mozilla-central/rev/6e3808d2f605

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8374833 [details] [diff] [review]
PinSiteDialog fragment should be owned by TopSitesPanel (r=mfinkle)

To get the crash data we should uplift this to branches now in order to have it in 28.0b4 mobile next week and gathering feedback with time to backout or make any tweaks if needed before ship.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5be150c0e8a1
https://hg.mozilla.org/releases/mozilla-aurora/rev/70f49278c2a1

https://hg.mozilla.org/releases/mozilla-beta/rev/151a690db415
https://hg.mozilla.org/releases/mozilla-beta/rev/0609dee24b0b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/151a690db415
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/0609dee24b0b

Comment 16

[Peipei Cheng (needinfo if you need my action)]

This is reproducible starting from V27.

Here are the reproduce steps:

1. Launch Firefox
2. Clear all user data, so there is some unpinned icon on Topsites page
3. Tap on support.mozilla.org and at the same time, quickly tap on "+" to add a site to pin
   You will notice that page 'support.mozilla.org' opens at background while website picker appears on the top layer.
4. Tap on any websites to Pin.
   --> Firefox crashes.

I took a video for the STR: https://mozilla.box.com/s/qjww078t5qrn5epbl5sm

Comment 17

[Peipei Cheng (needinfo if you need my action)]

BTW, after this fix was landed, I could not reproduce this crash with the same steps on V28 beta6 and latest nightly.

Comment 18

[Cristina Madaras, QA [:CristinaM]]

Verified as fixed in builds:
28.0
29.0
30.0
Latest Nightly 33.0a1 (2014-06-10)

Devices: 
Asus Transformer Pad TF300T (Android 4.2.1)
Motorola Razr (Android 4.0.4)