912524 - crash in java.lang.NullPointerException: at org.mozilla.gecko.home.BrowserSearch.showSuggestionsOptIn(BrowserSearch.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Robert Kaiser]

This bug was filed from the Socorro interface and is 
report bp-55288bb7-2908-4aea-bffa-20a542130830.
=============================================================

Java Stack Trace:	

java.lang.NullPointerException
	at org.mozilla.gecko.home.BrowserSearch.showSuggestionsOptIn(BrowserSearch.java:439)
	at org.mozilla.gecko.home.BrowserSearch.setSearchEngines(BrowserSearch.java:429)
	at org.mozilla.gecko.home.BrowserSearch.access$500(BrowserSearch.java:62)
	at org.mozilla.gecko.home.BrowserSearch$3.run(BrowserSearch.java:288)
	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:4898)
	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:1006)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:773)
	at dalvik.system.NativeStart.main(Native Method)

This is a new crash in 26.0a1 Nightly since 2013-08-21, currently #4 topcrash on Nightly and mostly within the first minute of usage.

The comment on this one implies it happens while typing in the awesomebar, and "showSuggestionsOptIn" sounds in line with that.

Find more such reports at https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A%20at%20org.mozilla.gecko.home.BrowserSearch.showSuggestionsOptIn%28BrowserSearch.java%29

Comment 1

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

Dupe of 910835?

Comment 2

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

*bug 910835

Comment 3

[Robert Kaiser]

Sounds like it, but the other bug does not have the Crash Signature field assigned so it's not found by the crash-stats system.

Comment 5

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

(Assuming repo steps from bug 910835)

Suspected issue: We have one BrowserSearch instance which emits a "SearchEngines:Get" message (onViewCreated) that is responded to with "SearchEngines:Data" which handled in a background thread (handleMessage). This handling method inflates the ViewStub (showSuggestionsOptIn).

However, according to the ViewStub docs [1], an inflated ViewStub is removed from the View hierarchy. Thus, when the first tab browser search screen is opened, the "SearchEngines:Get" message will sent. If a new browser search screen is opened, sending another message, before this first message's response ("SearchEngines:Data") is handled, the response handling for the first emitted message will inflate the single ViewStub instance. Thus, the second inflation will return null, throwing a NullPointerException.

Typically, onDestroyView, which sets mView = null, will get called when the browser search screen is closed. Since setSearchEngines() checks for mView == null, this prevents this inflation from happening if only one tab has been opened.

Presumably, this can happen any time the messages are handled slowly, but I imagine I can reliably repo on startup because Gecko (and thus message handling) has not yet started up.

Note to self: Ensure mView will not be set to null while in the setSearchEngines method.

[1]: http://developer.android.com/reference/android/view/ViewStub.html

Comment 6

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

(In reply to Michael Comella (:mcomella) from comment #5)
> Typically, onDestroyView, which sets mView = null, will get called when the
> browser search screen is closed. Since setSearchEngines() checks for mView
> == null, this prevents this inflation from happening if only one tab has
> been opened.

Also, and perhaps more correctly, the "SearchEngines:Data" event listener is unregistered (but re-registered when the new view is created, which creates the issue).

Comment 7

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

Attachment: 01: Patch (band-aid)

See comment 5 for the suspected issue.

What we really want to do here is ignore the response requested from the first BrowserSearch screen shown. However, to do this, we likely need a unique "message-response" ID. This can be done by reworking the message passing system and attaching this to all messages (quite unreasonable) or by passing an ID as extra value in the message and passing this value back in the response. I like the latter but I'm unsure of the implications of changing the message contents so I put it aside for this patch.

As such, we're left with largely band-aid fixes. I thought to unregister the handler after we handle the message for the first time, however, there may be legitimate reasons to handle Search Engine updates multiple times for a single Browser Search screen. Thus, I decided to just prevent the reinflation of the ViewStub in question, the bottom-most symptom.

Since this feels like a band-aid, I prefer adding an ID to the message contents. What do you think?

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 800505 [details] [diff] [review]
01: Patch (band-aid)

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

I agree that having a unique ID here would be a better solution, but I think we should implement that as a generic part of our messaging API instead of just for this specific case. Doing that would allow us to easily handle all similar situations where we have these one-off request/response situations.

::: mobile/android/base/home/BrowserSearch.java
@@ +447,5 @@
>  
>      private void showSuggestionsOptIn() {
> +        // An inflated ViewStub is removed from the View hierarchy so a second call to
> +        // findViewById will return null. We avoid this by not ovewriting the already inflated view.
> +        final boolean wasInflated = mSuggestionsOptInPrompt != null ? true : false;

Nit: The '? true : false' here is redundant.

@@ +450,5 @@
> +        // findViewById will return null. We avoid this by not ovewriting the already inflated view.
> +        final boolean wasInflated = mSuggestionsOptInPrompt != null ? true : false;
> +        if (!wasInflated) {
> +            mSuggestionsOptInPrompt = ((ViewStub) mView.findViewById(R.id.suggestions_opt_in_prompt)).inflate();
> +        }

Could you just do 'else return' here and keep this logic together? I don't think the setText() below this needs to be called twice since there's virtually no chance the suggestion engine has changed in this period.

Comment 9

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

Attachment: 01a: Patch

> Could you just do 'else return' here and keep this logic together?
> I don't think the setText() below this needs to be called twice
> since there's virtually no chance the suggestion engine has
> changed in this period.

Fair enough. I made the logic even simpler so I'm requested a re-review. :)

Comment 10

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

https://hg.mozilla.org/integration/fx-team/rev/6d8738e8dc71

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d8738e8dc71

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

It does look like this went down since landing, so calling it fixed in status.