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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

It's the third version after bug 793136 and bug 773089.
This one appeared in 19.0a1/20121018. Its regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac5700acf8b&tochange=cb573b9307e5

Here is a crash report: bp-26f3f9a7-7a4b-43be-8766-412f02121018.

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserApp.onPrepareOptionsMenu(BrowserApp.java:902)
	at org.mozilla.gecko.GeckoApp.invalidateOptionsMenu(GeckoApp.java:466)
	at org.mozilla.gecko.GeckoApp.onTabChanged(GeckoApp.java:243)
	at org.mozilla.gecko.BrowserApp.onTabChanged(BrowserApp.java:119)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:409)
	at org.mozilla.gecko.Tabs.notifyListeners(Tabs.java:398)
	at org.mozilla.gecko.Tabs$2.run(Tabs.java:142)
	at android.app.Activity.runOnUiThread(Activity.java:4591)
	at org.mozilla.gecko.Tabs.selectTab(Tabs.java:137)
	at org.mozilla.gecko.Tabs.loadUrl(Tabs.java:512)
	at org.mozilla.gecko.AwesomebarResultHandler.onActivityResult(AwesomebarResultHandler.java:27)
	at org.mozilla.gecko.ActivityHandlerHelper.handleActivityResult(ActivityHandlerHelper.java:220)
	at org.mozilla.gecko.GeckoApp.onActivityResult(GeckoApp.java:2492)
	at android.app.Activity.dispatchActivityResult(Activity.java:5192)
	at android.app.ActivityThread.deliverResults(ActivityThread.java:3137)
	at android.app.ActivityThread.handleSendResult(ActivityThread.java:3184)
	at android.app.ActivityThread.access$1100(ActivityThread.java:130)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1243)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	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)]

There are about 40 crashes per build so it seems easily reproducible.

Comment 2

[Chris AtLee [:catlee]]

Happens for me when I'm trying to open a new tab. I type e.g. 'la rafle', and select the Wikipedia search, and then firefox crashes.

Comment 3

[Chris Peterson [:cpeterson]]

Sriram, this BrowserApp.onPrepareOptionsMenu() crash is the #1 topcrash for Nightly 19. It looks just like bug 793136.

Comment 4

[Aaron Train [:aaronmt]]

This makes testing Nightly unusable (blocker status) on my Galaxy S.

Comment 5

[Brian Nicholson (:bnicholson)]

I'll take this - likely a regression from bug 800199.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Only set stubbed tab URLs for valid URIs

Bug 800199 sets the URL for tab stubs if userEntered is false. The thinking was that if the URI is not entered by the user, we can assume the URI is valid, so we can set it as the tab's URL. This turns out not to be a good test, though, because a number of actions can result in non-URIs being loaded where userEntered is false (e.g., clicking a search engine).

This patches does more a complete URI check before setting the tab's URL. If the URI is not valid, we wait for Gecko's Tab:Added before setting the tab's URL, which is what we always did before tab stubs.

Comment 7

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

Comment on attachment 674028 [details] [diff] [review]
Only set stubbed tab URLs for valid URIs

Is there a way we can add a test for this case?

Comment 8

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Comment on attachment 674028 [details] [diff] [review]
> Only set stubbed tab URLs for valid URIs
> 
> Is there a way we can add a test for this case?

Ideally, we'd have a test case for search engines, and that would have caught this. But since the tests are offline, we can't actually visit the search engine sites. Maybe we can still click a search engine entry and verify an engine URL is loaded, even though we'll get a 404?

Comment 9

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

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> (In reply to Mark Finkle (:mfinkle) from comment #7)
> > Comment on attachment 674028 [details] [diff] [review]
> > Only set stubbed tab URLs for valid URIs
> > 
> > Is there a way we can add a test for this case?
> 
> Ideally, we'd have a test case for search engines, and that would have
> caught this. But since the tests are offline, we can't actually visit the
> search engine sites. Maybe we can still click a search engine entry and
> verify an engine URL is loaded, even though we'll get a 404?

I was thinking of ways to simulate the situation. We could pass URLs on a command line. We could make a fake sessionstore.js with bad URLs. What is the exact situation we get into to cause this problem? What's the code path? We could just call Tabs.loadURL with a bogus URL, right?

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> I was thinking of ways to simulate the situation. We could pass URLs on a
> command line. We could make a fake sessionstore.js with bad URLs. What is
> the exact situation we get into to cause this problem? What's the code path?
> We could just call Tabs.loadURL with a bogus URL, right?

To simulate this particular situation, we could edit a bookmark entry to point to a bogus URI, then click it. That also results in the same crash here.

Comment 11

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/363b1a7f2cae

Leaving open for test case.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/363b1a7f2cae

Comment 13

[Chris Peterson [:cpeterson]]

(In reply to Brian Nicholson (:bnicholson) from comment #11)
> 
> Leaving open for test case.

Brian, are you still waiting for a test case? This crash looks like it has been fixed because it hasn't shown up in Socorro crash reports since you landed your fix on 2012-10-23.

Comment 14

[Brian Nicholson (:bnicholson)]

(In reply to Chris Peterson (:cpeterson) from comment #13)
> (In reply to Brian Nicholson (:bnicholson) from comment #11)
> > 
> > Leaving open for test case.
> 
> Brian, are you still waiting for a test case? This crash looks like it has
> been fixed because it hasn't shown up in Socorro crash reports since you
> landed your fix on 2012-10-23.

Yeah, I was going to add a simple test case as suggested in comment 7. I just filed bug 806465 so we don't need to leave this open.