1138529 - crash in java.lang.NullPointerException: at org.mozilla.gecko.BrowserApp$15.onStartEditing(BrowserApp.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-d03c4b40-e5a8-4e25-b3ad-aebe72150227.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserApp$15.onStartEditing(BrowserApp.java:880)
	at org.mozilla.gecko.toolbar.BrowserToolbar.startEditing(BrowserToolbar.java:812)
	at org.mozilla.gecko.toolbar.BrowserToolbarNewTablet.startEditing(BrowserToolbarNewTablet.java:183)
	at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java:1961)
	at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java:1937)
	at org.mozilla.gecko.BrowserApp.access$600(BrowserApp.java:144)
	at org.mozilla.gecko.BrowserApp$10.onActivate(BrowserApp.java:837)
	at org.mozilla.gecko.toolbar.BrowserToolbar$1.onClick(BrowserToolbar.java:241)
	at android.view.View.performClick(View.java:4445)
	at android.view.View$PerformClick.run(View.java:18446)
	at android.os.Handler.handleCallback(Handler.java:733)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:136)
	at android.app.ActivityThread.main(ActivityThread.java:5158)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:515)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:732)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:566)
	at dalvik.system.NativeStart.main(Native Method)

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

The failing line:

mDoorHangerPopup.disable();

It gets initialized from onWindowFocusChanged: soft-dep bug 973085.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

<AaronMT> mcomella: tablet code execution on phones?
<AaronMT> mcomella: second top device is a GS3

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to investigate.

Comment 4

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

Mike - Let's see if there is something we can do to handle this ASAP.

Comment 5

[Aaron Train [:aaronmt]]

Comment: 

  * "Happens almost (always?) when Android was suspended

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Precedent: bug 1090347 and potentially bug 1031365.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(fixing block)

Alternative diagnosis (to comment 1): something is getting inlined but bug 1031365 would imply otherwise.

The easiest thing to do is to initialize mDoorHangerPopup in onCreate instead because it doesn't appear to rely on the initialization in onWindowFocusChanged in anyway.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: bz://1138529/mcomella

/r/4791 - Bug 1138529 - Move doorhanger popup initialization to onCreate. r=liuche

Pull down this commit:

hg pull review -r 31692a858f439828cb663cf01a9e40f242e5eb1b

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ecae8c63acd

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 8573432 [details]
MozReview Request: bz://1138529/mcomella

https://reviewboard.mozilla.org/r/4789/#review3885

::: mobile/android/base/GeckoApp.java
(Diff revision 1)
> -        mDoorHangerPopup = new DoorHangerPopup(this);

We care a lot about startup time, so I'd probably just keep this here and put a null check in the Toolbar usage. We never use the DoorHanger before content is loaded anyways, so initializeChrome is the right place for it.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Chenxia Liu [:liuche] from comment #11)
> We care a lot about startup time, so I'd probably just keep this here and
> put a null check in the Toolbar usage. We never use the DoorHanger before
> content is loaded anyways, so initializeChrome is the right place for it.

Probably easier because we won't have to move it back later, and there are fewer possible side effects (which is important for uplift): wfm.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8573432 [details]
MozReview Request: bz://1138529/mcomella

/r/4791 - Bug 1138529 - Add null checks mDoorHangerPopup access in toolbar editing state. r=liuche

Pull down this commit:

hg pull review -r 44dbeebd1ea6ed31f8437decb52c75457f27eb7c

Comment 14

[Chenxia Liu [:liuche]]

Comment on attachment 8573432 [details]
MozReview Request: bz://1138529/mcomella

https://reviewboard.mozilla.org/r/4789/#review3891

It's possible there are other places where we try to dismiss the doorhanger before it's created, but I prefer maintaining the onCreate/intializeChrome for now. And creating and then destroying the DoorHanger before it's shown is not in line with good performance practices anyways.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8573432 [details]
MozReview Request: bz://1138529/mcomella

Approval Request Comment
[Feature/regressing bug #]: New tablet UI, it seems

[User impact if declined]:
  Users crash! STR unknown.

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]:
  Low - add some null checks.
 
[String/UUID change made/needed]: None

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5fcb418e9aa9

Comment 18

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

(In reply to Michael Comella (:mcomella) from comment #16)
> [Risks and why]:
>   Low - add some null checks.

Even something as simply as a null check has burned us in the past. Let's give this a couple of days on Nightly before uplifting as we have time to do so and still take the fix in Beta 4.

Comment 19

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

Comment on attachment 8573432 [details]
MozReview Request: bz://1138529/mcomella

Now that the fix has been on Nightly for a couple of days, let's take it in Beta 4. Beta+ Aurora+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b06724f2ed0a

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5b2516e8aea9

Comment 23

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1138529 - Add null checks mDoorHangerPopup access in toolbar editing state. r=liuche

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We fixed this without the soft-dep so removing it.