761929 - java.lang.IllegalStateException: The specified child already has a parent. You must call removeView() on the child''s parent first. at android.view.ViewGroup.addViewInner(ViewGroup.java) at com.android.internal.policy.impl.PhoneWindow.openPanel on ICS

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

It first appeared in 15.0a1/20120602 and has been hit by 11 ICS users so far. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=73783bf75c4c&tochange=5199196b65ec

java.lang.IllegalStateException: The specified child already has a parent. You must call removeView() on the child's parent first.
	at android.view.ViewGroup.addViewInner(ViewGroup.java:3337)
	at android.view.ViewGroup.addView(ViewGroup.java:3208)
	at android.view.ViewGroup.addView(ViewGroup.java:3188)
	at com.android.internal.policy.impl.PhoneWindow.openPanel(PhoneWindow.java:582)
	at com.android.internal.policy.impl.PhoneWindow.onKeyUpPanel(PhoneWindow.java:817)
	at com.android.internal.policy.impl.PhoneWindow.onKeyUp(PhoneWindow.java:1486)
	at com.android.internal.policy.impl.PhoneWindow$DecorView.dispatchKeyEvent(PhoneWindow.java:1813)
	at android.view.ViewRootImpl.deliverKeyEventPostIme(ViewRootImpl.java:3327)
	at android.view.ViewRootImpl.handleMessage(ViewRootImpl.java:2597)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4424)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.IllegalStateException%3A+The+specified+child+already+has+a+parent.+You+must+call+removeView%28%29+on+the+child%27%27s+parent+first.+at+android.view.ViewGroup.addViewInner%28ViewGroup.java%29

Comment 1

[Aaron Train [:aaronmt]]

Easy to reproduce via Monkey, with 1000 pseudo-random events sent to recent builds

Comment 2

[Chris Peterson [:cpeterson]]

sriram, does this crash regression look familiar? Could this be another problem with static variables not being cleaned up?

Comment 3

[Sriram Ramasubramanian [:sriram]]

I am not too sure if we are doing something wrong. If you look into the stack trace of bug 739152 (which is also similar), the crash happens when somewhere a view tries to be added back to a different parent, through Gecko.

at org.mozilla.gecko.GeckoApp$31.run(GeckoApp.java:1387)

However here, this particular stacktrace doesn't have any trace of Gecko. This feels like it happens randomly at Android level, with no Fennec's intervention.

Comment 4

[Sriram Ramasubramanian [:sriram]]

http://forum.xda-developers.com/showthread.php?t=849676&page=130 -- says it happens (in cyanogen mod) when hardware (!!) keyboard is opened at the wrong time.

http://stackoverflow.com/questions/10454265/various-exceptions-caused-by-the-android-fragments-compatibility-library

And I remember a new library was added for swiping between awesomebar screens. Adding :wesj.

Comment 5

[Scoobidiver (away)]

Crashes in 14.0 Beta with this crash signature are unrelated to this bug: different stack, not a top crasher (see the regression range in comment 0), not specific to ICS.

Comment 6

[Marco Zehe (:MarcoZ)]

FWIW, I just ran into this crash while surfing and trying to go back to the previous page. I was sliding my finger around the bottom between the home and back buttons, trying to aim at the back button, when it happened. Report:
bp-1b2ed479-e000-42a1-845b-bfd362120628

Comment 7

[Scoobidiver (away)]

It's #4 top crasher in 15.0a2 and #9 in 16.0a1.

It might be a regression from bug 739412.

There are a few crashes on Gingerbread with Linux kernel 3.1.10_IcyGlitch.

Here is the breakdown by device on:
* Aurora:
Samsung GT-I9100 	8
HTC One X 	7
HTC One V 	7
Samsung GT-P6210 	3
Samsung GT-P1000 	3
Unknown P300(Funbook) 	3
Samsung Galaxy Nexus 	3
Unknown AIRPAD 	2
Samsung GT-N7000 	1
Malata TeamDRH ICS for GTablet 	1
HTC Desire 	1
* Nightly:
samsung Galaxy Nexus 	4
Samsung GT-N7000 	4
Samsung GT-I9100 	2
telechips A3HD 	2
samsung GT-P1000 	2
samsung GT-I9100 	1
Samsung GT-P1000 	1
Samsung GT-I9000 	1
HTC ADR6425LVW 	1
HTC HD2 	1

Comment 8

[Alex Keybl [:akeybl]]

Hey Sriram/Mark - how can we help QA try to reproduce here? If there are no actionable leads for QA, can we make any speculative fixes early in the 15 beta cycle?

Comment 9

[Scoobidiver (away)]

Comment 1 has STR.

Comment 10

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

Sriram - Please take a look

Comment 11

[Sriram Ramasubramanian [:sriram]]

https://crash-stats.mozilla.com/report/index/9919af70-d3a7-42f7-98a2-45ec62120720 -- This is a different crash unrelated to this bug -- but shows up in the same crash stats.

https://crash-stats.mozilla.com/report/index/ca1b48a7-8e9f-4e91-b6cd-db2f52120720 -- This is a crash reported in this bug.

If you look into both the stack traces, 
#1 has something in GeckoApp try to add a view that already is attached to another parent. This is our fault -- which we can fix.
#2 has nothing done by GeckoApp. The stacktrace doesn't really have to be from us. This could be by Android trying to add something (like I mentioned, a h/w keyboard or something). So, we don't own this crash. -- which is what the bug reports.

Please file a bug for the other crash.

Comment 12

[Sriram Ramasubramanian [:sriram]]

There are so many crashes unrelated to this bug. Basically in some parts of the code, we try to add the same view to two different parents. If we solve that, those crashes will be solved.

This crash mentioned here -- it's not our fault.

Also, I would be interested to know, if we see similar crash reports in Google Play market, as they reports crashes for the deployed app. Please update the bug, if the crashes there match the same stack trace mentioned here.

Comment 13

[Scoobidiver (away)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #11)
> #1 has something in GeckoApp try to add a view that already is attached to
> another parent. This is our fault -- which we can fix.
It's already fixed in Fx 15.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Is it good to close this bug then? As it's not our fault..

Comment 15

[Scoobidiver (away)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> This crash mentioned here -- it's not our fault.
Why is Fx 14 unaffected?

Comment 16

[Robert Kaiser]

(In reply to Sriram Ramasubramanian [:sriram] from comment #14)
> Is it good to close this bug then? As it's not our fault..

Well, our users don't care whose fault it is, they only care that Firefox is crashing. Is there some way to circumvent the problem?

Comment 17

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

I concur with kairo and scoobidiver.  Even if it isn't our fault, people will blame us for the issue as this crash doesn't happen in other browsers.

Comment 18

[Sriram Ramasubramanian [:sriram]]

I agree that users don't care whose fault it is.
But here is the problem:
1. We cannot identify such a crash -- as it can happen anytime.
   And where exactly could be trap the exceptions?
2. We don't have a proper STR to add exception handlers in that path.

We can't do anything but wait for the user to restart.

Comment 19

[Scoobidiver (away)]

I checked some crashes with this signature in 14.0.1 and none of them has a stack trace containing com.android.internal.policy.impl.PhoneWindow.openPanel, so a change in Fx 15.0 makes it appear.
It looks similar to bug 775271 that also first appeared in 15.0 and occurs on ICS.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Here is a possibility (which I got while trying MonkeyRunner):
When the device is connected to a h/w keyboard, and menu key is pressed in h/w keyboard, we crash for custom menu -- starting 15.0.

Comment 21

[Scoobidiver (away)]

I think it's a regression from bug 739412 that is in the regression range (see comment 0).

Comment 22

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

As I had mentioned, this particular crash happens on honeycomb+ devices having a soft menu button, and the menu is tried to be opened using a h/w menu button -- say from a bluetooth keyboard.

This patch monitors for the MENU key at activity level and opens the custom menu, if the device had a custom menu attached to soft menu. If not, it uses the standard android way.

Note, onKeyDown() for Activity is called only when nothing else in the activity needs it. So, we won't have problems with the content.

The patch is all over the places just to decouple it between Activities and the actual responder -- BrowserToolbar.

Comment 23

[Matt Brubeck (:mbrubeck)]

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

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

::: mobile/android/base/GeckoApp.java
@@ +716,5 @@
> +    @Override
> +    public boolean onKeyDown(int keyCode, KeyEvent event) {
> +        // Custom Menu should be opened when hardware menu key is pressed.
> +        if (Build.VERSION.SDK_INT >= 11 && keyCode == KeyEvent.KEYCODE_MENU)
> +            return onMenuKeyPressed();

I don't think the SDK_INT check is needed here; there's already a check in BrowserApp.onMenuKeyPressed, which seems like the right place to do it.

Actually, why not just move all this code to BrowserApp?  If WebApp wants to respond to the menu key in some future version, it can just provide its own onKeyDown.  For now, it doesn't seem like putting this in the base class buys us anything.

Comment 24

[Sriram Ramasubramanian [:sriram]]

(In reply to Matt Brubeck (:mbrubeck) from comment #23)
> Comment on attachment 645470 [details] [diff] [review]
> Patch
> 
> Review of attachment 645470 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoApp.java
> @@ +716,5 @@
> > +    @Override
> > +    public boolean onKeyDown(int keyCode, KeyEvent event) {
> > +        // Custom Menu should be opened when hardware menu key is pressed.
> > +        if (Build.VERSION.SDK_INT >= 11 && keyCode == KeyEvent.KEYCODE_MENU)
> > +            return onMenuKeyPressed();
> 
> I don't think the SDK_INT check is needed here; there's already a check in
> BrowserApp.onMenuKeyPressed, which seems like the right place to do it.
> 
> Actually, why not just move all this code to BrowserApp?  If WebApp wants to
> respond to the menu key in some future version, it can just provide its own
> onKeyDown.  For now, it doesn't seem like putting this in the base class
> buys us anything.

I can monitor this at BrowserApp level. However, that didn't feel the right approach to me.
1. The custom menu operations should then be moved into BrowserApp.
   If they stay in GeckoApp -- everything related to Custom Menu will be in GeckoApp, this alone will stick out in BrowserApp -- which is wrong.
2. Build check is just for parity with other methods like onCreatePanelView() and onCreatePanelMenu(). It's just to make sure, we are looking at that part of code -- just for 11+.

Comment 25

[Matt Brubeck (:mbrubeck)]

(In reply to Sriram Ramasubramanian [:sriram] from comment #24)
> 1. The custom menu operations should then be moved into BrowserApp.
>    If they stay in GeckoApp -- everything related to Custom Menu will be in
> GeckoApp, this alone will stick out in BrowserApp -- which is wrong.

But it seems to me you are already adding this logic to BrowserApp, here:

>+    public boolean onMenuKeyPressed() {
>+        if (Build.VERSION.SDK_INT >= 11)
>+            return mBrowserToolbar.onMenuKeyPressed();

Putting the same code directly into BrowserApp.onKeyDown seems to be simpler, and just as clean.  Either way, BrowserApp now contains some of the custom menu logic (which is fine, since it is responsible for interaction with BrowserToolbar).

Comment 26

[Matt Brubeck (:mbrubeck)]

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

Okay, Sriram convinced me that it is useful to keep all the custom menu logic in GeckoApp, since WebApp may need it soon.  r=mbrubeck with one nit:

>+    @Override
>+    public boolean onMenuKeyPressed() {
>+        if (Build.VERSION.SDK_INT >= 11)
>+            return mBrowserToolbar.onMenuKeyPressed();
>+
>+        return false;
>+    }

We can remove the SDK_INT check from this BrowserApp method, since GeckoApp takes care of that logic for us.

Comment 27

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch cleans up the code.
Ideally when s/w or h/w menu button is pressed, we should be opening the menu by openOptionsMenu(). So, the onClickListener() of our s/w menu, and the h/w menu key from external keyboard should just call openOptionsMenu().

Extra condition checks (that were earlier in BrowserToolbar) is moved to BrowserApp, as BrowserApp is the one who should do the checks. BrowserApp knows BrowserToolbar might have a popup menu, hence calls BrowserToolbar's implementation. If BrowserToolbar doesn't have a popup menu, the default implementation is called.

Comment 28

[Sriram Ramasubramanian [:sriram]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a2923f4b32dd

Comment 29

[Sriram Ramasubramanian [:sriram]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Custom menu.
User impact if declined: Phones/Tablets with s/w menu button won't respond for menu from external keyboard.
Testing completed (on m-c, etc.): Landed in m-i on 07/24 
Risk to taking this patch (and alternatives if risky): None. This just makes sure to open menu, on press of h/w menu key.
String or UUID changes made by this patch: None.

Comment 30

[Ed Morley [:emorley]]

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

Comment 31

[Lukas Blakk [:lsblakk] use ?needinfo]

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

No risk patch, mobile-only, fixes native crash - approving.

Comment 32

[Sriram Ramasubramanian [:sriram]]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/97a20755be4f

Comment 33

[Sriram Ramasubramanian [:sriram]]

Pushed to beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/e956c519281c