1692385 - GetSinkDevice() leaks SourceListeners until the window is navigated

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, enhancement)

Description

[Karl Tomlinson (:karlt)]

GetSinkDevice() registers a new SourceListener, and discards its handle to the SourceListener.

GetUserMediaWindowListener holds a strong reference to the SourceListener and MediaManager holds a strong reference to the GetUserMediaWindowListener until the window is navigated.

Comment 1

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/jobs?repo=try&revision=5473e546b62d23cfa70594d4a656f97c6b6a49d4

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 remove unnecessary separate RemoveWindowID() path r?pehrsons

If GetInnerWindowWithId() returns false then RemoveWindowID() does nothing
but remove the window id from mActiveWindows if present.
https://searchfox.org/mozilla-central/rev/d3343662ce0aced933b30e053b33c93f759292eb/dom/media/MediaManager.cpp#3351,3357
GetUserMediaWindowListener::RemoveAll() is sufficient to call RemoveWindowID()
to do the same if the window id is present in mActiveWindows.
If the window id is not present, then GetWindowListener() returns false and
there is nothing to do.

The additional code path was added because IterateWindowListeners() needed a
nsPIDOMWindow,
https://hg.mozilla.org/mozilla-central/rev/786737c759d771029d393d793dff207d86f18faf#l1.60
but IterateWindowListeners() is no longer used.
https://hg.mozilla.org/mozilla-central/rev/ec16fb789a045e6d644d32bf269ff48944637c2b#l1.21

The "safe" comment is not necessary because mActiveWindows is clearly
documented as main-thread-only and there are plenty of main thread assertions.
https://searchfox.org/mozilla-central/rev/d3343662ce0aced933b30e053b33c93f759292eb/dom/media/MediaManager.h#332-333

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 Remove unnecessary extra active window check in EnumerateDevices() r?pehrsons

The only caller of MediaManager::EnumerateDevices() checks that the inner
window is [current] after the returned MozPromise settles and
Document::CanSavePresentation() says [no] if
MediaManager::Get()->IsWindowStillActive(win->WindowID()) so there should be
no observable difference in depending on MediaDevices::EnumerateDevices() for
the active window check.

[current]
https://searchfox.org/mozilla-central/rev/4dac9993b609fccc87e82682614faf2a44cda306/dom/media/MediaDevices.cpp#121,149
[no]
https://searchfox.org/mozilla-central/rev/d3343662ce0aced933b30e053b33c93f759292eb/dom/base/Document.cpp#10738

Depends on D105247

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 Let SourceListeners identify the GetUserMediaWindowListener from which to remove themselves r?pehrsons

This was already happening to some extent due to the recursive relationship of
SourceListener::Stop() and GetUserMediaWindowListener::Remove(), but being
direct about it saves a parameter.

Depends on D105248

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 Add and use a helper method to create a GetUserMediaWindowListener on demand r?pehrsons

Depends on D105249

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 Remove repetition of IsWindowStillActive() test in same task r?jib

The task is main-thread, so the second IsWindowStillActive() call always
returned true.

This became obsolete with
https://hg.mozilla.org/mozilla-central/rev/69b1d1c5e010097cfd7c10c9a373f72ea6873e2c#l1.587

Depends on D105250

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 move SourceListener logic needed for testing window existence into EnumerateDevicesImpl() r?jib,pehrsons

so that EnumerateDevicesImpl() clients don't need to care about this.

This also adds a SourceListener::Stop() call to fix a leak from the logic in
GetSinkDevice().

Depends on D105251

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72d38fc9b2bb
remove unnecessary separate RemoveWindowID() path r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/fff6d438adb9
Remove unnecessary extra active window check in EnumerateDevices() r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/861447a14631
Let SourceListeners identify the GetUserMediaWindowListener from which to remove themselves r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/5dda16539d3d
Add and use a helper method to create a GetUserMediaWindowListener on demand r=pehrsons
https://hg.mozilla.org/integration/autoland/rev/a4d8f07a3e41
Remove repetition of IsWindowStillActive() test in same task r=pehrsons

Comment 9

[Karl Tomlinson (:karlt)]

Bug 1693724 may be useful for testing this.
See e.g. https://searchfox.org/mozilla-central/source/dom/media/webaudio/test/test_WebAudioMemoryReporting.html

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f2cb3ed27e68
move SourceListener logic needed for testing window existence into EnumerateDevicesImpl() r=pehrsons

Comment 11

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/72d38fc9b2bb
https://hg.mozilla.org/mozilla-central/rev/fff6d438adb9
https://hg.mozilla.org/mozilla-central/rev/861447a14631
https://hg.mozilla.org/mozilla-central/rev/5dda16539d3d
https://hg.mozilla.org/mozilla-central/rev/a4d8f07a3e41
https://hg.mozilla.org/mozilla-central/rev/f2cb3ed27e68

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: Bug 1692385 add some memory usage testing for setSinkId() r?pehrsons

Depends on D106250

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9eb5a35d5450
add some memory usage testing for setSinkId() r=pehrsons

Comment 14

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/9eb5a35d5450