Closed Bug 969637 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

30 Branch
All
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified
fennec 29+ ---

People

(Reporter: aaronmt, Assigned: rnewman)

References

Details

(Keywords: crash, regression, reproducible, Whiteboard: [native-crash])

Attachments

(2 files, 1 obsolete file)

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
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)
Whiteboard: [native-crash]
Blocks: 962644
This only happens when entering Sync config settings though the browser's Settings.
I can repro this, but I need an additional step after (i): hit Home, so Setup appears in the app switcher.
The error here is that GeckoConnectivityReceiver has never been initialized, but GeckoApplication.onActivityResume assumes that it has.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
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?
Attachment #8372643 - Flags: review?(wjohnston)
(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.
Flagging wesj for opinions on scope creep.
Flags: needinfo?(wjohnston)
(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)...
Flags: needinfo?(wjohnston)
tracking-fennec: ? → 29+
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
Duplicate of this bug: 976781
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.
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!
Attachment #8381918 - Flags: review?(wjohnston)
Attachment #8372643 - Attachment is obsolete: true
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.
Attachment #8381918 - Flags: review?(wjohnston) → review+
(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
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);
https://hg.mozilla.org/mozilla-central/rev/d4c7d7ed2196
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
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.
Attachment #8381918 - Flags: approval-mozilla-aurora?
Attachment #8381918 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Attached patch Branch patch.Splinter Review
Duplicate of this bug: 1020412
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.