1366352 - Crash in java.lang.NullPointerException: Attempt to invoke virtual method ''int android.view.View.getWidth()'' on a null object reference at org.mozilla.gecko.menu.MenuPopup.showAsDropDown(MenuPopup.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andrzej Hunt :ahunt]

This bug was filed from the Socorro interface and is 
report bp-6a1a9d57-954c-48b6-b371-bb2c00170519.
=============================================================

Comment 1

[Andrzej Hunt :ahunt]

This looks like a regression from Bug 1282763:
https://hg.mozilla.org/mozilla-central/rev/073efe219a15#l1.34

If a user clicks on the menu button before the Runnable executes, the menu has no anchor, and we crash in MenuPopup.showAsDropDown().

Comment 2

[Andrzej Hunt :ahunt]

STR: convert postToUiThread() to postDelayedToUiThread() with a large delay (e.g. 2000). Build, run, open tabs tray, press menu button -> crashes.

Comment 3

[Andrzej Hunt :ahunt]

Attachment: Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

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

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Bug 1366352 - Post: check anchor is set before trying to show GeckoPopupMenu

This means we'll at least see a sensible exception, instead of an NPE when
no anchor is set. That makes it easier to debug cases where no anchor was
set (see e.g. the main commit from this bug).

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

Comment 5

[Andrzej Hunt :ahunt]

Comment on attachment 8869558 [details]
Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141134/diff/1-2/

Comment 6

[Andrzej Hunt :ahunt]

Comment on attachment 8869559 [details]
Bug 1366352 - Post: check anchor is set before trying to show GeckoPopupMenu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141136/diff/1-2/

Comment 7

[Andrzej Hunt :ahunt]

It seems there are multiple instances in crash-stats, currently:
No. 54, 0.23%, 620 / 7 days
No. 70, 0.18%, 488 / 7 days

Not horrendous, but it might still be worthy of uplifting?

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8869558 [details]
Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

https://reviewboard.mozilla.org/r/141134/#review145756

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8869559 [details]
Bug 1366352 - Post: check anchor is set before trying to show GeckoPopupMenu

https://reviewboard.mozilla.org/r/141136/#review145760

::: mobile/android/base/java/org/mozilla/gecko/widget/GeckoPopupMenu.java:141
(Diff revision 2)
>      /**
>       * Show the inflated menu.
>       */
>      public void show() {
> +        if (mAnchor == null) {
> +            throw new IllegalStateException("GeckoPopupMenu.show() called without preceeding call to setAnchor()");

Why do we throw IllegalStateException here instead of just doing nothing?

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8869558 [details]
Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141134/diff/2-3/

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8869559 [details]
Bug 1366352 - Post: check anchor is set before trying to show GeckoPopupMenu

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/141136/diff/2-3/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66a8c78c4c24
Ensure anchor is always set before trying to show tabs tray menu r=nechen
https://hg.mozilla.org/integration/autoland/rev/80286a18d5d6
Post: check anchor is set before trying to show GeckoPopupMenu r=nechen

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/66a8c78c4c24
https://hg.mozilla.org/mozilla-central/rev/80286a18d5d6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 15

[Marcia Knous [:marcia]]

Tracking 54/55+. Between the two signatures, I think it would be great to have this fixed in beta.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

This has probably missed 54 now due to the lack of timely approval requests. Given the high volume we've seen on 53, there's probably good reason to suspect that it'll spike on 54 once that goes to release as well. Can you please help get this nominated for mozilla-release approval so it's at least on the radar for dot release consideration?

Comment 17

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8869558 [details]
Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

Approval Request Comment
[Feature/Bug causing the regression]: In tab tray, if Anchor is not set before menuPopup.showAsDropDown is called, we'll have a Null Pointer Exception 
[User impact if declined]:App will crash and it's hard to debug
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: It adds more check. and hook the UI base on System-based callback. So it should be safe.
[String changes made/needed]: No

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8869558 [details]
Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

54 RC build is released today and it's late for 54. Release54-. Mark 54 won't fix.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

I'm setting this back to affected because I think this is high-volume enough on 53 to pay attention to after 54 goes to release...

Comment 20

[Gerry Chang [:gchang]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> I'm setting this back to affected because I think this is high-volume enough
> on 53 to pay attention to after 54 goes to release...

Make sense. Keep tracking this in 54.

Comment 21

[Gerry Chang [:gchang]]

Comment on attachment 8869558 [details]
Bug 1366352 - Ensure anchor is always set before trying to show tabs tray menu

Fix a crash. Release54+. Should be in 54.0.1.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/54f5341b8f16
https://hg.mozilla.org/releases/mozilla-release/rev/e92ae23e769b

Comment 23

[Bogdan Surd, QA [:bsurd, NI]]

Hello, the SV team looked over this and we could not reproduce the issue in our testing session.