Closed Bug 1184683 Opened 5 years ago Closed 5 years ago

crash in java.lang.NullPointerException: at org.mozilla.gecko.GeckoAppShell.getContext(GeckoAppShell.java)

Categories

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

42 Branch
All
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox39 --- wontfix
firefox40 --- affected
firefox41 --- affected
firefox42 --- fixed

People

(Reporter: aaronmt, Assigned: sebastian)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-b33635ea-a37b-439d-b7cd-a6ddd2150715.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.GeckoAppShell.getContext(GeckoAppShell.java:2137)
	at org.mozilla.gecko.RestrictedProfiles.isUserRestricted(RestrictedProfiles.java:185)
	at org.mozilla.gecko.preferences.GeckoPreferences.setupPreferences(GeckoPreferences.java:698)
	at org.mozilla.gecko.preferences.GeckoPreferences.setupPreferences(GeckoPreferences.java:646)
	at org.mozilla.gecko.preferences.GeckoPreferences.onWindowFocusChanged(GeckoPreferences.java:501)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.onWindowFocusChanged(PhoneWindow.java:2034)
	at android.view.View.dispatchWindowFocusChanged(View.java:3933)
	at android.view.ViewGroup.dispatchWindowFocusChanged(ViewGroup.java:659)
	at android.view.ViewRoot.handleMessage(ViewRoot.java:1973)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:130)
	at android.app.ActivityThread.main(ActivityThread.java:3695)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:507)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:878)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:636)
	at dalvik.system.NativeStart.main(Native Method)
Ally or Sebastian, can one of you look into this?
Flags: needinfo?(s.kaspari)
Flags: needinfo?(ally)
I'll look into this.
Assignee: nobody → s.kaspari
Flags: needinfo?(s.kaspari)
Flags: needinfo?(ally)
I can't reproduce this. Even though the crash reports contain a comment:

>  Sent app to background to use Google Voice, then long press for recent (s3 - jellybean 4.3)and clicked on nightly. App crashed!? 

This doesn't seem to be a new thing in the code base (First crash reported with Firefox 39) but just looking at the code it seems like GeckoAppShell.getContext() is called before sContextGetter is set.

The attached patch avoids the situation completely as we don't need to rely on GeckoAppShell to get a context. GeckoPreferences is already a context and can be passed to the method.

Eventually, in bug 1184064, I'd like to make isUserRestricted not accessible by other classes. Instead they should ask for specific restrictions instead of implementing their own logic based on isUserRestricted.
Attachment #8635956 - Flags: review?(ally)
(In reply to :Sebastian Kaspari from comment #4)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0977e1622573

Not related to this patch (in this case it makes sense), but I notice every patch you've sent me has a treeherder run.

You don't need a treeherder run for every patch and its probably not the best use of Mozilla money to do so. Use your best judgement.
(In reply to :Sebastian Kaspari from comment #3)
> Created attachment 8635956 [details] [diff] [review]
> 1184683_restrictedprofiles_crash.patch
> 
> I can't reproduce this. Even though the crash reports contain a comment:
> 
> >  Sent app to background to use Google Voice, then long press for recent (s3 - jellybean 4.3)and clicked on nightly. App crashed!? 
> 
> This doesn't seem to be a new thing in the code base (First crash reported
> with Firefox 39) but just looking at the code it seems like
> GeckoAppShell.getContext() is called before sContextGetter is set.
> 
> The attached patch avoids the situation completely as we don't need to rely
> on GeckoAppShell to get a context. GeckoPreferences is already a context and
> can be passed to the method.
> 
> Eventually, in bug 1184064, I'd like to make isUserRestricted not accessible
> by other classes. Instead they should ask for specific restrictions instead
> of implementing their own logic based on isUserRestricted.

+1, though I don't think I'd hold v1 for it. I'm crusty & grumpy like that. :)
Comment on attachment 8635956 [details] [diff] [review]
1184683_restrictedprofiles_crash.patch

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

r+ provided you explain what why the first line is needed.

::: mobile/android/base/RestrictedProfiles.java
@@ +180,5 @@
>      public static boolean isUserRestricted() {
>          return isUserRestricted(GeckoAppShell.getContext());
>      }
>  
> +    public static boolean isUserRestricted(final Context context) {

Your knowledge of java far exceeds mine. Please educate me why this switch from private to public helps.
Attachment #8635956 - Flags: review?(ally) → review+
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #7)
> r+ provided you explain what why the first line is needed.

Previously GeckoPreferences called into isUserRestricted() which then called GeckoAppShell to get a Context object from somewhere - just to call isUserRestricted(context) after that. Because GeckoPreferences is already a Context (The application object as well as every Activity implements Context) I wanted to avoid all this black magic around GeckoAppShell and call isUserRestricted(Context) directly from GeckoPreferences. But to make this happen I had to make the method visible for GeckoPreferences (private -> public).

These duplicated methods (isUserRestricted, getUserRestrictions) without Context argument seem to only exist because of native C++ code calling them via JNI and this part doesn't know about Contexts. So they only fetch a Context object from GeckoAppShell and then call their counterpart with this Context object. But here Java code called one of these methods that are made for JNI calls and I wanted to break this circle.
url:        https://hg.mozilla.org/integration/fx-team/rev/f1382c7f4e8276f4f991797036a28ff40a3f42e0
changeset:  f1382c7f4e8276f4f991797036a28ff40a3f42e0
user:       Sebastian Kaspari <s.kaspari@gmail.com>
date:       Mon Jul 20 11:55:32 2015 +0200
description:
Bug 1184683 - GeckoPreferences: Pass current context to RestrictedProfiles instead of relying on GeckoAppShell. r=ally
https://hg.mozilla.org/mozilla-central/rev/f1382c7f4e82
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
ni for uplift consideration after bake.
Flags: needinfo?(s.kaspari)
OS: Unspecified → Android
Hardware: Unspecified → All
Comment on attachment 8635956 [details] [diff] [review]
1184683_restrictedprofiles_crash.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1184683 / Crash
[User impact if declined]: Possible crash in settings screen. However the actual steps to reproduce are unknown.
[Describe test coverage new/current, TreeHerder]: Tested locally, as well as on try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0977e1622573 
[Risks and why]: Pretty low risk. One line change, keeps current code flow but skips unnecessary / crashing step in between.
[String/UUID change made/needed]: -
Flags: needinfo?(s.kaspari)
Attachment #8635956 - Flags: approval-mozilla-beta?
Attachment #8635956 - Flags: approval-mozilla-aurora?
Restoring NI to keep track of this bug.
Flags: needinfo?(s.kaspari)
Comment on attachment 8635956 [details] [diff] [review]
1184683_restrictedprofiles_crash.patch

(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> [Feature/regressing bug #]: Bug 1184683 / Crash

Do you know in what bug or what version of Firefox this issue was introduced?

> [User impact if declined]: Possible crash in settings screen. However the
> actual steps to reproduce are unknown.

From the crash report linked in the description, the incidence of this crash is very low (4 total in 28 days with 3 of those being on 42). There doesn't seem to be a need to uplift this fix. I'm denying uplift to Beta and Aurora. Please renom if you think I've missed something and this crash is more critical than it appears.
Attachment #8635956 - Flags: approval-mozilla-beta?
Attachment #8635956 - Flags: approval-mozilla-beta-
Attachment #8635956 - Flags: approval-mozilla-aurora?
Attachment #8635956 - Flags: approval-mozilla-aurora-
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #14)
> Do you know in what bug or what version of Firefox this issue was introduced?

Unfortunately I don't. This has been fixed mostly because of the other work that has been done around the affected RestrictedProfiles class lately.

> From the crash report linked in the description, the incidence of this crash
> is very low (4 total in 28 days with 3 of those being on 42). There doesn't
> seem to be a need to uplift this fix. I'm denying uplift to Beta and Aurora.
> Please renom if you think I've missed something and this crash is more
> critical than it appears.

WFM. Uplift was requested because the risk was pretty low.
Flags: needinfo?(s.kaspari)
You need to log in before you can comment on or make changes to this bug.