732572 - Changing Android system language while fennec is running yields a blank page and blank tab counter -- android.app.IntentReceiverLeaked

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (03/02) - about:home content MIA

E/StrictMode(15801): class org.mozilla.fennec.App; instances=4; limit=1
E/StrictMode(15801): android.os.StrictMode$InstanceCountViolation: class org.mozilla.fennec.App; instances=4; limit=1
E/StrictMode(15801): 	at android.os.StrictMode.setClassInstanceLimit(StrictMode.java:1)
D/dalvikvm(15801): Trying to load lib /data/data/org.mozilla.fennec/lib/libmozglue.so 0x41808090
D/dalvikvm(15801): Shared lib '/data/data/org.mozilla.fennec/lib/libmozglue.so' already loaded in same CL 0x41808090
W/GeckoApp(15801): zerdatime 19272526 - onCreate
W/GeckoApp(15801): zerdatime 19272539 - onStart
I/GeckoApp(15801): start
W/PhoneWindow(15801): Previously focused view reported id 2131492871 during save, but can't be found during restore.
I/GeckoApp(15801): resume
D/GeckoInputConnection(15801): IME: resetSelection
W/GeckoProfile(15801): zerdatime 19272554 - start check sessionstore.js exists
W/GeckoProfile(15801): zerdatime 19272554 - finish check sessionstore.js exists
E/ActivityThread(15801): Activity org.mozilla.fennec.App has leaked IntentReceiver android.accounts.AccountManager$12@41a52838 that was originally registered here. Are you missing a call to unregisterReceiver()?
E/ActivityThread(15801): android.app.IntentReceiverLeaked: Activity org.mozilla.fennec.App has leaked IntentReceiver android.accounts.AccountManager$12@41a52838 that was originally registered here. Are you missing a call to unregisterReceiver()?
E/ActivityThread(15801): 	at android.app.LoadedApk$ReceiverDispatcher.<init>(LoadedApk.java:763)
E/ActivityThread(15801): 	at android.app.LoadedApk.getReceiverDispatcher(LoadedApk.java:567)
E/ActivityThread(15801): 	at android.app.ContextImpl.registerReceiverInternal(ContextImpl.java:1043)
E/ActivityThread(15801): 	at android.app.ContextImpl.registerReceiver(ContextImpl.java:1030)
E/ActivityThread(15801): 	at android.app.ContextImpl.registerReceiver(ContextImpl.java:1024)
E/ActivityThread(15801): 	at android.accounts.AccountManager.addOnAccountsUpdatedListener(AccountManager.java:1899)
E/ActivityThread(15801): 	at org.mozilla.gecko.AboutHomeContent.init(AboutHomeContent.java:142)
E/ActivityThread(15801): 	at org.mozilla.gecko.GeckoApp$AboutHomeRunnable.run(GeckoApp.java:1112)
E/ActivityThread(15801): 	at android.os.Handler.handleCallback(Handler.java:605)
E/ActivityThread(15801): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/ActivityThread(15801): 	at android.os.Looper.loop(Looper.java:137)
E/ActivityThread(15801): 	at org.mozilla.gecko.GeckoApp$35.run(GeckoApp.java:1766)
E/ActivityThread(15801): 	at android.os.Handler.handleCallback(Handler.java:605)
E/ActivityThread(15801): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/ActivityThread(15801): 	at android.os.Looper.loop(Looper.java:137)
E/ActivityThread(15801): 	at android.app.ActivityThread.main(ActivityThread.java:4340)
E/ActivityThread(15801): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/ActivityThread(15801): 	at java.lang.reflect.Method.invoke(Method.java:511)
E/ActivityThread(15801): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
E/ActivityThread(15801): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
E/ActivityThread(15801): 	at dalvik.system.NativeStart.main(Native Method)
D/GeckoFavicons(15801): Creating Favicons instance
D/GeckoFavicons(15801): Creating DatabaseHelper

STR:

1. Idle on about:home
2. Back out into Android system settings and change the system language, e.g, English to Российская.
3. Switch back to Nightly, and see about:home

See screenshot.

This is another case where content vanishes on about:home.

--
Samsung Galaxy Nexus (Android 4.0.2)
Nightly (03/02)
Mozilla/5.0 (Android; Mobile; rv:13.0) Gecko/13.0 Firefox/13.0a1

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

As per the documentation, the registered listener should be removed on onDestroy(). Also, the PopupWindow added as a part of DoorHangerPopup also leaks. I've fixed that too, as a part of this patch.

However, I don't see the about:home still. :( Probably because I am running a single-locale apk. 

Even for a single locale apk, if the system runs in different locale which we don't have, we should default to English. Are we doing it? I would wait for this patch to land -- check it in Nightly if the about:home comes in proper language -- and file a bug for "default" language.

Comment 2

[Matt Brubeck (:mbrubeck)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #1)
> However, I don't see the about:home still. :( Probably because I am running
> a single-locale apk.

No, this bug happens even if you are changing *to* English.  The problem is that changing the configuration causes the activity to be destroyed and re-created, but the newly created activity fails to show about:home because hasSession() returns "true" here:

https://hg.mozilla.org/mozilla-central/file/343ec916dfd5/mobile/android/base/GeckoApp.java#l1692

There are other problems too; for example, the tab counter is missing.

Comment 3

[Sriram Ramasubramanian [:sriram]]

This problem happens "when language is changed". And I just fixed it :D
mInitialized is true -- as the class remains.
The sequence of calls is as follows.
onDestroy()
onRestart()
onCreate()
..
onResume()

Since mInitialized is true, about:home is not showing up when everything is rendered again.

I am uplaoding a new patch soon.

Comment 4

[Matt Brubeck (:mbrubeck)]

Note: This happens on any page, not just about:home.

One workaround would be to add configChanges="locale" to AndroidManifest.xml, but this would prevent us from automatically loading new string resources when the locale changes.  And this bug would still exist for any config types not listed in the manifest (including ones added in future OS versions).  It would be better to make sure our activity restarts correctly.

(In reply to Sriram Ramasubramanian [:sriram] from comment #3)
> Since mInitialized is true, about:home is not showing up when everything is
> rendered again.

Hmm, my own logging showed that mInitialized was false when the activity was recreated... I'll try again with your patch.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Monitoring locale and acting upon it is to going to be a big hassle. It's better to leave android to do it for us.

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch resets mInitialized to false in onRestart(). We need to do this only when we are restarting. On a cold start or after an OOM kill, the activity will be "created afresh" and mInitialized will be false. (Basically the variables are not newly created in onRestart() path).

I believe that the strings will take the values from the actual locale in multi-locale apk.

Comment 7

[Matt Brubeck (:mbrubeck)]

This patch does not fix the bug for me.  I still see a blank screen and missing tab counter after I change the system language while Fennec is running.

As far as I can tell, the activity *is* created afresh when it gets restarted, and the problem is that we don't restore the existing tabs or re-display the gecko view.  This also leaves Fennec in a state where web pages fail to render.

Comment 8

[Sriram Ramasubramanian [:sriram]]

I verified it and it works for me. :(
If we are looking for "locale" changes in onConfigurationChanged(), how do you think we should handle it?

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #8)
> I verified it and it works for me. :(

These are my steps to reproduce:
1. Launch Fennec
2. Minimize Fennec and open the "Settings" app
3. Go to the "Language and Keyboard" settings and select a new locale
4. Minimise the settings app
5. Tap on the Fennec icon to return to Fennec

After doing this I get a blank screen and missing tab indicator, even with the patch from attachment 602517 [details] [diff] [review].  I tested this on HTC G2 and 7" Galaxy Tab (both running Android 2.3).

> If we are looking for "locale" changes in onConfigurationChanged(), how do
> you think we should handle it?

I don't think we should add configChanges="locale", I just mentioned that as a possible last resort.

Instead, I think we probably need to add some code in onRestoreInstanceState to keep the current "session" (tabs, etc.) when the activity is destroyed and re-created.

Comment 10

[Sriram Ramasubramanian [:sriram]]

I was basically played around moving 
https://bugzilla.mozilla.org/attachment.cgi?id=602517&action=diff#a/mobile/android/base/GeckoApp.java_sec1 here and there :D

This worked on onDestroy(). And then I felt it to be in onRestart(). (This actually worked for me :( probably some compilation issues. But ya, this was slower!)

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I get a blank about:home screen when migrating a large profile.  Not sure if this is the same issue or not.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 602517 [details] [diff] [review]
Patch

Sounds like we need a new patch to fix this issue. Also sounds like Matt could do the review here. I am going offline for a few days.

Also have Brian look at the patch too since this will likely affect session restore.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Here is the list of events that happen in this path.

host-3-243:~/Documents/fennec/central/android/dist sriramramasubramanian$ adb logcat | grep Sriram
I/Sriram  (11649): oncreate
I/Sriram  (11649): mInitialized = false
I/Sriram  (11649): mAppContext in onCreate = null
I/Sriram  (11649): onstart
I/Sriram  (11649): onresume
I/Sriram  (11649): onpause
I/Sriram  (11649): onstop

-- The app goes to onStop() when we go to home screen. Now when the language is changed, it still remains in this state. Once we open Fennec again, onDestroy() is called, followed by onCreate. At this point, the "activity" is "not created afresh". This can be noted from the value of mAppContext in onCreate(). (The log prints it right before it is assigned. In the above log, it is "null", during first call to onCreate()).

I/Sriram  (11649): ondestroy
I/Sriram  (11649): mInitialized = true
I/Sriram  (11649): oncreate
I/Sriram  (11649): mInitialized = false
I/Sriram  (11649): mAppContext in onCreate = org.mozilla.fennec_sriramramasubramanian.App@41330840
I/Sriram  (11649): onstart
I/Sriram  (11649): onresume

-- In this path, however, onRestart() is not called. Where this patch fails unfortunately.
Self note: Don't keep playing with patches ;)
If we move the mInitialized to onDestroy(), we can be sure that things work fine.

I/Sriram  (11649): onpause
I/Sriram  (11649): onstop

-- If we are "resuming" the activity from "locking the screen and and unlocking it", we get an onRestart().

I/Sriram  (11649): onrestart
I/Sriram  (11649): onstart
I/Sriram  (11649): onresume


The part that we need to be sure of now is, what all need to be taken care of when the activity "doesn't get created afresh" in this sequence?

Comment 14

[Aaron Train [:aaronmt]]

Should we go the multi-locale route, this issue is still present and pretty bad.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Sriram - This bug will become important soon... likely a release blocker. We should get ready to deal with it.

Comment 16

[Sriram Ramasubramanian [:sriram]]

Attachment: Try

I tried finding if the static variables are having proper values, to see if the GeckoApp alone is restarted.

If sGeckoThread is not null, then Gecko is already started.
If mLayerController is not null, then we need to add the view back to mGeckoLayout.
However, this view needs to be "removed" first from its old parent, before adding. But then, at this point, we don't have the old parent. Hence, we cannot add the view back to mGeckoLayout.

mLayerController is tightly coupled to mAppContext. So, creating a new view is hard it seems.

So, while adding the view, the app gets killed and restarts -- which isn't the desired behavior I guess.

I tried doing a "doRestart()". There is an unwanted pause in restarting without any message to the user on why we are restarting.

Comment 17

[Sriram Ramasubramanian [:sriram]]

I tried doing a "doRestart()". I constantly get this crash:

I/WindowManager(  133): WIN DEATH: Window{419da468 org.mozilla.fennec_sriramramasubramanian/org.mozilla.fennec_sriramramasubramanian.App paused=false}
W/InputDispatcher(  133): channel '419da468 org.mozilla.fennec_sriramramasubramanian/org.mozilla.fennec_sriramramasubramanian.App (server)' ~ Consumer closed input channel or an error occurred.  events=0x8
E/InputDispatcher(  133): channel '419da468 org.mozilla.fennec_sriramramasubramanian/org.mozilla.fennec_sriramramasubramanian.App (server)' ~ Channel is unrecoverably broken and will be disposed!
W/InputDispatcher(  133): Attempted to unregister already unregistered input channel '419da468 org.mozilla.fennec_sriramramasubramanian/org.mozilla.fennec_sriramramasubramanian.App (server)'
W/WindowManager(  133): Force-removing child win Window{41aa8650 SurfaceView paused=false} from container Window{419da468 org.mozilla.fennec_sriramramasubramanian/org.mozilla.fennec_sriramramasubramanian.App paused=false}
I/ActivityManager(  133): Process org.mozilla.fennec_sriramramasubramanian (pid 2388) has died.
W/WindowManager(  133): Failed looking up window
W/WindowManager(  133): java.lang.IllegalArgumentException: Requested window android.os.BinderProxy@415dbab0 does not exist
W/WindowManager(  133): 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:7163)
W/WindowManager(  133): 	at com.android.server.wm.WindowManagerService.windowForClientLocked(WindowManagerService.java:7154)
W/WindowManager(  133): 	at com.android.server.wm.WindowState$DeathRecipient.binderDied(WindowState.java:1545)
W/WindowManager(  133): 	at android.os.BinderProxy.sendDeathNotice(Binder.java:417)
W/WindowManager(  133): 	at dalvik.system.NativeStart.run(Native Method)
I/WindowManager(  133): WIN DEATH: null
I/WindowManager(  133): WINDOW DIED Window{419da468 org.mozilla.fennec_sriramramasubramanian/org.mozilla.fennec_sriramramasubramanian.App paused=false}

Comment 18

[Sriram Ramasubramanian [:sriram]]

Capturing locale configuration changes and restarting works fine only for first time. It keep failing after that, as always.

Also, capturing locale is going to be a problem in that it should be captured in all activities.

Comment 19

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (1/2): Fix for restater

This is a pre-requisite for the fix. Restarter shouldnt have "excludeFromRecents" set. Setting that causing GeckoApp to load on a stack that is not shown in recent activity -- hence locale changes are not given to Fennec, even if it is running.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch (2/2): Restart for locale change.

This fixes the locale change restarting.
onRetainNonConfigurationInstance() is called when the app is running and the configuration (like locale) changes. This is a chance for the app to save any needed values. However, we have decided to restart the app, hence we return a non-null value. This non-null value is checked in onCreate() and restarted if needed.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 624500 [details] [diff] [review]
Patch (1/2): Fix for restater

We added the excludeFromRecent flag to the restarter so it wouldn't show up in the recent task list. Granted, fixing this bug is more important than the recent list, but I want to check. Does the ACTIVITY_CLEAR_TOP do something that affects the recent task list too?

Adding Brad so he is aware os this change.

Again, I am willing to regress the recent task list issue to fix this bug, if it is really needed.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 624502 [details] [diff] [review]
Patch (2/2): Restart for locale change.

I am glad this works but I have one issue with it. Let's assume this is the flow:

1. User starts Fennec
2. User puts Fennec in background
3. User starts Settings and changes locale
4. onRetainNonConfigurationInstance is called in Fennec
5. Fennec returns a Boolean(true) and Android kills the GeckoApp activity, but not the entire application
6. User switches back to Fennec
7. Android must recreate GeckoApp activity
8. Fennec checks the config, finds a |true| and does a full app restart
9. User sees Fennec restart

OK. Assuming this is the flow, I don't like that the user must wait for Fennec to restart after switching back. Would it be possible to do the restart in step #5, instead of storing the boolean?

Comment 23

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #21)
> Comment on attachment 624500 [details] [diff] [review]
> Patch (1/2): Fix for restater
> 
> We added the excludeFromRecent flag to the restarter so it wouldn't show up
> in the recent task list. Granted, fixing this bug is more important than the
> recent list, but I want to check. Does the ACTIVITY_CLEAR_TOP do something
> that affects the recent task list too?
> 
> Adding Brad so he is aware os this change.
> 
> Again, I am willing to regress the recent task list issue to fix this bug,
> if it is really needed.

We are not regressing here. excludeFromRecent for Restarter was fine -- in the sense, Restarter shouldn't be shown. But then, Restarter starts GeckoApp in its task stack, and GeckoApp isn't visible in recent activities. That's the reason excludeFromRecent shouldn't be used for Restarter. excludeFromRecent seems to apply for the entire task stack -- which is causing the problem.

CLEAR_TOP is used for re-using an activity it is already present, and put it at the bottom of the stack, and remove all activities above it. Here a new activity is created for us, this just ensures that it stays at the bottom of the stack.

Comment 24

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #22)
> Comment on attachment 624502 [details] [diff] [review]
> Patch (2/2): Restart for locale change.
> 
> I am glad this works but I have one issue with it. Let's assume this is the
> flow:
> 
> 1. User starts Fennec
> 2. User puts Fennec in background
> 3. User starts Settings and changes locale
> 4. onRetainNonConfigurationInstance is called in Fennec
> 5. Fennec returns a Boolean(true) and Android kills the GeckoApp activity,
> but not the entire application
> 6. User switches back to Fennec
> 7. Android must recreate GeckoApp activity
> 8. Fennec checks the config, finds a |true| and does a full app restart
> 9. User sees Fennec restart
> 
> OK. Assuming this is the flow, I don't like that the user must wait for
> Fennec to restart after switching back. Would it be possible to do the
> restart in step #5, instead of storing the boolean?

Agreed that this should be done silently. I tried doing it in step #5. This causes few StrictModeViolations with database. This is because, onStop(), onDestroy() are not called in proper order, if we trigger a restart there. That's the reason I had to wait till next onCreate() to trigger a restart.

Comment 25

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 624502 [details] [diff] [review]
Patch (2/2): Restart for locale change.

>+        if (getLastNonConfigurationInstance() != null) {
>+            doRestart();
>+            System.exit(0);
>+            return;

Let's add a comment above this check. Something like: "Check to see if this activity is being recreated from a configuration change" and inside the block: "Restart the application as a safe way to handle the configuration change"

>+    @Override
>+    public Object onRetainNonConfigurationInstance() {
>+        return new Boolean(true);

Add one here too: "Send a signal so we can restart the application in response to a configuration change"

I won't block this change on getting a more elegant solution in place. We can always file a new bug for restarting more immediately, instead of during re-creation.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 624500 [details] [diff] [review]
Patch (1/2): Fix for restater

After getting more details from Sriram, I feel more confident in this change and don't need backup review.

Comment 27

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/1c2c14d8be78
http://hg.mozilla.org/integration/mozilla-inbound/rev/c462ef73a65f

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1c2c14d8be78
https://hg.mozilla.org/mozilla-central/rev/c462ef73a65f

Comment 29

[Aaron Train [:aaronmt]]

Verified Fixed on Nightly (05/17) via: 20120517040216
http://hg.mozilla.org/integration/mozilla-inbound/rev/12f13acb5ea8

Aurora nomination?

Comment 30

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 624500 [details] [diff] [review]
Patch (1/2): Fix for restater

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -

User impact if declined: 
Locale change will show a blank screen to users. They would have to close the app and re-open. Closing apps in pre-honeycomb is not easier.

Testing completed (on m-c, etc.): 05/16

Risk to taking this patch (and alternatives if risky): No risk seen.

String or UUID changes made by this patch: None.

Comment 31

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 624502 [details] [diff] [review]
Patch (2/2): Restart for locale change.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -

User impact if declined: 
Locale change will show a blank screen to users. They would have to close the app and re-open. Closing apps in pre-honeycomb is not easier.

Testing completed (on m-c, etc.): 05/16

Risk to taking this patch (and alternatives if risky): No risk seen.

String or UUID changes made by this patch: None.

Comment 32

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09c74e820684
https://hg.mozilla.org/releases/mozilla-aurora/rev/671f45ef1092

Comment 33

[Catalin Suciu [:csuciu]]

Unable to reproduce using the steps from comment #0 and comment #9

Aurora 14.0a2 (2012-05-24)
Beta 14.0b2 
Samsung Galaxy SII (Android 2.3.4)