698114 - java.lang.IllegalArgumentException crash during rotation with select UI prompt

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

STR:

1. Rotate device with a select UI prompt visible

--

E/AndroidRuntime( 3191): FATAL EXCEPTION: Thread-11
E/AndroidRuntime( 3191): java.lang.IllegalArgumentException: View not attached to window manager
E/AndroidRuntime( 3191): 	at android.view.WindowManagerImpl.findViewLocked(WindowManagerImpl.java:355)
E/AndroidRuntime( 3191): 	at android.view.WindowManagerImpl.removeView(WindowManagerImpl.java:200)
E/AndroidRuntime( 3191): 	at android.view.Window$LocalWindowManager.removeView(Window.java:432)
E/AndroidRuntime( 3191): 	at android.app.Dialog.dismissDialog(Dialog.java:278)
E/AndroidRuntime( 3191): 	at android.app.Dialog.access$000(Dialog.java:71)
E/AndroidRuntime( 3191): 	at android.app.Dialog$1.run(Dialog.java:111)
E/AndroidRuntime( 3191): 	at android.app.Dialog.dismiss(Dialog.java:268)
E/AndroidRuntime( 3191): 	at org.mozilla.gecko.PromptService.onDestroy(PromptService.java:242)
E/AndroidRuntime( 3191): 	at org.mozilla.gecko.GeckoApp$24.run(GeckoApp.java:1331)
E/AndroidRuntime( 3191): 	at android.os.Handler.handleCallback(Handler.java:587)
E/AndroidRuntime( 3191): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/AndroidRuntime( 3191): 	at android.os.Looper.loop(Looper.java:130)
E/AndroidRuntime( 3191): 	at org.mozilla.gecko.GeckoAppShell$LooperThread.run(GeckoAppShell.java:143)

--

20111028133548
http://hg.mozilla.org/projects/birch/rev/ef30c6eb723a
Samsung Nexus S

Comment 1

[Aaron Train [:aaronmt]]

E/WindowManager( 3191): Activity org.mozilla.fennec.App has leaked window com.android.internal.policy.impl.PhoneWindow$DecorView@406451b0 that was originally added here
E/WindowManager( 3191): android.view.WindowLeaked: Activity org.mozilla.fennec.App has leaked window com.android.internal.policy.impl.PhoneWindow$DecorView@406451b0 that was originally added here
E/WindowManager( 3191): 	at android.view.ViewRoot.<init>(ViewRoot.java:258)
E/WindowManager( 3191): 	at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:148)
E/WindowManager( 3191): 	at android.view.WindowManagerImpl.addView(WindowManagerImpl.java:91)
E/WindowManager( 3191): 	at android.view.Window$LocalWindowManager.addView(Window.java:424)
E/WindowManager( 3191): 	at android.app.Dialog.show(Dialog.java:241)
E/WindowManager( 3191): 	at org.mozilla.gecko.PromptService.Show(PromptService.java:237)
E/WindowManager( 3191): 	at org.mozilla.gecko.PromptService.processMessage(PromptService.java:348)
E/WindowManager( 3191): 	at org.mozilla.gecko.GeckoAppShell$7.run(GeckoAppShell.java:1548)
E/WindowManager( 3191): 	at android.os.Handler.handleCallback(Handler.java:587)
E/WindowManager( 3191): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/WindowManager( 3191): 	at android.os.Looper.loop(Looper.java:130)
E/WindowManager( 3191): 	at org.mozilla.gecko.GeckoAppShell$LooperThread.run(GeckoAppShell.java:143)
W/PluginManager( 3191): zerdatime 1319838444951 - onCreate
W/dalvikvm( 3191): threadid=10: thread exiting with uncaught exception (group=0x40015560)

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I think onDestroy must be called after the view associated with GeckoApp is already destroyed, causing this crash?

blassey, hoping you know a bit about this and can tell me if I'm doing something stupid.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 570762 [details] [diff] [review]
Patch v1

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

yes, when onDestroy() is called on the activity, we're no longer visible (see http://developer.android.com/reference/android/app/Activity.html#ActivityLifecycle). Sorry, I should have caught this in the review. onPause sounds like the right place to do this, although I wonder why you need to dismiss the dialog in the first place (a code comment would be very useful).

One other note (this is something I also missed in the review), please rename this function and its sister to onActivityPause() and onActivityResume(). Their current naming makes me think this is an activity which would make what you're doing truely evil.

r- because I'd like to know why we need this before r+'ing

Comment 4

[Wesley Johnston (:wesj)]

No problem. Talked a bit about this with blassey on irc. I think the issue is that we are holding a reference to this dialog in our (static) gPromptService class. It holds a view which is attached to the main view of GeckoApp. When the user rotates, that GeckoApp view is destroyed, and our view "leaks" according to Android, throwing errors in logcat. If we then try to do anything with that view, we'll throw "java.lang.IllegalArgumentException: View not attached to window manager" and crash. Dismissing and reshowing the dialog seems to help avoid that.

One alternative solution I was avoiding is to use the onCreateDialog methods of the Activity (which will let android do the dialog management for us). Unfortunately they only take an id, so we'd have to do some fun trickery to show the correct messages/title/etc, but I see that they've been deprecated anyway.

The correct way now is apparently to use fragments. I like them. We should be using them for things like tab lists, awesomescreen, etc. instead of the multiple activities we have right now anyway (they'll make it easier to transition to tablet ui later). So I'm going to look at a rewrite using DialogFragments. I don't think it will be much work. Just a reorganization of what we have I hope.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This just saves us from being destroyed/recreated when switching. I have no idea what other things this might break.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 570856 [details] [diff] [review]
Patch v2

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

let's test the hell out of this

Comment 7

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

Note to testers:
Suggested testing: 
orientation changing; also try combinations with hitting the home button and playing with the virtual keyboard

ie
- change orientation hit home, change orientation, go back to app
- bring out keyboard hit home, change orientation go back to app
- hide the keyboard hit home, change orientation, go back to app
- bring out the SVK keyboard hit home, change orientation, close the keyboard, go back to the app

- reboot or quit the app if you lose track of your steps.

Comment 8

[Wesley Johnston (:wesj)]

my eyes are peeled

http://hg.mozilla.org/projects/birch/rev/7203d86d5868

Comment 10

[Aaron Train [:aaronmt]]

20111101040211
http://hg.mozilla.org/projects/birch/rev/7203d86d5868
Samsung Nexus S (Android 2.3.6)