969637 - Regression: Crash java.lang.NullPointerException in org.mozilla.gecko.GeckoConnectivityReceiver.start(GeckoConnectivityReceiver.java:49)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

E/AndroidRuntime(27154): FATAL EXCEPTION: main
E/AndroidRuntime(27154): Process: org.mozilla.fennec, PID: 27154
E/AndroidRuntime(27154): java.lang.RuntimeException: Unable to resume activity {org.mozilla.fennec/org.mozilla.gecko.preferences.GeckoPreferences}: java.lang.NullPointerException
E/AndroidRuntime(27154): 	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2788)
E/AndroidRuntime(27154): 	at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:2817)
E/AndroidRuntime(27154): 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2250)
E/AndroidRuntime(27154): 	at android.app.ActivityThread.access$800(ActivityThread.java:135)
E/AndroidRuntime(27154): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1196)
E/AndroidRuntime(27154): 	at android.os.Handler.dispatchMessage(Handler.java:102)
E/AndroidRuntime(27154): 	at android.os.Looper.loop(Looper.java:136)
E/AndroidRuntime(27154): 	at android.app.ActivityThread.main(ActivityThread.java:5017)
E/AndroidRuntime(27154): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime(27154): 	at java.lang.reflect.Method.invoke(Method.java:515)
E/AndroidRuntime(27154): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:779)
E/AndroidRuntime(27154): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:595)
E/AndroidRuntime(27154): 	at dalvik.system.NativeStart.main(Native Method)
E/AndroidRuntime(27154): Caused by: java.lang.NullPointerException
E/AndroidRuntime(27154): 	at org.mozilla.gecko.GeckoConnectivityReceiver.start(GeckoConnectivityReceiver.java:49)
E/AndroidRuntime(27154): 	at org.mozilla.gecko.GeckoApplication.onActivityResume$642b2292(GeckoApplication.java:107)
E/AndroidRuntime(27154): 	at org.mozilla.gecko.preferences.GeckoPreferences.onResume(GeckoPreferences.java:242)
E/AndroidRuntime(27154): 	at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1192)
E/AndroidRuntime(27154): 	at android.app.Activity.performResume(Activity.java:5310)
E/AndroidRuntime(27154): 	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2778)
E/AndroidRuntime(27154): 	... 12 more
W/ActivityManager(  593):   Force finishing activity org.mozilla.fennec/org.mozilla.gecko.preferences.GeckoPreferences

Steps to reproduce

i) Open Firefox Account Sync settings → (Settings → Sync)
ii) Swipe it off the recent application listing
iii) Swap back to the browser

Reproducible on

Comment 1

[Aaron Train [:aaronmt]]

On swipe off of the FxA Settings screen in Android

I/WindowState(  593): WIN DEATH: Window{42f7b9a0 u0 org.mozilla.fennec/org.mozilla.fennec.App}
W/WindowManager(  593): Force-removing child win Window{427a0ca0 u0 SurfaceView} from container Window{42f7b9a0 u0 org.mozilla.fennec/org.mozilla.fennec.App}
W/WindowManager(  593): Failed looking up window
W/WindowManager(  593): java.lang.IllegalArgumentException: Requested window android.os.BinderProxy@427a0a40 does not exist
W/WindowManager(  593): 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:7923)
W/WindowManager(  593): 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:7914)
W/WindowManager(  593): 	at com.android.server.wm.WindowState$DeathRecipient.binderDied(WindowState.java:1047)
W/WindowManager(  593): 	at android.os.BinderProxy.sendDeathNotice(Binder.java:493)
W/WindowManager(  593): 	at dalvik.system.NativeStart.run(Native Method)
I/WindowState(  593): WIN DEATH: null
I/WindowState(  593): WIN DEATH: Window{428fd450 u0 org.mozilla.fennec/org.mozilla.gecko.preferences.GeckoPreferences}
I/WindowState(  593): WIN DEATH: Window{429b6e20 u0 org.mozilla.fennec/org.mozilla.gecko.fxa.activities.FxAccountStatusActivity}
I/ActivityManager(  593): Start proc org.mozilla.fennec for service org.mozilla.fennec/org.mozilla.gecko.background.healthreport.prune.HealthReportPruneService: pid=27494 uid=10190 gids={50190, 3003, 1028, 1015}

100% reproducible on my LG Nexus 4 (Android 4.4.2)

Comment 2

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e0a22097bff&tochange=8840c133115a

bug 962644?

Comment 3

[Aaron Train [:aaronmt]]

This only happens when entering Sync config settings though the browser's Settings.

Comment 4

[Richard Newman [:rnewman]]

I can repro this, but I need an additional step after (i): hit Home, so Setup appears in the app switcher.

Comment 5

[Richard Newman [:rnewman]]

The error here is that GeckoConnectivityReceiver has never been initialized, but GeckoApplication.onActivityResume assumes that it has.

Comment 6

[Richard Newman [:rnewman]]

Attachment: NullPointerException in org.mozilla.gecko.GeckoConnectivityReceiver.start.

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 8372643 [details] [diff] [review]
NullPointerException in org.mozilla.gecko.GeckoConnectivityReceiver.start.

Review of attachment 8372643 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense now, but a question. Can we avoid doing this initialization in intialize() at all and instead just "initialize" in show() (or getInstance?)

::: mobile/android/base/GeckoConnectivityReceiver.java
@@ +39,5 @@
>          mFilter.addAction(ConnectivityManager.CONNECTIVITY_ACTION);
>      }
>  
> +    public void init(final Context applicationContext) {
> +        mApplicationContext = applicationContext;

This relies on callers being smart. I don't think we want to do that.

@@ +45,5 @@
>  
>      public synchronized void start() {
> +        if (mApplicationContext == null) {
> +            Log.e(LOGTAG, "Never successfully inited. Throwing.");
> +            throw new IllegalStateException("No app context. Cannot start.");

So this will take down the whole app? I don't think we want that. Can we just pass the context in here? In fact, do we even need to initialize this class in initialize() or are we better to just start/stop it in pause/resume?

Comment 8

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #7)

> Makes sense now, but a question. Can we avoid doing this initialization in
> intialize() at all and instead just "initialize" in show() (or getInstance?)

I don't know what `show` method you mean; could you clarify?

(But see discussion around your last question.)


> > +    public void init(final Context applicationContext) {
> > +        mApplicationContext = applicationContext;
> 
> This relies on callers being smart. I don't think we want to do that.

All callers were passing in the application context anyway, which is why I removed the redundant .gAC() call. (In other words, this patch doesn't change the meaning.)

I think the callers (in this case, GeckoApplication) should be the one deciding on the context, rather than GCR.

Ideally we'd be binding these listeners/managers to an Activity, not to the global Application, in which case these would move out of GeckoApplication and into GeckoApp or BrowserApp, and this would be a regular Context. And most of this bug would go away, replaced by a slimming-down of GeckoApplication.

Thoughts?


> So this will take down the whole app? I don't think we want that.

That's what happens now, just with a NPE. This just adds a more descriptive death :P

And this should never occur after this patch is added. That's the failure path if you try to start that component without first having initialized it.


> just pass the context in here? In fact, do we even need to initialize this
> class in initialize() or are we better to just start/stop it in pause/resume?

The current division of labor is:

MemoryMonitor:              init in GeckoApplication.initialize, no start needed?
GeckoBatteryManager:        init and started in GeckoApplication.initialize
GeckoConnectivityReceiver:  init in GeckoApplication.initialize, start in GeckoApplication.onActivityResume
GeckoNetworkManager:        init in GeckoApplication.initialize, start in GeckoApplication.onActivityResume

GCR is never touched by any other class, so early init (prior to .start()) is unnecessary.
GNM can be called from JNI, so theoretically it could be touched prior to onActivityResume, and should thus be live earlier. I don't know if that's a practical concern.
GBM is exactly the same, but *is* started earlier. Those two should, IMO, follow the same pattern.

A pattern I started in a big rework for Bug 951959 involved passing contexts into calls, rather than statically hanging on to a context instance. That's messy in other ways, alas, because not every caller has the context.

Comment 9

[Richard Newman [:rnewman]]

Flagging wesj for opinions on scope creep.

Comment 10

[Wesley Johnston (:wesj)]

(In reply to Richard Newman [:rnewman] from comment #8)

> Ideally we'd be binding these listeners/managers to an Activity, not to the
> global Application, in which case these would move out of GeckoApplication
> and into GeckoApp or BrowserApp, and this would be a regular Context. And
> most of this bug would go away, replaced by a slimming-down of
> GeckoApplication.

These are in GeckoApplication because their lifetimes are tied to Gecko, not the Activity. i.e. The Activity should be able to die and these should live on (and not leak the Activity context at the same time). I don't think we can move them without breaking things.

I can see your point about them taking a generic context though, and letting the caller determine which that is. Then again, these are singletons, so in a way THEY own their own lifetime. Having them call getApplicationContext() is just them being safe and making sure their lifetimes are correct despite what the caller passes in.

> And this should never occur after this patch is added. That's the failure
> path if you try to start that component without first having initialized it.

Yeah, I get that. My thought was just that removing this whole initialize thing and calling

GCR.start(applicationContext);

in onActivityResume we can hopefully mitigate these crashes, and reduce a little complexity.

> A pattern I started in a big rework for Bug 951959 involved passing contexts
> into calls, rather than statically hanging on to a context instance. That's
> messy in other ways, alas, because not every caller has the context.

Yeah, I'd prefer to avoid that scope creep here if we can. TBH, I think the first thing I'd do is rename "initialize()" since it implies that we're initializing the Application (even though it doesn't necessarily run every time the Application is started)...

Comment 11

[Flaviu Cos]

Last good revision: 8840c133115a (2014-02-01)
First bad revision: 3e40f7389d1b (2014-02-02)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8840c133115a&tochange=3e40f7389d1b

Comment 13

[Richard Newman [:rnewman]]

As I loop back around on this, observations:

> These are in GeckoApplication because their lifetimes are tied to Gecko, not
> the Activity. i.e. The Activity should be able to die and these should live
> on (and not leak the Activity context at the same time). I don't think we
> can move them without breaking things.

This is not the bug in which to address this, but I don't regard the Application as the correct thing to which to tie Gecko. Gecko is indeed a persistent subcomponent with a lifespan longer than some activities, but it's not mapped 1:1 to the application. Gecko is kinda like an HTML-rendering Service that we want to be running when necessary, and to stick around once it's started.

Unlike in 2011, we have this structure (both for Fennec and for apps that use GeckoView):

  App
    Gecko-related persistent parts
      Gecko-related activities
    Non-Gecko persistent parts
      Non-Gecko-related activities

GeckoApplication is currently our Application, and we simply rely on it not necessarily being inited unless we're launching BrowserApp. E.g., if you launch FxAccountGetStartedActivity -- one of our non-browser activities -- Android will instantiate GeckoApplication, which will run all this:

    @Override
    public void onCreate() {
        HardwareUtils.init(getApplicationContext());
        Clipboard.init(getApplicationContext());
        FilePicker.init(getApplicationContext());
        GeckoLoader.loadMozGlue();
        HomeConfigInvalidator.getInstance().init(getApplicationContext());
        super.onCreate();
    }

but because FxAGSA doesn't call GeckoApplication.initialize (only GeckoApp does that) or GeckoApplication.onActivityResume (only GeckoActivity and GeckoPreferences do that), Gecko itself is never inited.

We might have to address this at some point, perhaps as GeckoView matures. That might be as simple as "GeckoApplication should have no reference to GeckoAppShell, and GAS becomes the long-term owner of the Gecko parts".

Do you have any thoughts on this?


(This bug, incidentally, is that GeckoPreferences isn't GeckoApp, so it never calls GeckoApplication.initialize, but some of onActivityResume expects that initialize has been called.)


> I can see your point about them taking a generic context though, and letting
> the caller determine which that is. Then again, these are singletons, so in
> a way THEY own their own lifetime. Having them call getApplicationContext()
> is just them being safe and making sure their lifetimes are correct despite
> what the caller passes in.

I think I agree in the context of Fennec. I don't know yet if the same applies for GeckoView, which I could imagine might reuse some of these constructs.

Comment 14

[Richard Newman [:rnewman]]

Attachment: NullPointerException in org.mozilla.gecko.GeckoConnectivityReceiver.start. v2

I think this is the direction you wanted. Note that I carried forward the "always call getApplicationContext" logic to FilePicker and Clipboard.

Lots of good cleanup!

Comment 15

[Wesley Johnston (:wesj)]

Comment on attachment 8381918 [details] [diff] [review]
NullPointerException in org.mozilla.gecko.GeckoConnectivityReceiver.start. v2

Review of attachment 8381918 [details] [diff] [review]:
-----------------------------------------------------------------

Seems good.

::: mobile/android/base/GeckoApp.java
@@ +1219,5 @@
> +            Class.forName("android.os.AsyncTask");
> +        } catch (ClassNotFoundException e) {}
> +
> +        MemoryMonitor.getInstance().init(getApplicationContext());
> +        ((GeckoApplication) getApplication()).prepareLightweightTheme();

We could probably move this guy into BrowserApp, since Webapps in general don't support themes.

::: mobile/android/base/GeckoBatteryManager.java
@@ +61,5 @@
>      }
>  
>      public synchronized void stop() {
> +        if (!mIsEnabled) {
> +            Log.w(LOGTAG, "Already stopped!");

I'm a little worried about logspew every time we enter/exit the background from these.

Comment 16

[Richard Newman [:rnewman]]

(In reply to Wesley Johnston (:wesj) from comment #15)

> We could probably move this guy into BrowserApp, since Webapps in general
> don't support themes.

Will do.

> >      public synchronized void stop() {
> > +        if (!mIsEnabled) {
> > +            Log.w(LOGTAG, "Already stopped!");
> 
> I'm a little worried about logspew every time we enter/exit the background
> from these.

The theory is that they should only ever be paired -- start and stop are called from onActivityResume/onActivityPause. If they're not, it's a bug, so we should fix that...

... so if you see logspew, let's file and fix! :D

Comment 17

[Richard Newman [:rnewman]]

Moved to BrowserApp, but N.B.,

        // This has to be prepared prior to calling GeckoApp.onCreate, because
        // widget code and BrowserToolbar need it, and they're created by the
        // layout, which GeckoApp takes care of.
        ((GeckoApplication) getApplication()).prepareLightweightTheme();
        super.onCreate(savedInstanceState);

Comment 18

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/d4c7d7ed2196

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 20

[Richard Newman [:rnewman]]

Comment on attachment 8381918 [details] [diff] [review]
NullPointerException in org.mozilla.gecko.GeckoConnectivityReceiver.start. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Long-standing, but made visible by Sync entry point in Fx29.

User impact if declined: 
  Possible to get crashes if Sync setup is interrupted.

Testing completed (on m-c, etc.): 
  Baked.

Risk to taking this patch (and alternatives if risky): 
  Relatively straightforward change; moving code around. No alternatives if we're going to ship Sync.

String or IDL/UUID changes made by this patch:
  None.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

This has conflicts in FilePicker.java due to bug 970506 not existing there. I can see where this code was in ActivityHandlerHelper.java, but I'm not sure if there's an equivalent fix that needs to be done there or if it can just be ignored.

Comment 22

[Richard Newman [:rnewman]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #21)
> This has conflicts in FilePicker.java due to bug 970506 not existing there.
> I can see where this code was in ActivityHandlerHelper.java, but I'm not
> sure if there's an equivalent fix that needs to be done there or if it can
> just be ignored.

It actually looks like ActivityHandlerHelper's context usage is perfect, so nothing needs to be done there. Will upload branch patch momentarily.

Comment 23

[Richard Newman [:rnewman]]

Attachment: Branch patch.

Comment 24

[Richard Newman [:rnewman]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7072e385c5de

Comment 26

[Cristina Madaras, QA [:CristinaM]]

Verified on Firefox for Android 29.0 and Firefox for Android 30.0 using the steps from comment 0. After the step iii) the Settings page is displayed but the options are grayed out. Considering that the crash is not reproducible I will mark this as verified fixed.