945832 - java.lang.IllegalStateException: No ContextGetter; cannot fetch prefs. on rotation or system locale change

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

FATAL EXCEPTION: main
java.lang.IllegalStateException: No ContextGetter; cannot fetch prefs.
	at org.mozilla.gecko.LocaleManager.getSharedPreferences(LocaleManager.java:78)
	at org.mozilla.gecko.LocaleManager.getPersistedLocale(LocaleManager.java:175)
	at org.mozilla.gecko.LocaleManager.getCurrentLocale(LocaleManager.java:130)
	at org.mozilla.gecko.LocaleManager.correctLocale(LocaleManager.java:88)
	at org.mozilla.gecko.GeckoApplication.onConfigurationChanged(GeckoApplication.java:44)
	at android.app.ActivityThread.performConfigurationChanged(ActivityThread.java:4385)
	at android.app.ActivityThread.handleConfigurationChanged(ActivityThread.java:4634)
	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1536)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:158)
	at android.app.ActivityThread.main(ActivityThread.java:5789)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:525)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1027)
	at com.android.internal.os.ZygoteInit.main(Zygote

This should never occur, as they say, and yet it is.

This should be fixed when Bug 936756 is relanded.

Comment 1

[Richard Newman [:rnewman]]

Attachment: java.lang.IllegalStateException: No ContextGetter; cannot fetch prefs. on rotation or system locale change.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 8341842 [details] [diff] [review]
java.lang.IllegalStateException: No ContextGetter; cannot fetch prefs. on rotation or system locale change.

If GeckoApplication is ever instantiated without a GeckoApp, LocaleManager won't have a ContextGetter, and will throw when GeckoApplication tries to fix the locale in a provided config.

This patch adds a tiny amount of logging and catches that exception.

See also Bug 945840, which really ought to invert ContextGetter to use the Application instance.

Comment 3

[Wesley Johnston (:wesj)]

Comment on attachment 8341842 [details] [diff] [review]
java.lang.IllegalStateException: No ContextGetter; cannot fetch prefs. on rotation or system locale change.

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

::: mobile/android/base/GeckoApp.java
@@ +1295,5 @@
>                  final SharedPreferences prefs = GeckoApp.getAppSharedPreferences();
>  
>                  // Wait until now to set this, because we'd rather throw an exception than 
>                  // have a caller of LocaleManager regress startup.
> +                Log.d(LOGTAG, "Calling setContextGetter: " + GeckoApp.this);

Do we need this logging and the logging in setContextGetter? I would remove this logging and just let the line in setContextGetter run if you really want things logged.

::: mobile/android/base/GeckoApplication.java
@@ +15,5 @@
>  import android.content.res.Configuration;
>  import android.util.Log;
>  
>  public class GeckoApplication extends Application {
> +    private static final String LOG_TAG = "GeckoApplication";

Nit: We always use LOGTAG.

@@ +45,5 @@
> +        try {
> +            LocaleManager.correctLocale(getResources(), config);
> +        } catch (IllegalStateException ex) {
> +            // GeckoApp hasn't started, so we have no ContextGetter in
> +            // LocaleManager.

Nit: In the past, we've explicitly said we don't care about 80 char lines here. You might as well move this up.

@@ +46,5 @@
> +            LocaleManager.correctLocale(getResources(), config);
> +        } catch (IllegalStateException ex) {
> +            // GeckoApp hasn't started, so we have no ContextGetter in
> +            // LocaleManager.
> +            Log.w(LOG_TAG, "Couldn't correct locale.", ex);

Nit: No need for the period.

Comment 4

[Richard Newman [:rnewman]]

Thanks for the quick review!

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

> Nit: We always use LOGTAG.

s/always/sometimes. :P

$ ag "LOG_TAG =" | wc -l
     122
$ ag "LOGTAG =" | wc -l
     147


> Nit: No need for the period.

ITYM "No need for the period"

Comment 5

[Richard Newman [:rnewman]]

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

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/942827d00677

Comment 7

[Aaron Train [:aaronmt]]

I'm still seeing this on Nightly (12/12). I'll open a new bug.