Closed Bug 1041448 Opened 10 years ago Closed 4 years ago

crash in java.lang.NullPointerException: at org.mozilla.gecko.home.PinSiteDialog$3.onFocusChange(PinSiteDialog.java)

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

ARM
Android
defect
Not set
critical

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox33+ fixed, firefox34+ fixed, firefox35 fixed, firefox39 affected, firefox40 affected, firefox41 affected, firefox42 affected, fennec+)

RESOLVED WORKSFORME
Firefox 35
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 + fixed
firefox34 + fixed
firefox35 --- fixed
firefox39 --- affected
firefox40 --- affected
firefox41 --- affected
firefox42 --- affected
fennec + ---

People

(Reporter: cos_flaviu, Unassigned)

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-b6f621f0-7aba-45f5-abfd-5b2832140721.
=============================================================

Environment: 
Device: Samsung Galaxy R (Android 2.3.4);
Build: Nightly 33.0a1 (2014-07-20);

Steps to reproduce:
1. Tap on an empty grid from top sites;
2. Change device orientation to landscape.

Expected result:
The device orientation is successfully changed.

Actual result:
Firefox crashes.

Notes:
Not reproducible on Android 4.x
Stack trace:
java.lang.NullPointerException
	at org.mozilla.gecko.home.PinSiteDialog$3.onFocusChange(PinSiteDialog.java:131)
	at android.view.View.onFocusChanged(View.java:2831)
	at android.widget.TextView.onFocusChanged(TextView.java:7089)
	at android.widget.EditText.onFocusChanged(EditText.java:239)
	at android.view.View.handleFocusGainInternal(View.java:2654)
	at android.view.View.requestFocus(View.java:3809)
	at android.view.ViewGroup.onRequestFocusInDescendants(ViewGroup.java:1079)
	at android.view.ViewGroup.requestFocus(ViewGroup.java:1035)
	at android.view.ViewGroup.onRequestFocusInDescendants(ViewGroup.java:1079)
	at android.view.ViewGroup.requestFocus(ViewGroup.java:1035)
	at android.view.ViewGroup.onRequestFocusInDescendants(ViewGroup.java:1079)
	at android.view.ViewGroup.requestFocus(ViewGroup.java:1035)
	at android.view.ViewGroup.onRequestFocusInDescendants(ViewGroup.java:1079)
	at android.view.ViewGroup.requestFocus(ViewGroup.java:1035)
	at android.view.ViewGroup.onRequestFocusInDescendants(ViewGroup.java:1079)
	at android.view.ViewGroup.requestFocus(ViewGroup.java:1038)
	at android.view.View.requestFocus(View.java:3760)
	at android.view.ViewRoot.performTraversals(ViewRoot.java:1259)
	at android.view.ViewRoot.handleMessage(ViewRoot.java:1897)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3691)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:907)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:665)
	at dalvik.system.NativeStart.main(Native Method)
tracking-fennec: --- → ?
Keywords: reproducible
Last good build: 2014-02-13
First bad build: 2014-02-14

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a62bde1d6efe&tochange=d275eebfae04
In the fx-team merge I see

6e3808d2f605	Lucas Rocha — Bug 935542 - Unset site selection listener when PinSiteDialog gets destroyed (r=mfinkle)
fbd6ad52ace7	Lucas Rocha — Bug 935542 - PinSiteDialog fragment should be owned by TopSitesPanel (r=mfinkle)

Lucas?
Flags: needinfo?(lucasr.at.mozilla)
Assignee: nobody → lucasr.at.mozilla
tracking-fennec: ? → +
Found other steps to reproduce

double tap quickly on any empty tile

100% crash every time
Stack on Nightly (09/04) on my Nexus 5 (4.4.4)

Double tap quickly on any empty tile before the picker comes up

E/GeckoAppShell(24849): java.lang.IllegalStateException: Fragment already added: PinSiteDialog{428aa080 #0 pin_site}
E/GeckoAppShell(24849): 	at android.support.v4.app.FragmentManagerImpl.addFragment(FragmentManager.java:1192)
E/GeckoAppShell(24849): 	at android.support.v4.app.BackStackRecord.run(BackStackRecord.java:616)
E/GeckoAppShell(24849): 	at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1484)
E/GeckoAppShell(24849): 	at android.support.v4.app.FragmentManagerImpl$1.run(FragmentManager.java:450)
E/GeckoAppShell(24849): 	at android.os.Handler.handleCallback(Handler.java:733)
E/GeckoAppShell(24849): 	at android.os.Handler.dispatchMessage(Handler.java:95)
E/GeckoAppShell(24849): 	at android.os.Looper.loop(Looper.java:136)
E/GeckoAppShell(24849): 	at android.app.ActivityThread.main(ActivityThread.java:5001)
E/GeckoAppShell(24849): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoAppShell(24849): 	at java.lang.reflect.Method.invoke(Method.java:515)
E/GeckoAppShell(24849): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785)
E/GeckoAppShell(24849): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601)
E/GeckoAppShell(24849): 	at dalvik.system.NativeStart.main(Native Method)
Comment on attachment 8484970 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

An isAdded() check is enough here.
Attachment #8484970 - Flags: review?(bnicholson)
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 8484970 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

Just a sec.
Attachment #8484970 - Attachment is obsolete: true
Attachment #8484970 - Flags: review?(bnicholson)
Comment on attachment 8484972 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

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

Only add dialog fragment if it's not present yet.
Attachment #8484972 - Flags: review?(bnicholson)
This is a top crasher in Fx33 btw.
Comment on attachment 8484972 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

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

The example at http://developer.android.com/reference/android/app/DialogFragment.html shows them removing a fragment if it exists instead of ignoring the show. One advantage might be that if the second edit call is for a different entry, the fragment would show that entry instead of the stale one. I can't think of any easily reproducible situations where this would happen, but something to consider as an alternative.
Attachment #8484972 - Flags: review?(bnicholson) → review+
(In reply to Brian Nicholson (:bnicholson) from comment #12)
> Comment on attachment 8484972 [details] [diff] [review]
> Fix crash when double-tapping on empty top site spot (r=bnicholson)
> 
> Review of attachment 8484972 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The example at
> http://developer.android.com/reference/android/app/DialogFragment.html shows
> them removing a fragment if it exists instead of ignoring the show. One
> advantage might be that if the second edit call is for a different entry,
> the fragment would show that entry instead of the stale one. I can't think
> of any easily reproducible situations where this would happen, but something
> to consider as an alternative.

Good point. But removing existing dialogs involves clearing the backstack, which makes me a bit nervous. And, as you said, this is a very likely type of interaction. Let's keep the original approach.
Comment on attachment 8484972 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

Approval Request Comment
[Feature/regressing bug #]: bug 935542
[User impact if declined]: Top crasher #11 in Fx34 right now.
[Describe test coverage new/current, TBPL]: Local testing only. Confirmed that it fixes the issue. Let's bake it in Nightly for a few days then uplift to Aurora.
[Risks and why]: Low, just ensures a dialog is not re-created if it's already visible. 
[String/UUID change made/needed]: n/a
Attachment #8484972 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6564ea470de4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Comment on attachment 8484972 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

Let's see if crash volume changes over the weekend. Aurora+
Attachment #8484972 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Lucas, any reason why you haven't requested the patch for beta? Thanks
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Sylvestre Ledru [:sylvestre] from comment #18)
> Lucas, any reason why you haven't requested the patch for beta? Thanks

I didn't see it in the top crasher list there. But maybe we should uplift anyway. Requesting now.
Flags: needinfo?(lucasr.at.mozilla)
Comment on attachment 8484972 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

Approval Request Comment
[Feature/regressing bug #]: bug 935542
[User impact if declined]: Top crasher #11 in Fx34 right now.
[Describe test coverage new/current, TBPL]: Local testing only. Confirmed that it fixes the issue. Let's bake it in Nightly for a few days then uplift to Aurora.
[Risks and why]: Low, just ensures a dialog is not re-created if it's already visible. 
[String/UUID change made/needed]: n/a
Attachment #8484972 - Flags: approval-mozilla-beta?
Comment on attachment 8484972 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

In comment #11, you mentioned it is a top crash
Attachment #8484972 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
The crash is still reproducible.
Tested on Samsung Galaxy R (Android 2.3.4);
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bugs are cheap, please file a new one with a crash report URL and steps to reproduce.
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
FWIW, I just realized that my patch actually fixes the crasher described in comment #5, not the one in the bug summary.
Is this then not fixed Lucas?
Flags: needinfo?(lucasr.at.mozilla)
(In reply to Aaron Train [:aaronmt] from comment #26)
> Is this then not fixed Lucas?

I'm afraid not. I suspect this bug has different steps to reproduce. The crash-on-double-tap (described in comment #5) was fixed with my patch.
Flags: needinfo?(lucasr.at.mozilla)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
filter on [mass-p5]
Priority: -- → P5
Please forgive me if I'm way off base as I'm new to this whole Android thing. Since we're not actively backing out something here I'm wondering if we can spin off the remaining work to a follow-up bug. It's hard/confusing to track a bug that is flagged as fixed but also reopened.
Don't think we should de-prioritize reproducible crashes
tracking-fennec: + → ?
Priority: P5 → --
Plus based on discussion in triage. Fairly low volume crasher on release 100-200 crashes a week.
tracking-fennec: ? → +
Assignee: lucasr.at.mozilla → nobody
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.gecko.home.PinSiteDialog$3.onFocusChange(PinSiteDialog.java)] → [@ java.lang.NullPointerException: at org.mozilla.gecko.home.PinSiteDialog$3.onFocusChange(PinSiteDialog.java)] [@ java.lang.NullPointerException: at org.mozilla.gecko.home.PinSiteDialog$3.onFocusChange]

Closing because no crashes reported for 12 weeks.

Status: REOPENED → RESOLVED
Closed: 10 years ago4 years ago
Resolution: --- → WORKSFORME
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.