1816693 - (latent) permafailing TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/webrtcLegacyIndicator.xhtml]

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect, P1)

Description

[Andreas Pehrson [:pehrsons]]

When re-enabling browser/base/content/test/webrtc/browser_devices_get_user_media.js for test/webrtc/legacyIndicator I get on linux-debug locally (and on try (including for other platforms)) the following permafailing errors:

ERROR TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media.js | leaked 1 window(s) until shutdown [url = chrome://browser/content/webrtcLegacyIndicator.xhtml]
ERROR TEST-UNEXPECTED-FAIL | browser/base/content/test/webrtc/browser_devices_get_user_media.js | leaked 1 window(s) until shutdown [url = about:blank]

These are caused by opening the permissions panel through the legacy indicator. get_user_media_screen.js exhibits the same issue.

I am getting this with the patches on bug 1775945, but also on m-c.

Comment 1

[Andreas Pehrson [:pehrsons]]

Did a big effort analyzing this but pernosco and DMD were not much help in this case unfortunately.

The leak is in nsXULPopupManager which is not participating in GC but has a member mNativeMenuSubmenuState a nsTHashMap<RefPtr<mozilla::dom::Element>, nsPopupState> where the hash key keeps a strong ref to a dom::Element.

I haven't worked out the exact chain of events but have verified that making the key a rawptr solves the leak. The keys are never accessed directly so as long as the caller of these native menu event handlers keeps a strong ref it should be safe. I'll put it up for review and take it from there.

Comment 2

[Andreas Pehrson [:pehrsons]]

Sorry, I'll have to backtrack on the regressor. Still looks suspect but for the failing test case this hash map is not used. I had removed a verification thing from a .js file and only did ./mach build binaries so got a false positive.

Comment 3

[Andreas Pehrson [:pehrsons]]

It appears to be the _openPopupCancelCallback from bug 1434883 which captures the entire scope, including the OpenPopupOptions where triggerEvent has a strong ref to the webrtcLegacyIndicator.xhtml window.

OTOH the options with triggerEvent were added later in bug 1506503 so I guess both combined count as regressors.

Comment 4

[:Gijs (he/him)]

(In reply to Andreas Pehrson [:pehrsons] from comment #3)

It appears to be the _openPopupCancelCallback from bug 1434883 which captures the entire scope, including the OpenPopupOptions where triggerEvent has a strong ref to the webrtcLegacyIndicator.xhtml window.

OTOH the options with triggerEvent were added later in bug 1506503 so I guess both combined count as regressors.

I wonder if this leak is related to bug 1809000 as well - though that is macOS only, and comment 0 suggests that is happening on linux debug.

Comment 5

[:Gijs (he/him)]

I'm also confused because that ref (ie to _openPopupCancelCallback) gets nulled out here which is called here when the window closes. Does that not happen?

Comment 6

[Andreas Pehrson [:pehrsons]]

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

I wonder if this leak is related to bug 1809000 as well - though that is macOS only, and comment 0 suggests that is happening on linux debug.

Linux debug is circumstantial. It's what I run locally. The webrtc legacy indicator is disabled on mac in favor of the MacOSWebRTCStatusbarIndicator, but would happen there otherwise. It happens on Windows on try.

I guess it doesn't happen on non-debug because there is no ShutdownLeaks check.

I wonder if bug 180900 could be a result of the stray strongref in comment 1. It can be active when a native popup is showing and was added mac-specifically. I'll comment.

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

I'm also confused because that ref (ie to _openPopupCancelCallback) gets nulled out here which is called here when the window closes. Does that not happen?

We are just leaking past the ShutdownLeaks check, which happens before the main window is closed (in the callback passed to finish()). See pernosco.

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1816693 - In PanelMultiView.jsm don't tie the openPopup args to the main window lifetime through the cancelCallback capture. r?Gijs!

It appears that even a noop function () => {} will capture the entire scope,
so this patch also changes the normal state of the cancel callback to be
undefined.

Comment 8

[Andreas Pehrson [:pehrsons]]

pre-patch: https://treeherder.mozilla.org/jobs?repo=try&revision=269b40de773b2d6360be98d74aad3ad4893e8d0c
post-patch: https://treeherder.mozilla.org/jobs?repo=try&revision=06bc67b46f03ed75f90c3390551abd3a634a7233
full desktop b-c suite: https://treeherder.mozilla.org/jobs?repo=try&revision=38369b8a0d5225b6c98f02be813628ddfbb33c6c

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83e6cd1dfb3f
In PanelMultiView.jsm don't tie the openPopup args to the main window lifetime through the cancelCallback capture. r=Gijs

Comment 10

[:Gijs (he/him)]

Thanks for driving this!

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/83e6cd1dfb3f