Closed Bug 1020412 Opened 6 years ago Closed 6 years ago

Crash (java.lang.NullPointerException) in GeckoPreferences on swiping away the Sync activity

Categories

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

32 Branch
ARM
Android
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Firefox 33
Tracking Status
firefox30 --- wontfix
firefox31 + verified
firefox32 + verified
firefox33 --- verified
fennec 32+ ---

People

(Reporter: aaronmt, Assigned: rnewman)

Details

(Keywords: crash, reproducible)

Attachments

(1 file, 1 obsolete file)

E/AndroidRuntime(16994): java.lang.RuntimeException: Unable to resume activity {org.mozilla.fennec/org.mozilla.gecko.preferences.GeckoPreferences}: java.lang.NullPointerException
E/AndroidRuntime(16994): 	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2774)
E/AndroidRuntime(16994): 	at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:2803)
E/AndroidRuntime(16994): 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2238)
E/AndroidRuntime(16994): 	at android.app.ActivityThread.access$800(ActivityThread.java:135)
E/AndroidRuntime(16994): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1196)
E/AndroidRuntime(16994): 	at android.os.Handler.dispatchMessage(Handler.java:102)
E/AndroidRuntime(16994): 	at android.os.Looper.loop(Looper.java:136)
E/AndroidRuntime(16994): 	at android.app.ActivityThread.main(ActivityThread.java:5001)
E/AndroidRuntime(16994): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/AndroidRuntime(16994): 	at java.lang.reflect.Method.invoke(Method.java:515)
E/AndroidRuntime(16994): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:785)
E/AndroidRuntime(16994): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:601)
E/AndroidRuntime(16994): 	at dalvik.system.NativeStart.main(Native Method)
E/AndroidRuntime(16994): Caused by: java.lang.NullPointerException
E/AndroidRuntime(16994): 	at org.mozilla.gecko.GeckoAppShell.getContext(GeckoAppShell.java:2118)
E/AndroidRuntime(16994): 	at org.mozilla.gecko.GeckoNetworkManager.getApplicationContext(GeckoNetworkManager.java:63)
E/AndroidRuntime(16994): 	at org.mozilla.gecko.GeckoNetworkManager.getConnectionType(GeckoNetworkManager.java:186)
E/AndroidRuntime(16994): 	at org.mozilla.gecko.GeckoNetworkManager.start$faab20d(GeckoNetworkManager.java:90)
E/AndroidRuntime(16994): 	at org.mozilla.gecko.GeckoApplication.onActivityResume$642b2292(GeckoApplication.java:113)
E/AndroidRuntime(16994): 	at org.mozilla.gecko.preferences.GeckoPreferences.onResume(GeckoPreferences.java:447)
E/AndroidRuntime(16994): 	at android.app.Instrumentation.callActivityOnResume(Instrumentation.java:1192)
E/AndroidRuntime(16994): 	at android.app.Activity.performResume(Activity.java:5310)
E/AndroidRuntime(16994): 	at android.app.ActivityThread.performResumeActivity(ActivityThread.java:2764)

Steps to Reproduce

 i) Settings -> Sync
 ii) Hit Back
 iii) Tap your device application switcher button
 iv) Swipe off Firefox Sync

Reported by sawrubh in #mobile, reproducible on my Nexus 5 (Android 4.4.3)

--
Nightly (06/04)
Severity: normal → critical
tracking-fennec: --- → ?
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 969637
Can't be, this is trunk.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee: nobody → rnewman
Status: REOPENED → ASSIGNED
This continues the approach in Bug 969637 -- being more deliberate about using
a provided context rather than always asking GeckoAppShell. There's one point
in particular that caused the bug: the fix in Bug 969637 didn't dive into
getConnectionType, so we passed in a context and then promptly ignored it in
favor of GAS. This patch finishes that work.

I also cleaned up a few typos and such.

With this patch I don't get a crash when swiping away Sync setup as described
in Comment 0.

I do still see Bug 979899, but that's a separate issue.
Attachment #8435091 - Flags: review?(wjohnston)
tracking-fennec: ? → 32+
Comment on attachment 8435091 [details] [diff] [review]
Proper handling of Context in GeckoNetworkManager. v1

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

A couple questions before I r+.

::: mobile/android/base/GeckoNetworkManager.java
@@ +95,4 @@
>          }
>      }
>  
> +    private void startListening(Context appContext) {

All the callers use mApplicationContext. Do we need to pass anything in here?

@@ +99,5 @@
> +        if (appContext == null) {
> +            Log.w(LOGTAG, "Not registering receiver: no context!");
> +            return;
> +        } else {
> +            Log.i(LOGTAG, "Registering receiver.");

Probably a little more logging than we need.

@@ +186,5 @@
>              stopListening();
>          }
>      }
>  
> +    private static ConnectionType getConnectionType(Context appContext) {

Same here, except this is static so we have to pass something in. Why is this private static?
Attachment #8435091 - Flags: review?(wjohnston) → review-
(In reply to Wesley Johnston (:wesj) from comment #4)

> All the callers use mApplicationContext. Do we need to pass anything in here?

Personal preference: I tend to aim for pure functions (only operating on their arguments), because it makes things easier to reason about. It's not a big deal in this case, so if you prefer the implicit stateful OO approach, it can switch.


> > +            Log.i(LOGTAG, "Registering receiver.");
> 
> Probably a little more logging than we need.

I left a review note that this is .d in the current patch. Is .d too much?
 

> Same here, except this is static so we have to pass something in. Why is
> this private static?

Because it already was. That can change if you prefer.
Is this what you were looking for, Wes?
Attachment #8438694 - Flags: review?(wjohnston)
Attachment #8435091 - Attachment is obsolete: true
Attachment #8438694 - Flags: review?(wjohnston) → review+
Needs landing (fx-team closed) and uplift request.
Flags: needinfo?(rnewman)
Comment on attachment 8438694 [details] [diff] [review]
Proper handling of Context in GeckoNetworkManager. v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Long-standing bad assumptions in activities, interacting with new Sync setup in 29. Prior work didn't solve one avenue, so this crash remains.

User impact if declined: 
  Crashes if a user swipes away Sync setup in task switcher, then resumes Firefox.

Testing completed (on m-c, etc.): 
  Tested locally. Just landed on m-c.

Risk to taking this patch (and alternatives if risky): 
  Relatively low; applies a common pattern that we used elsewhere for similar fixes. Small set of material changes.

String or IDL/UUID changes made by this patch:
  None.
Attachment #8438694 - Flags: approval-mozilla-aurora?
Flags: needinfo?(rnewman)
https://hg.mozilla.org/mozilla-central/rev/eef1522f8eb1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
(In reply to Richard Newman [:rnewman] from comment #9)
> Bug caused by (feature/regressing bug #): 
>   Long-standing bad assumptions in activities, interacting with new Sync
> setup in 29. Prior work didn't solve one avenue, so this crash remains.

Given your description I take it this bug impacts 31 (beta) as well. Should we discuss a beta uplift as well?

> Testing completed (on m-c, etc.): 
>   Tested locally. Just landed on m-c.

I would like to give this fix a couple of days on m-c before approving the uplift request.
(In reply to Lawrence Mandel [:lmandel] from comment #11)

> Given your description I take it this bug impacts 31 (beta) as well. Should
> we discuss a beta uplift as well?

If you're happy with the risk profile at this stage of beta, sure. The patch applies cleanly, but I haven't tested it there yet.


> I would like to give this fix a couple of days on m-c before approving the
> uplift request.

Totally expected!
Comment on attachment 8438694 [details] [diff] [review]
Proper handling of Context in GeckoNetworkManager. v2

nom for beta so that we give this proper consideration. Approval request comment 9 has the details.
Attachment #8438694 - Flags: approval-mozilla-beta?
Comment on attachment 8438694 [details] [diff] [review]
Proper handling of Context in GeckoNetworkManager. v2

Taking it now to have it in beta4.
Attachment #8438694 - Flags: approval-mozilla-beta?
Attachment #8438694 - Flags: approval-mozilla-beta+
Attachment #8438694 - Flags: approval-mozilla-aurora?
Attachment #8438694 - Flags: approval-mozilla-aurora+
Verified as fixed in builds:
33.0a1 (2014-06-25)
32.0a2 (2014-06-25)
31.0b4

Device: Nexus 5 (Android 4.4.3) and Asus Transformer Pad TF300T (Android 4.2.1)

Using the steps from comment 0 the only issue I see is Bug 979899, so I'll mark this as verified fixed.
Duplicate of this bug: 1025931
You need to log in before you can comment on or make changes to this bug.