Closed Bug 912524 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: Awesomescreen, defect, critical)

All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 26
Tracking Status
firefox25 --- unaffected
firefox26 + fixed
fennec 26+ ---

People

(Reporter: kairo, Assigned: mcomella)

References

Details

(Keywords: crash, topcrash, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(1 file, 1 obsolete file)

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
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.
Assignee: nobody → michael.l.comella
tracking-fennec: --- → 26+
Duplicate of this bug: 910835
Status: NEW → ASSIGNED
(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
(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).
Attached patch 01: Patch (band-aid) (obsolete) — Splinter Review
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?
Attachment #800505 - Flags: review?(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.
Attachment #800505 - Flags: review?(bnicholson) → review+
Attached patch 01a: PatchSplinter Review
> 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. :)
Attachment #800505 - Attachment is obsolete: true
Attachment #802434 - Flags: review?(bnicholson)
Attachment #802434 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/6d8738e8dc71
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
It does look like this went down since landing, so calling it fixed in status.
You need to log in before you can comment on or make changes to this bug.