1660201 - ToolbarPanelHub.jsm assumes that appmenu-whatsnew-button is always available, causing JS errors

Status: VERIFIED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[:Gijs (he/him)]

It's unclear to me right now if this causes any user-visible issues. But I'm seeing JS errors so I figured I'd file them.

STR:

1. clean profile
2. open firefox
3. open browser console
4. open new browser window
5. close that new browser window

ER:
no errors

AR:

TypeError: can't access property "setAttribute", document.getElementById(...) is nullToolbarPanelHub.jsm:487:14
    _hideElement resource://activity-stream/lib/ToolbarPanelHub.jsm:487
    _hideAppmenuButton resource://activity-stream/lib/ToolbarPanelHub.jsm:467
    _hideAppmenuButton self-hosted:1161
    observer resource:///modules/EveryWindow.jsm:68

Seems the ToolbarPanelHub _hideElement code at https://searchfox.org/mozilla-central/rev/6cc48251bb97600fdf11a5b4c5f621bfc8606d55/browser/components/newtab/lib/ToolbarPanelHub.jsm#487 assumes that it can just document.getElementById things, but that doesn't work.

It's also not really clear to me why this code is running when the window closes; it doesn't seem necessary? :Mardak, can you help explain what the code is trying to do? As it is I'm a little lost.

Comment 1

[Andrei Oprea [:andreio]]

It's using EveryWindow to register event callbacks. Normally the add/remove/DOM manipulation is all handled in ToolbarPanelHub but there's an extra call to the unregister fn on domwindowclose which I did not account for.

Comment 2

[Andrei Oprea [:andreio]]

Attachment: Bug 1660201 - Don't try to access node on domwindowclosed in ToolbarPanelHub.jsm

Comment 3

[Ed Lee :Mardak]

Thanks andreio. Bug 1563319 originally added this code and other places like bug 1561880 switched over to EveryWindow

Comment 4

[Jens Stutte [:jstutte]]

(In reply to :Gijs (he/him) from comment #0)

It's unclear to me right now if this causes any user-visible issues. But I'm seeing JS errors so I figured I'd file them.

Hi Tim, if you agree, can you please set the severity accordingly? Thank you!

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/354216b0c3b8
Don't try to access node on domwindowclosed in ToolbarPanelHub.jsm r=Gijs

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/354216b0c3b8

Comment 7

[Marius Coman [:mcoman], Ecosystem QA]

I have verified, using the steps from the description, that this issue is no longer reproducible with the latest Firefox Nightly (81.0a1 Build ID - 20200824094458) installed, on Windows 10 x64, Ubuntu 18.04 x64 and Mac 10.15. Now, I can confirm that the error is no longer displayed after the window is closed.