1277333 - Crash in java.lang.NullPointerException: at org.mozilla.gecko.GeckoNetworkManager.updateNetworkStateAndConnectionType(GeckoNetworkManager.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-6b588624-a6f8-409d-870a-550ad2160601.
=============================================================
We've fixed similar bugs previously (e.g. bug 1271020) but this build ID is from 5/31 so I don't think we nailed down these issues yet. Here's the trace:

java.lang.NullPointerException
	at org.mozilla.gecko.GeckoNetworkManager.updateNetworkStateAndConnectionType(GeckoNetworkManager.java:274)
	at org.mozilla.gecko.GeckoNetworkManager.handleManagerEvent(GeckoNetworkManager.java:151)
	at org.mozilla.gecko.GeckoNetworkManager.start(GeckoNetworkManager.java:120)
	at org.mozilla.gecko.GeckoApplication.onActivityResume$642b2292(GeckoApplication.java:136)
	at org.mozilla.gecko.preferences.GeckoPreferences.onResume(GeckoPreferences.java:555)
	at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1195)
	at android.app.Activity.performResume(Activity.java:5273)

Grisha, can you take a look?

Comment 1

[:Grisha Kruglov]

Attachment: Bug 1277333 - Keep track of a reference to GeckoApplication to eliminate "no context NPE" crashes

It seems that in certain cases, it's possible for GeckoAppShell.getApplicationContext() to return null.
We've seen it when .start() was called from GeckoPreferences's onResume (see Bug 1277333 for stacktrace).
We have applicationContext available to us in GeckoApplication's onActivityResume, so as a quick fix let's use it
as a back up.

This doesn't address underlying (lifecycle?) issue but should get us to not crash.

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

Comment 2

[:Grisha Kruglov]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7161e808699

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

https://reviewboard.mozilla.org/r/57094/#review54346

Mhhh. This is hacky.

Doesn't this partially revert the change you did in bug 1271318?

How we end up in a situation where we do not have no application context is beyond me; Without it there's no application.

Well, we won't be able to fix this mess now, I guess. but can you add a comment explaining the situation why we juggle with two contexts here?

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:134
(Diff revision 1)
>          final Context applicationContext = getApplicationContext();
>          GeckoBatteryManager.getInstance().start(applicationContext);
> -        GeckoNetworkManager.getInstance().start();
> +        GeckoNetworkManager.getInstance().start(applicationContext);

GeckoBatteryManager uses the applicationContext in start(). So I assume this one is not null, because otherwise we'd see a crash there? Why not always use this one then? After all there's only one application context. But then again we had issues with it in bug 1271020.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Michael Comella (:mcomella) from comment #0)
> We've fixed similar bugs previously (e.g. bug 1271020) but this build ID is
> from 5/31 so I don't think we nailed down these issues yet. Here's the trace:

So far there's still no new report with a build ID higher than 20160531*.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Sebastian Kaspari (:sebastian) from comment #4)
> (In reply to Michael Comella (:mcomella) from comment #0)
> > We've fixed similar bugs previously (e.g. bug 1271020) but this build ID is
> > from 5/31 so I don't think we nailed down these issues yet. Here's the trace:
> 
> So far there's still no new report with a build ID higher than 20160531*.

Now we got new reports :|

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758988 [details]
Bug 1277333 - Keep track of a reference to GeckoApplication to eliminate "no context NPE" crashes

https://reviewboard.mozilla.org/r/57094/#review55030

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:136
(Diff revision 1)
>              mPausedGecko = false;
>          }
>  
>          final Context applicationContext = getApplicationContext();
>          GeckoBatteryManager.getInstance().start(applicationContext);
> -        GeckoNetworkManager.getInstance().start();
> +        GeckoNetworkManager.getInstance().start(applicationContext);

That's what we did before and removed in bug 1271318. It seems like according to bug 1271318 applicationContext can be null here too; which is kind of strange because we use it in GeckoBatteryManager above too.

Comment 7

[:Grisha Kruglov]

(In reply to Sebastian Kaspari (:sebastian) from comment #6)
> Comment on attachment 8758988 [details]
> MozReview Request: Bug 1277333 - Pass in context to start() and use it as a
> "back up" r=sebastian
> 
> https://reviewboard.mozilla.org/r/57094/#review55030
> 
> ::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:136
> (Diff revision 1)
> >              mPausedGecko = false;
> >          }
> >  
> >          final Context applicationContext = getApplicationContext();
> >          GeckoBatteryManager.getInstance().start(applicationContext);
> > -        GeckoNetworkManager.getInstance().start();
> > +        GeckoNetworkManager.getInstance().start(applicationContext);
> 
> That's what we did before and removed in bug 1271318. It seems like
> according to bug 1271318 applicationContext can be null here too; which is
> kind of strange because we use it in GeckoBatteryManager above too.

Yeah, this is all too hacky. I wonder if we had crashes in GeckoBatterymanager's start method - NetworkManager before the rewrite did a lot of null checks when working with passed in context.

I'll give this another lookover.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

This is the top crash in Nightly (https://crash-stats.mozilla.com/report/index/6d4ee146-4bc0-403b-8316-204922160703)

It looks like when we're here in GeckoApplication.onActivityResume() the context getter in GeckoAppShell is null. Grisha do you have time to look at this?

Comment 9

[:Grisha Kruglov]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #8)
> This is the top crash in Nightly
> (https://crash-stats.mozilla.com/report/index/6d4ee146-4bc0-403b-8316-
> 204922160703)
> 
> It looks like when we're here in GeckoApplication.onActivityResume() the
> context getter in GeckoAppShell is null. Grisha do you have time to look at
> this?

Definitely on my radar, will get to this next thing.

Comment 10

[:Grisha Kruglov]

Comment on attachment 8758988 [details]
Bug 1277333 - Keep track of a reference to GeckoApplication to eliminate "no context NPE" crashes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57094/diff/1-2/

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8758988 [details]
Bug 1277333 - Keep track of a reference to GeckoApplication to eliminate "no context NPE" crashes

https://reviewboard.mozilla.org/r/57094/#review63940

Still a bit hacky but let's move on. :)

::: mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java:134
(Diff revision 2)
>          final Context applicationContext = getApplicationContext();
>          GeckoBatteryManager.getInstance().start(applicationContext);
> -        GeckoNetworkManager.getInstance().start();
> +        GeckoNetworkManager.getInstance().start(this);

nit: Let's just use 'this' for both. Technically there's no reason to call getApplicationContext() on *the* application context.

Comment 12

[:Grisha Kruglov]

Comment on attachment 8758988 [details]
Bug 1277333 - Keep track of a reference to GeckoApplication to eliminate "no context NPE" crashes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57094/diff/2-3/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/db3f18dc78a3
Keep track of a reference to GeckoApplication to eliminate "no context NPE" crashes r=sebastian

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/db3f18dc78a3

Comment 15

[:Grisha Kruglov]

Attachment: gecko-network-manager.patch

Let's consider uplifting this to 49, if it's not too late. The crash has been fixed in 50+, but no reason this change can't be uplifted.

[Feature/regressing bug #]: GeckoNetworkManager rewrite which landed in 49 (in support of offline browsing) and friends. See Bug 1236130, Bug 1271318, Bug 1232867 (offline meta).

[User impact if declined]: A somewhat rare startup crash happens, currently #12 crash in 49 beta.

[Describe test coverage new/current, TreeHerder]: Manual testing.

[Risks and why]: Low risk; change has been baking since 50 - no similar crashes reported since the patch landed. Change is contained in its nature to GeckoNetworkManager.

[String/UUID change made/needed]: N/A

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

I think it better to keep this in 50 as we are heading into our final week of 49 before release. We could have taken this earlier in beta though.

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8787690 [details] [diff] [review]
gecko-network-manager.patch

Too late for 49, let's let it ride with 50.