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

RESOLVED FIXED in Firefox 26

Status

()

Firefox for Android
Awesomescreen
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Robert Kaiser, Assigned: mcomella)

Tracking

({crash, topcrash})

unspecified
Firefox 26
All
Android
crash, topcrash
Points:
---

Firefox Tracking Flags

(firefox25 unaffected, firefox26+ fixed, fennec26+)

Details

(Whiteboard: [native-crash][startupcrash], crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Assignee)

Comment 1

4 years ago
Dupe of 910835?
(Assignee)

Comment 2

4 years ago
*bug 910835
(Reporter)

Comment 3

4 years ago
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
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
(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
(Assignee)

Comment 6

4 years ago
(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).
(Assignee)

Comment 7

4 years ago
Created attachment 800505 [details] [diff] [review]
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?
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+
(Assignee)

Comment 9

4 years ago
Created attachment 802434 [details] [diff] [review]
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. :)
Attachment #800505 - Attachment is obsolete: true
Attachment #802434 - Flags: review?(bnicholson)
Attachment #802434 - Flags: review?(bnicholson) → review+
(Assignee)

Comment 10

4 years ago
https://hg.mozilla.org/integration/fx-team/rev/6d8738e8dc71
https://hg.mozilla.org/mozilla-central/rev/6d8738e8dc71
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
It does look like this went down since landing, so calling it fixed in status.
status-firefox26: affected → fixed
tracking-firefox26: ? → +
You need to log in before you can comment on or make changes to this bug.