1401279 - java.lang.IllegalArgumentException: Cannot add a null child view to a ViewGroup when dismissing main menu

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mike Kaply [:mkaply]]

Android O exception:

Process: org.mozilla.firefox
PID: 12237
Flags: 0x38c83e45
Package: org.mozilla.firefox v2015503969 (55.0)
Foreground: Yes
Lifetime: 1215s
Build: XXX platform/ERT-A09/unknownERT:5.0.1/XXX platformERT-A09/39(C07):user/release-keys

java.lang.IllegalArgumentException: Cannot add a null child view to a ViewGroup
	at android.view.ViewGroup.addView(ViewGroup.java:4705)
	at android.view.ViewGroup.addView(ViewGroup.java:4687)
	at org.mozilla.gecko.GeckoApp.onOptionsMenuClosed(GeckoApp.java:661)
	at org.mozilla.gecko.toolbar.BrowserToolbar$11.onDismiss(BrowserToolbar.java:875)
	at android.widget.PopupWindow.dismiss(PopupWindow.java:1985)
	at android.widget.PopupWindow$PopupDecorView.onTouchEvent(PopupWindow.java:2503)
	at android.view.View.dispatchTouchEvent(View.java:11788)
	at android.view.ViewGroup.dispatchTransformedTouchEvent(ViewGroup.java:2968)
	at android.view.ViewGroup.dispatchTouchEvent(ViewGroup.java:2647)
	at android.widget.PopupWindow$PopupDecorView.dispatchTouchEvent(PopupWindow.java:2483)
	at android.view.View.dispatchPointerEvent(View.java:12027)
	at android.view.ViewRootImpl$ViewPostImeInputStage.processPointerEvent(ViewRootImpl.java:5203)
	at android.view.ViewRootImpl$ViewPostImeInputStage.onProcess(ViewRootImpl.java:5012)
	at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:4520)
	at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:4573)
	at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:4539)
	at android.view.ViewRootImpl$AsyncInputStage.forward(ViewRootImpl.java:4666)
	at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:4547)
	at android.view.ViewRootImpl$AsyncInputStage.apply(ViewRootImpl.java:4723)
	at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:4520)
	at android.view.ViewRootImpl$InputStage.onDeliverToNext(ViewRootImpl.java:4573)
	at android.view.ViewRootImpl$InputStage.forward(ViewRootImpl.java:4539)
	at android.view.ViewRootImpl$InputStage.apply(ViewRootImpl.java:4547)
	at android.view.ViewRootImpl$InputStage.deliver(ViewRootImpl.java:4520)
	at android.view.ViewRootImpl.deliverInputEvent(ViewRootImpl.java:7083)
	at android.view.ViewRootImpl.doProcessInputEvents(ViewRootImpl.java:7057)
	at android.view.ViewRootImpl.enqueueInputEvent(ViewRootImpl.java:7018)
	at android.view.ViewRootImpl$WindowInputEventReceiver.onInputEvent(ViewRootImpl.java:7237)
	at android.view.InputEventReceiver.dispatchInputEvent(InputEventReceiver.java:192)
	at android.os.MessageQueue.nativePollOnce(Native Method)
	at android.os.MessageQueue.next(MessageQueue.java:379)
	at android.os.Looper.loop(Looper.java:144)
	at android.app.ActivityThread.main(ActivityThread.java:7352)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.Zygote$MethodAndArgsCaller.run(Zygote.java:245)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:926)
-loghandler

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

	at android.view.ViewGroup.addView(ViewGroup.java:4705)
	at android.view.ViewGroup.addView(ViewGroup.java:4687)
	at org.mozilla.gecko.GeckoApp.onOptionsMenuClosed(GeckoApp.java:661)

And the code in onOptionsMenuClosed, from m-c, is:

    @Override
    public void onOptionsMenuClosed(Menu menu) {
        mMenuPanel.removeAllViews();
        mMenuPanel.addView((GeckoMenu) mMenu);
    }

So apparently mMenu is null (assuming this trace came from a version where this was the code).

Comment 2

[Mike Kaply [:mkaply]]

This was most likely 55.

Comment 3

[Jan Henning [:JanH]]

Again doesn't seem O-specific according to Crash Stats. The crash happens in
https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#605, which in this case is called from
https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java#908

As far as I can tell:
- BrowserToolbar's menuPopup is only set in openOptionsMenu [1] and always preceeded by a call to activity.getMenuPanel() (the "activity" always being BrowserApp). [1] is also the place where we set the onDismiss listener that then leads up to the crash.
- GeckoApp.getMenuPanel() checks for the existence of mMenuPanel [2] - if it's null, it's initialised via onCreatePanelMenu() and invalidateOptionsMenu(), otherwise we just return the existing mMenuPanel
- mMenuPanel is initialised either indirectly through onCreatePanelMenu() or directly through onCreatePanelView [3].
- mMenu however is initialised only through onCreatePanelMenu, which creates a new GeckoMenu and then writes it to mMenu via onCreateOptionsMenu [4].

This means that if somebody calls onCreatePanelView directly, we initialise mMenuPanel, but not mMenu. Since the rest of our code seems to be built on the assumption that mMenu will always have been initialised as well if mMenuPanel != null, we then run into this crash if my theory is correct. As far as I can see, short of onCreatePanelMenu we don't have any internal callers of onCreatePanelView, though, and I've no idea under which circumstances this might be called directly by the OS. Unless I've overlooked something, it seems the only way we could end up with mMenuPanel != null and mMenu == null, though.

[1] https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/mobile/android/base/java/org/mozilla/gecko/toolbar/BrowserToolbar.java#902
[2] https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#448
[3] Interestingly enough, onCreatePanelView directly creates a new MenuPanel and writes it to mMenuPanel and also returns a reference to it, while onCreatePanelMenu just writes the very same reference returned by onCreatePanelView to mMenuPanel again.
[4] https://dxr.mozilla.org/mozilla-central/rev/ffe6cc09ccf38cca6f0e727837bbc6cb722d1e71/mobile/android/base/java/org/mozilla/gecko/GeckoApp.java#516

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1401279 - Ensure that the menu itself is initialised as well when returning the menu panel.

Review commit: https://reviewboard.mozilla.org/r/181452/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181452/

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Forwarding this review to Nevin because I'm just on Activity Stream.

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Hi Mike
Do you have a STR for this?

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8909964 [details]
Bug 1401279 - Ensure that the menu itself is initialised as well when returning the menu panel.

Hi Julian
Have you seen this error before?

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8909964 [details]
Bug 1401279 - Ensure that the menu itself is initialised as well when returning the menu panel.

https://reviewboard.mozilla.org/r/181452/#review186802

r+ for the code. But it'll be better if we can find STR for this. Simplely closing the menu item can't reproduce it 100%

Comment 9

[Jan Henning [:JanH]]

This shouldn't make things any worse in any case - if mMenuPanel is != null in getMenuPanel(), but we now call onCreatePanelMenu() anyway because of mMenu == null, then onCreatePanelMenu() does it's own null check for mMenuPanel and doesn't replace it if it already exists.

So I'm inclined to just land this and see whether the crashes stop - unfortunately (in a manner of speaking - for users it's of course fortunate) the crash rate isn't very high, especially on Nightly/Beta, so we'll have to wait until this hits Release before knowing for sure whether this had any effect or not.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1459712dc9ca
Ensure that the menu itself is initialised as well when returning the menu panel. r=nechen

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1459712dc9ca

Comment 12

[Julian Chu [:walkingice]]

Comment on attachment 8909964 [details]
Bug 1401279 - Ensure that the menu itself is initialised as well when returning the menu panel.

Remove myself since this patch is already landed.

Comment 13

[Jan Henning [:JanH]]

No new crashes in 57, so at least in that regard this patch was successful - let's hope it didn't cause any other weird side effects on affected devices instead.