793136 - java.lang.NullPointerException: at org.mozilla.gecko.BrowserApp.onPrepareOptionsMenu(BrowserApp.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Scoobidiver (away)]

There's one crash in 18.0a1/20120921: bp-4c42839b-8fa1-490c-b7e2-97f082120921.

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserApp.onPrepareOptionsMenu(BrowserApp.java:785)
	at org.mozilla.gecko.GeckoApp.invalidateOptionsMenu(GeckoApp.java:451)
	at org.mozilla.gecko.BrowserApp.onTabChanged(BrowserApp.java:82)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:375)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:364)
	at org.mozilla.gecko.Tab$5.run(Tab.java:371)
	at android.os.Handler.handleCallback(Handler.java:615)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4745)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:786)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.BrowserApp.onPrepareOptionsMenu%28BrowserApp.java%29

Comment 1

[Scoobidiver (away)]

It has been hit by 11 users in less than 2 hours. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1e56d3016820&tochange=48c4938eaf57

Comment 2

[Scoobidiver (away)]

It's #1 top crasher in today's build.

Comment 3

[Aaron Train [:aaronmt]]

bug 785994?

Comment 4

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

Sriram - Please take at this one. It's happening a lot.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Is there any STR for this? I was running a build with submenu over the weekend and browsed a lot. I never faced this crash.

Comment 6

[Chris Peterson [:cpeterson]]

Sriram, all the crash reports point to BrowserApp.java:785:

https://hg.mozilla.org/mozilla-central/file/e327e66a027d/mobile/android/base/BrowserApp.java#l785

Can tab.getURL() or tab.getContentType() return null? When you are comparing a someString.equals("Some string literal"), a safer style is to reverse the check like "Some string literal".equals(someString) because this style won't crash if someString is null.

Comment 7

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This is a precautionary patch (both at Java and Gecko side). This may work.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This is an updated patch using ternary operator (I failed to refresh my last patch).

Comment 9

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee445d27224

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8ee445d27224

Comment 11

[Andreea Pod]

I am able to reproduce this crash - STR:

1. install Phony add-on from addons.mozilla.org
2. go to Add-ons Manager > Phony and disable it
3. tap on back button et voila

Build: Firefox 18.0a1 (2012-09-25)
Device: Samnsun Galaxu Nexus
OS: Android 4.1.1

https://crash-stats.mozilla.com/report/index/bp-9d4f7e1f-dc56-49b1-b47d-a5f102120925

Comment 12

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

This patch only just landed in mozilla-central. It is not in a nightly yet. Check it tomorrow.

Comment 13

[Aaron Train [:aaronmt]]

I can still crash on latest-inbound (09/25) with this fix with the STR in comment #11.

D/AndroidRuntime( 7776): Shutting down VM
W/dalvikvm( 7776): threadid=1: thread exiting with uncaught exception (group=0x40d15300)
E/GeckoAppShell( 7776): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoAppShell( 7776): java.lang.NullPointerException
E/GeckoAppShell( 7776): 	at org.mozilla.gecko.BrowserApp.onPrepareOptionsMenu(BrowserApp.java:794)
E/GeckoAppShell( 7776): 	at org.mozilla.gecko.GeckoApp.invalidateOptionsMenu(GeckoApp.java:451)

Comment 14

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

(In reply to Aaron Train [:aaronmt] from comment #13)
> I can still crash on latest-inbound (09/25) with this fix with the STR in
> comment #11.
> 
> D/AndroidRuntime( 7776): Shutting down VM
> W/dalvikvm( 7776): threadid=1: thread exiting with uncaught exception
> (group=0x40d15300)
> E/GeckoAppShell( 7776): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1
> ("main")
> E/GeckoAppShell( 7776): java.lang.NullPointerException
> E/GeckoAppShell( 7776): 	at
> org.mozilla.gecko.BrowserApp.onPrepareOptionsMenu(BrowserApp.java:794)
> E/GeckoAppShell( 7776): 	at
> org.mozilla.gecko.GeckoApp.invalidateOptionsMenu(GeckoApp.java:451)

I tested this and found the the "Save as PDF" menu is null. How can that be? Well, when disabling the Phony add-on, the "Phony" menu is removed. However, the entire "Tools" menu is removed too. Therefore, there is no longer a "Save as PDF" menu and it is null.

Obviously, the submenu code regressed this. I am looking into why it regressed.

Comment 15

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

Attachment: additional patch

This patch is required to stop the add-on menu from colliding with a submenu. The ID for the first add-on menu is 0, the same as NO_ID constant used for submenu IDs. This patch uses an offset of 1000 for all add-on menu IDs. The offset is applied in Java since that's the part of the code that cares about the collisions.

Comment 16

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

I was able to crash without the patch. No crash with the patch.

Comment 17

[Thomas Stache]

The 20120926 build seems even worse, or it's that I installed a working Adblock Plus dev build, that adds a menu item. Unfortunately Fennec Android has no Safe Mode, so the browser is really dead, crashes seconds after startup!

https://crash-stats.mozilla.com/query/query?product=FennecAndroid&version=FennecAndroid%3A18.0a1&build_id=20120926030625&do_query=1#

Comment 18

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 664778 [details] [diff] [review]
additional patch

I get the problem. "tools" menu doesn't have an ID, and we remove it, if the ID of addon is "0". Do we need an offset like 1000? I believe an offset of 1 should be fine as we won't enter into the zone that android generates.

r+ with that change (if needed).

Comment 19

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

(In reply to Sriram Ramasubramanian [:sriram] from comment #18)
> ID of addon is "0". Do we need an offset like 1000?

Using 1 might work fine, but I like to go big, or go home :)

Comment 20

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/e80d2a8e2fc0

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e80d2a8e2fc0

Comment 22

[Thomas Stache]

What is the test story for this, is it possible to ensure issues like this are caught in try or tbpl in the future? The mere presence of (recommended) add-ons should never lead to startup problems :(

Comment 24

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

Not trying to jinx things, but I see a drop in these crashes over the last few days, and of the crashes I see from Sept 27, none are from the Sept 27th build. All are from previous builds.