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

Status: RESOLVED WORKSFORME

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Flaviu Cos]

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

Comment 1

[Flaviu Cos]

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)

Comment 2

[Flaviu Cos]

Last good build: 2014-02-13
First bad build: 2014-02-14

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a62bde1d6efe&tochange=d275eebfae04

Comment 3

[Aaron Train [:aaronmt]]

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?

Comment 4

[Aaron Train [:aaronmt]]

Found other steps to reproduce

double tap quickly on any empty tile

100% crash every time

Comment 5

[Aaron Train [:aaronmt]]

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 6

[Lucas Rocha (:lucasr)]

Attachment: Fix crash when double-tapping on empty top site spot (r=bnicholson)

Comment 7

[Lucas Rocha (:lucasr)]

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.

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8484970 [details] [diff] [review]
Fix crash when double-tapping on empty top site spot (r=bnicholson)

Just a sec.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Fix crash when double-tapping on empty top site spot (r=bnicholson)

Comment 10

[Lucas Rocha (:lucasr)]

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.

Comment 11

[Lucas Rocha (:lucasr)]

This is a top crasher in Fx33 btw.

Comment 12

[Brian Nicholson (: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]:
-----------------------------------------------------------------

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.

Comment 13

[Lucas Rocha (:lucasr)]

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

[Lucas Rocha (:lucasr)]

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

Comment 15

[Lucas Rocha (:lucasr)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6564ea470de4

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

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+

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Lucas, any reason why you haven't requested the patch for beta? Thanks

Comment 19

[Lucas Rocha (:lucasr)]

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

Comment 20

[Lucas Rocha (:lucasr)]

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

Comment 21

[Sylvestre Ledru [:Sylvestre]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f5b683c6c69a
https://hg.mozilla.org/releases/mozilla-beta/rev/e0c49c71cc55

Comment 23

[Flaviu Cos]

The crash is still reproducible.
Tested on Samsung Galaxy R (Android 2.3.4);

Comment 24

[Aaron Train [:aaronmt]]

Bugs are cheap, please file a new one with a crash report URL and steps to reproduce.

Comment 25

[Lucas Rocha (:lucasr)]

FWIW, I just realized that my patch actually fixes the crasher described in comment #5, not the one in the bug summary.

Comment 26

[Aaron Train [:aaronmt]]

Is this then not fixed Lucas?

Comment 27

[Lucas Rocha (:lucasr)]

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

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

filter on [mass-p5]

Comment 29

[u279076]

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.

Comment 30

[Ioana Chiorean]

Got into this while testing 34.0b6:
https://crash-stats.mozilla.com/report/index/1877a932-fd0f-4ea3-821d-981412141104
https://crash-stats.mozilla.com/report/index/18a1354d-956c-4b10-8de9-9f6d62141104

Comment 31

[Aaron Train [:aaronmt]]

Don't think we should de-prioritize reproducible crashes

Comment 32

[Kevin Brosnan [Ex-Mozilla]]

Plus based on discussion in triage. Fairly low volume crasher on release 100-200 crashes a week.

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.