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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

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)

Comment 1

[:Margaret Leibovic]

Ally or Sebastian, can one of you look into this?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll look into this.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: 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.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0977e1622573

Comment 5

[Allison Naaktgeboren :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.

Comment 6

[Allison Naaktgeboren :ally]

(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 7

[Allison Naaktgeboren :ally]

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.

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

url:        https://hg.mozilla.org/integration/fx-team/rev/f1382c7f4e8276f4f991797036a28ff40a3f42e0
changeset:  f1382c7f4e8276f4f991797036a28ff40a3f42e0
user:       Sebastian Kaspari <[e-mail]>
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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Richard Newman [:rnewman]]

ni for uplift consideration after bake.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

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]: -

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

Restoring NI to keep track of this bug.

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

(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.