Closed Bug 761929 Opened 8 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect, critical)

15 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 17
Tracking Status
firefox14 --- unaffected
firefox15 + fixed
firefox16 + fixed
fennec 15+ ---

People

(Reporter: scoobidiver, Assigned: sriram)

Details

(4 keywords, Whiteboard: [native-crash])

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Crash Signature: [@ 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)] → [@ 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) ]
Easy to reproduce via Monkey, with 1000 pseudo-random events sent to recent builds
Keywords: reproducible
sriram, does this crash regression look familiar? Could this be another problem with static variables not being cleaned up?
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.
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.
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.
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
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
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?
Assignee: nobody → sriram
Comment 1 has STR.
tracking-fennec: ? → 15+
Sriram - Please take a look
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.
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.
(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.
Is it good to close this bug then? As it's not our fault..
(In reply to Sriram Ramasubramanian [:sriram] from comment #12)
> This crash mentioned here -- it's not our fault.
Why is Fx 14 unaffected?
(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?
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.
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.
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.
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.
I think it's a regression from bug 739412 that is in the regression range (see comment 0).
Attached patch Patch (obsolete) — Splinter Review
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.
Attachment #645470 - Flags: review?(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.
Attachment #645470 - Flags: review?(mbrubeck) → review-
(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+.
(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 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.
Attachment #645470 - Flags: review- → review+
Attached patch PatchSplinter Review
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.
Attachment #645604 - Flags: review?(mbrubeck)
Attachment #645604 - Flags: review?(mbrubeck) → review+
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.
Attachment #645604 - Flags: approval-mozilla-beta?
Attachment #645604 - Flags: approval-mozilla-aurora?
Attachment #645470 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a2923f4b32dd
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17
Comment on attachment 645604 [details] [diff] [review]
Patch

No risk patch, mobile-only, fixes native crash - approving.
Attachment #645604 - Flags: approval-mozilla-beta?
Attachment #645604 - Flags: approval-mozilla-beta+
Attachment #645604 - Flags: approval-mozilla-aurora?
Attachment #645604 - Flags: approval-mozilla-aurora+
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.