Closed
Bug 912524
Opened 11 years ago
Closed 11 years ago
crash in java.lang.NullPointerException: at org.mozilla.gecko.home.BrowserSearch.showSuggestionsOptIn(BrowserSearch.java)
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Tracking
(firefox25 unaffected, firefox26+ fixed, fennec26+)
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)
1.53 KB,
patch
|
bnicholson
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Dupe of 910835?
Assignee | ||
Comment 2•11 years ago
|
||
*bug 910835
Reporter | ||
Comment 3•11 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.
Updated•11 years ago
|
Assignee: nobody → michael.l.comella
tracking-fennec: --- → 26+
Assignee | ||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•11 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•11 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•11 years ago
|
||
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 8•11 years ago
|
||
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•11 years ago
|
||
> 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)
Updated•11 years ago
|
Attachment #802434 -
Flags: review?(bnicholson) → review+
Assignee | ||
Comment 10•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/6d8738e8dc71
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6d8738e8dc71
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Comment 12•11 years ago
|
||
It does look like this went down since landing, so calling it fixed in status.
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•