Relevant logcat output is attached. The crash in question is on https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoNetworkManager.java?rev=d4c7d7ed2196#221

`GeckoNetworkManager.start` inits mApplicationContext` that is null when `GeckoNetworkManager.enableNotifications` is called. `GNM.start` is called in Fennec by `GeckoActivity.onActivityResume` (as GeckoApp extends GeckoActivity). GeckoView does not call `GNM.start` because it does not extend GeckoApp, and thus never inits GNM.

Brad: xpcom components exceed my knowledge so I can't debug this further. Needinfo'ing you so you can keep this moving if you think it's important. Some more debug info, attempting to trace where GNM.enableNetworkNotifications was called from in GeckoView: GNM.enableNetworkNotifications GeckoAppShell.enableNetworkNotifications [1] GeckoAppShell::EnableNetworkNotifications [2] Tickler::Init [3] nsHttpHandler::Init [4] nsNetModule.cpp [5] as an xpcom component (according to someone in #mobile) [1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoAppShell.java?rev=683870122209#2465 [2]: https://mxr.mozilla.org/mozilla-central/source/widget/android/GeneratedJNIWrappers.cpp?rev=e5ae503d3dba#427 [3]: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/src/Tickler.cpp?rev=a136d86479c5#84 [4]: https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHandler.cpp?rev=a4c51a14aaf2#356 [5]: https://mxr.mozilla.org/mozilla-central/source/netwerk/build/nsNetModule.cpp?rev=495387c38734#238

Your problem is not XPCOM, its that this isn't being initialized correctly. changeset d4c7d7ed2196 is probably relevant. http://hg.mozilla.org/mozilla-central/rev/d4c7d7ed2196

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3) > Your problem is not XPCOM, its that this isn't being initialized correctly. [1] appears to have removed the initialization from GeckoView purposely. Do we want to initialize GeckoNetworkManager in GeckoView or fix the associated code paths so that GeckoView can run without GeckoNetworkManager being initialized? [1]: https://hg.mozilla.org/mozilla-central/diff/d4c7d7ed2196/mobile/android/base/GeckoView.java#l1.14

Initing GNM was not the same as starting it. We eliminated init in that changeset, because all consumers should be starting, and thus init was pointless. The old GV code was wrong. Call GeckoNetworkManager.getInstance().start(Context) if you want to use GNM, otherwise don't touch it. One of these things should happen, I suspect: * GV (and all other consumers of Gecko) should be mandated to always init GNM, and the appropriate start call can be inserted where that comment currently lives. * GAS should ensure that GNM is started, because apparently GAS depends on it, and we can remove the start calls from some other part of Fennec. * GAS should be more mindful of what it's calling. * Gecko consumers (e.g., GV) should be in control of which random Gecko services (like Tickler) run, and when, and hooking them up to whichever Java-side functionality they need.

(In reply to Michael Comella (:mcomella) from comment #4) > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #3) > > Your problem is not XPCOM, its that this isn't being initialized correctly. > > [1] appears to have removed the initialization from GeckoView purposely. Do > we want to initialize GeckoNetworkManager in GeckoView or fix the associated > code paths so that GeckoView can run without GeckoNetworkManager being > initialized? > > [1]: > https://hg.mozilla.org/mozilla-central/diff/d4c7d7ed2196/mobile/android/base/ > GeckoView.java#l1.14 The former, it should be being initialized. GNM is accessed from core gecko so it needs to be ready for those incoming calls. The perspective is a bit off here though, GeckoActivity uses GeckoView. Initialization should be pushed down from GeckoActiity to GeckoView.

(In reply to Michael Comella (:mcomella) from comment #1) > `GNM.start` is called in Fennec by `GeckoActivity.onActivityResume` (as > GeckoApp extends GeckoActivity). I messed up. `GNM.start` is called in `GeckoApplication.onActivityResume` which is called by `GeckoActivity.onResume`. This can occur because GeckoApplication is the application that owns GeckoActivity. > GeckoView does not call `GNM.start` because it does not extend GeckoApp, and > thus never inits GNM. `GeckoViewExample` extends `Activity` (not `GeckoApp`) and does not specify `GeckoApplication` as the owning application [1], and thus does not make these calls. [1]: https://mxr.mozilla.org/mozilla-central/source/embedding/android/geckoview_example/AndroidManifest.xml#11

don't get GeckoView and GeckoViewExample confused. GeckoView is initialized with a context, from there it should init GNM

Comment on attachment 8394441 [details] [diff] [review] Initialize GeckoNetworkManager in GeckoView. Is this what you're looking for? I'll r? rnewman if you f+. Just to point out that rnewman mentioned in comment 5: > We eliminated init in that changeset, because all consumers should be starting, and thus init was pointless. But I take it this is not the case, considering Gecko uses it and expects it to be initialized, whether or not the application embedding GV wants to use it (and if they do want to use it, they'll call `GNM.start`).

Comment on attachment 8394441 [details] [diff] [review] Initialize GeckoNetworkManager in GeckoView. Review of attachment 8394441 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoNetworkManager.java @@ -125,4 @@ > public void start(final Context context) { > - // Note that this initialization clause only runs once. > - if (mApplicationContext == null) { > - mApplicationContext = context.getApplicationContext(); get ride of mApplicationContext and replace it with GeckoAppShell().getContext().getApplicationContext() or a helper function to the same effect.

Comment on attachment 8395910 [details] [diff] [review] Part 1: Remove mApplicationContext from GeckoNetworkManager. Review of attachment 8395910 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoNetworkManager.java @@ +122,5 @@ > public void onReceive(Context aContext, Intent aIntent) { > updateNetworkType(); > } > > public void start(final Context context) { you're not using the context anymore, so no need to pass it in @@ +124,5 @@ > } > > public void start(final Context context) { > // Note that this initialization clause only runs once. > + if (!isInitialized) { rather than isInitialized, check for mNetworkType == NetworkType.NETWORK_NONE @@ +140,5 @@ > } > > private void startListening() { > + final Context context = GeckoAppShell.getContext().getApplicationContext(); > + context.registerReceiver(sInstance, mNetworkFilter); probably best to use a helper function and null check Context getApplicationContext() { Context context = GeckoAppShell.getContext(); if (null == context) return null; return context.getApplicationContext(); } and for startListening: private void startListening() { Context context = getApplicationContext(); if (null != context) context.registerReciever(sInstance, mNetworkFilter); }

Comment on attachment 8395911 [details] [diff] [review] Part 2: Initialize GeckoNetworkManager in GeckoView. Review of attachment 8395911 [details] [diff] [review]: ----------------------------------------------------------------- not needed

Comment on attachment 8396850 [details] [diff] [review] NPE_GNM.patch Review of attachment 8396850 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoNetworkManager.java @@ +102,4 @@ > MNC > } > > + private static Context getApplicationContext() { Maybe just add this to GeckoAppShell, or GeckoInterface? @@ +142,4 @@ > } > > private void startListening() { > + if (null !=getApplicationContext()) nit: add a space

Comment on attachment 8396850 [details] [diff] [review] NPE_GNM.patch Review of attachment 8396850 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoNetworkManager.java @@ +102,4 @@ > MNC > } > > + private static Context getApplicationContext() { If you do, might be worth changing the name. "getApplicationContext" is widely available, and doesn't mean "return null if nobody has set up GeckoAppShell yet". Perhaps consider having GeckoAppShell.isInitialized(), which will expose the gnarly coupling here: rather than using GNM.getApplicationContext() and a null check, you'd have couplets like: if (!GeckoAppShell.isInitialized()) { return; } GeckoAppShell.getContext().getApplicationContext().registerReceiver(...); I personally find that approach to be much clearer: it indicates: * We're dependent on GAS being initialized * Initialization implies a context being set * We want the application context, which says something about our lifecycle. @@ +142,4 @@ > } > > private void startListening() { > + if (null !=getApplicationContext()) And braces throughout.