920224 - Intermittent TEST-UNEXPECTED-FAIL | browser_876926_customize_mode_wrapping.js | Unexpected exception: TypeError: panelEl is undefined

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[:Gijs (he/him)]

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js | Unexpected exception: TypeError: panelEl is undefined With stack: afterPanelOpen@chrome://mochitests/content/browser/browser/components/customizableui/test/browser_876926_customize_mode_wrapping.js:158

I suspect this is a pretty simple thing to fix: the window open code should have an option to wait for delayed startup to have finished. Right now it doesn't, so there's no guarantee PanelUI.init will have run (it just usually has, which is why this failure is so rare). That said, we could (maybe should) also make PanelUI's getters just be lazy self-overwriting getters, which would eliminate the need for creating those references in init. We'd still need to fix the test because it tries to open the panel - which will definitely not work before init has been called.

Comment 1

[:Gijs (he/him)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=28297718&tree=UX

https://tbpl.mozilla.org/php/getParsedLog.php?id=28236741&tree=UX

Comment 2

[:Gijs (he/him)]

Attachment: make test wait for delayed startup,

We should probably at least do this bit.

Comment 3

[:Gijs (he/him)]

Attachment: add getters immediately,

But perhaps we should also consider this?

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 809470 [details] [diff] [review]
add getters immediately,

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

Er, why not just move |PanelUI.init()| from browser.js to the end of panelUI.js ? It's a pattern we use everywhere, and it makes it more obvious what's going on.

Comment 5

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 809451 [details] [diff] [review]
make test wait for delayed startup,

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

I suppose if we weren't having trouble with the window already being loaded, then we won't have trouble with delayed-startup already having finished.

Comment 6

[:Gijs (he/him)]

Attachment: initialize panelUI immediately,

Well, duh. Much more sensible.

Comment 7

[:Gijs (he/him)]

https://hg.mozilla.org/projects/ux/rev/f80b3c1cd29b
https://hg.mozilla.org/projects/ux/rev/a257f4a1591a

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> Created attachment 810438 [details] [diff] [review]
> initialize panelUI immediately,
> 
> Well, duh. Much more sensible.

Backed out because of mochitest orange.

https://hg.mozilla.org/projects/ux/rev/af53342e5e4d

Because you see, inbetween my writing the original version of this patch, and my writing per the suggestion of Unfocused, someone added stuff to init, namely:

    this.menuButton.addEventListener("mousedown", this);
    this.menuButton.addEventListener("keypress", this);

which means calling init before the document is loaded is no longer possible, so a more elaborate patch would be needed.

(this bug can remain fixed because the *other*, test-only, cset, should work just fine in avoiding the random orange this bug was filed about)

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

Can we just delay adding those event listeners?

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Jared Wein [:jaws] from comment #9)
> Can we just delay adding those event listeners?

Yea, I think it might be worth doing that. Having the PanelUI object half (or all?) broken during that period of startup is a footgun.

Comment 11

[:Gijs (he/him)]

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