Closed Bug 935542 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect, critical)

28 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 30
Tracking Status
firefox28 --- verified
firefox29 --- verified
firefox30 --- verified
b2g-v1.3 --- fixed
fennec 28+ ---

People

(Reporter: aaronmt, Assigned: lucasr)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

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)
Version: Firefox 26 → Firefox 28
Regression from bug 925082?
(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.
This is dominating crash-stats on 28.0b1 as far as I can tell
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: --- → ?
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.
Attachment #8374833 - Flags: review?(mark.finkle)
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.
Attachment #8374834 - Flags: review?(mark.finkle)
Blocks: 960202
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.
Attachment #8374833 - Flags: review?(mark.finkle) → review+
Attachment #8374834 - Flags: review?(mark.finkle) → review+
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
Attachment #8374833 - Flags: approval-mozilla-beta?
Attachment #8374833 - Flags: approval-mozilla-aurora?
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
Attachment #8374834 - Flags: approval-mozilla-beta?
Attachment #8374834 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/fbd6ad52ace7
https://hg.mozilla.org/mozilla-central/rev/6e3808d2f605
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
tracking-fennec: ? → 28+
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.
Attachment #8374833 - Flags: approval-mozilla-beta?
Attachment #8374833 - Flags: approval-mozilla-beta+
Attachment #8374833 - Flags: approval-mozilla-aurora?
Attachment #8374833 - Flags: approval-mozilla-aurora+
Attachment #8374834 - Flags: approval-mozilla-beta?
Attachment #8374834 - Flags: approval-mozilla-beta+
Attachment #8374834 - Flags: approval-mozilla-aurora?
Attachment #8374834 - Flags: approval-mozilla-aurora+
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
Keywords: steps-wanted
BTW, after this fix was landed, I could not reproduce this crash with the same steps on V28 beta6 and latest nightly.
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)
You need to log in before you can comment on or make changes to this bug.