1100403 - crash in java.lang.NullPointerException: at org.mozilla.gecko.home.BrowserSearch.access$500(BrowserSearch.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-ad72a320-ac58-4032-8fe5-a41412141116.
=============================================================

ava.lang.NullPointerException
	at org.mozilla.gecko.home.BrowserSearch.access$500(BrowserSearch.java:67)
	at org.mozilla.gecko.home.BrowserSearch$4.run(BrowserSearch.java:355)
	at android.os.Handler.handleCallback(Handler.java:730)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:5175)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:525)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:737)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:553)
	at dalvik.system.NativeStart.main(Native Method)

Comment 1

[Christian Schmitz (:tynn)]

This might have been fixed with Bug 1186683 as well.

Comment 2

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

This crashed 12 times in 28 days – it's probably not worth looking deeply into this bug over the other top crashers we have.

Comment 3

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

Also, crashes occurred in 44, where bug 1186683 is landed, so I don't think it fixed this bug.

Comment 4

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #2)
> This crashed 12 times in 28 days – it's probably not worth looking deeply
> into this bug over the other top crashers we have.

I saw multiple similar signatures related to BrowserSearch.access. With these reports combined, this is crashing a lot more often than 12 times in the last 28 days.

Comment 5

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #3)
> Also, crashes occurred in 44, where bug 1186683 is landed, so I don't think
> it fixed this bug.

I wonder if bug 1186683 even made this crash occur more often, since most of these new stacks appear to happen only in 44.

Comment 6

[:Margaret Leibovic]

Looking into this a bit, I wonder if mAdapter is null here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#1127

It only gets set in onActivityCreated:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#388

And if we're destroying/creating the fragment more often, I suspect we can get into race issues with loaders.

Comment 7

[Richard Newman [:rnewman]]

Yeah, two loaders racing -- one will reset mAdapter to null. Shared mutable state is the enemy :/

Comment 8

[:Margaret Leibovic]

liuche or ahunt, can one of you write a patch here?

Comment 9

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #8)
> liuche or ahunt, can one of you write a patch here?

Looking into it right now!

Comment 10

[Andrzej Hunt :ahunt]

I've got one patch which would prevent us from using the adapter if null, however the last trace that was linked seems to be a separate issue ("java.lang.NullPointerException: Attempt to invoke virtual method 'boolean java.lang.String.startsWith(java.lang.String)' on a null object reference") - I wonder it that's in:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#546

or

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/home/BrowserSearch.java#564

Trace:
https://crash-stats.mozilla.com/report/index/aa12bdb8-1dca-41a2-a73b-626fd2160115

Comment 11

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1100403 - Don't read mAdapter if null r?margaret

Review commit: https://reviewboard.mozilla.org/r/31063/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31063/

Comment 12

[:Margaret Leibovic]

Comment on attachment 8708465 [details]
MozReview Request: Bug 1100403 - Don't read mAdapter if null r?margaret

https://reviewboard.mozilla.org/r/31063/#review27879

Thanks for the quick fix. Let's also request uplift for this one and see if we can get it into the next beta.

Comment 13

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/80e55e78a44c60640f975722caff586da3af65fe
Bug 1100403 - Don't read mAdapter if null r=margaret

Comment 14

[Andrzej Hunt :ahunt]

Comment on attachment 8708465 [details]
MozReview Request: Bug 1100403 - Don't read mAdapter if null r?margaret

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: 30th top crasher in current beta (44.0b8)
[Describe test coverage new/current, TreeHerder]: no tests - not reproducable locally.
[Risks and why]: low-risk: add null check where object was previously accessed directly.
[String/UUID change made/needed]: None.

(Separate patch required for beta due to path changes)

Comment 15

[Andrzej Hunt :ahunt]

Attachment: 1100403_backport_beta.patch

Separate patch for beta due to path changes:

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: 30th top crasher in current beta (44.0b8)
[Describe test coverage new/current, TreeHerder]: no tests - not reproducable locally.
[Risks and why]: low-risk: add null check where object was previously accessed directly.
[String/UUID change made/needed]: None.

(Separate patch required for beta due to path changes)

Comment 16

[Richard Newman [:rnewman]]

I'm in favor of a band-aid to avoid a NPE, but this is the symptom of a more significant lifecycle problem. The loader should capture its adapter, or otherwise untie this knot.

Please file a follow-up to do this right -- more 'final', please!


(In reply to Andrzej Hunt :ahunt from comment #10)

> however the last trace that was linked seems to be a separate issue
> ("java.lang.NullPointerException: Attempt to invoke virtual method 'boolean
> java.lang.String.startsWith(java.lang.String)' on a null object reference")

This kinda doesn't make sense. The DB URL must be non-null, we null-check host, stripCommonSubdomains doesn't return null, so perhaps this is inlined code?

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/80e55e78a44c

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8708500 [details] [diff] [review]
1100403_backport_beta.patch

Fennec crash fix, makes sense to uplift to Beta44 as Fennec stability remains a big concern for this cycle.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35bc14c546b1
https://hg.mozilla.org/releases/mozilla-aurora/rev/9667c15ae9c9

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8708465 [details]
MozReview Request: Bug 1100403 - Don't read mAdapter if null r?margaret

Just like with beta.

Comment 21

[u421692]

Will verify this after checking in crash-stats that this crash signature will not be present.

Comment 22

[Andrzej Hunt :ahunt]

(In reply to Richard Newman [:rnewman] from comment #16)
> I'm in favor of a band-aid to avoid a NPE, but this is the symptom of a more
> significant lifecycle problem. The loader should capture its adapter, or
> otherwise untie this knot.
> 
> Please file a follow-up to do this right -- more 'final', please!

Done at bug 1240797.

> 
> 
> (In reply to Andrzej Hunt :ahunt from comment #10)
> 
> > however the last trace that was linked seems to be a separate issue
> > ("java.lang.NullPointerException: Attempt to invoke virtual method 'boolean
> > java.lang.String.startsWith(java.lang.String)' on a null object reference")
> 
> This kinda doesn't make sense. The DB URL must be non-null, we null-check
> host, stripCommonSubdomains doesn't return null, so perhaps this is inlined
> code?

I really have no idea what's happening here - however it doesn't seem like a common cause of crashing, so probably not worth prioritising right now?

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/35bc14c546b1

Comment 24

[:Margaret Leibovic]

It looks like this crash is still happening on 44:
https://crash-stats.mozilla.com/report/index/01f0d5cf-1011-48c7-966e-001272160127

Either that, or it only solved some of the signatures here. Bug 1232608 has a similar signature, we could use that bug to investigate the remaining problems in BrowserSearch.