1597451 - Audit nsIDocShellTreeItem usage in mozilla::MediaManager::IterateWindowListeners in dom/media/MediaManager.cpp

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect, P2)

Description

[Kannan Vijayan [:djvj]]

In file dom/media/MediaManager.cpp

https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/dom/media/MediaManager.cpp#3932

Iterates over child DocShell tree, and for each one with a Window, pulls out its UserMediaWindowListener.

Calls a visitor callback on every such UserMediaWindowListener.

Four users each passing their own callback logic:

• DeviceListChanged
  • Calls StopRawId() on listener with ID of device.
  • Should be invoked rarely, can be switched to using IPC to invoke on parent process.
• OnNavigation
  • Calls RemoveAll on listeners.
  • Itself invoked by Navigator::OnNavigation
  • Convert to using BrowsingContext.
  • If any children are out-of-process, collect root of each out-of-process subtree and send async-IPC to chrome to forward listener removal.
• MediaCaptureWindowState
  • Retrieves media capture state (Enabled|Disabled|Off for camera/browser/etc.), merges it across the tree, and returns it.
  • Used by WebRTC code.
  • Seems OK to use sync IPC here - ask process to collate this across windows.
  • WindowContext seems most reasonable place for this state.
• StopScreenSharing
  • Calls StopSharing() on listener.
  • Rare call.
  • Can use IPC too.

Comment 1

[Chris Peterson [:cpeterson]]

Kannan says replacing nsIDocShellTreeItem calls should block enabling Fission in Nightly (M6).

Comment 2

[Chris Peterson [:cpeterson]]

Moving to Audio/Video.

Please audit this use of the nsIDocShellTreeItem interface. With Fission enabled, Documents and nsDocShells for related frames, such as subframes and parent documents, may not be available within the current process and the corresponding nsIDocShellTreeItem methods will return null

If this code works as-is with Fission, we don't need to remove this usage of nsIDocShellTreeItem until when we remove nsIDocShellTreeItem entirely (bug 1607591) after we ship Fission MVP.

Fission documentation about replacing nsIDocShellTree Item:
https://wiki.mozilla.org/Project_Fission/DocShell_Tree_Replace

:farre's presentation with examples of replacing nsIDocShellTreeItem with BrowsingContext, WindowContext, SyncedContexts, and BrowsingContextGroup:
https://docs.google.com/presentation/d/1K4j6ngty64TZjJNS5qH-MBoOm3TI2dJedVsbH8jUhKE/edit#slide=id.g6e35225e5d_1_264

Comment 3

[Chris Peterson [:cpeterson]]

Bryce, someone on the Media team will need to triage this bug and audit this code.

Comment 4

[Chris Peterson [:cpeterson]]

Auditing whether this use of nsIDocShellTreeItem breaks when Fission is enabled blocks Fission Nightly.

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

To make sure we're on the same page, :jib, is this something that WebRTC folks can investigate?

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Yeah MediaManager is WebRTC. Thanks for the heads up.

Comment 7

[Neha Kochar [:neha]]

Randell said the functionality that this effects are:

• Stop sharing from within an iframe will not work
• UI indicator when using webRTC from an iframe will not work
• when one plugs or unplugs headphones, webRTC in an iframe will not be able to detect the change

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

-> jib since Andreas is on leave (extensive discussion on Matrix). Stop Sharing (and probably plug/unplug) can be modeled on the BrowsingContext handling of Mute. UI indicators can be modeled on the Audio In Use indicators

Comment 9

[Neha Kochar [:neha]]

Nils, FYI, this needs some attention.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I'm not back until next Monday in the end. Andreas, any chance you could take a look?

Comment 11

[Andreas Pehrson [:pehrsons]]

(In reply to Neha Kochar [:neha] from comment #7)

Randell said the functionality that this effects are:

• Stop sharing from within an iframe will not work

This seems to be handled by frontend. Stop sharing for same-process iframes might not work without this use of nsIDocShellTreeItem, and same-process nested iframes (with an oop-iframe in between) might not work with this use of nsIDocShellTreeItem, depending on if frontend handle those cases. I'll try to add a test for the nested case since it seems to be missing.

• UI indicator when using webRTC from an iframe will not work

Like the stop sharing case above.

• when one plugs or unplugs headphones, webRTC in an iframe will not be able to detect the change

IterateWindowListeners is no longer used thanks to bug 1595618. Same-process windows get notified through the singleton MediaManager. The MediaManager gets notified over IPC from the parent for both audio and video already, making separate processes work.

---

I think the only work to be done here is to swap out nsIDocShellTreeItem for a same-process equivalent using BrowsingContext. More could be done but that would rather have the goal of simplifying frontend (would it become simpler? it is another discussion to have), and that seems out of scope.

Comment 12

[Andreas Pehrson [:pehrsons]]

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

(In reply to Neha Kochar [:neha] from comment #7)

Randell said the functionality that this effects are:

• Stop sharing from within an iframe will not work

This seems to be handled by frontend. Stop sharing for same-process iframes might not work without this use of nsIDocShellTreeItem, and same-process nested iframes (with an oop-iframe in between) might not work with this use of nsIDocShellTreeItem, depending on if frontend handle those cases. I'll try to add a test for the nested case since it seems to be missing.

Those cases are indeed all handled by desktop frontend. I have a WIP test for the nested same-process case but it doesn't help me test that I don't regress the IterateWindowListeners code.

I think geckoview frontend also handles all cases of subframe interaction, even though code suggests otherwise. It requests the mediaCaptureWindowState with the includeDescendants flag set. But it also iterates over all windows in MediaManagerService.activeMediaCaptureWindows which would include any iframes. It doesn't interact with MediaManager apart from this. I'll have to test manually since there doesn't seem to exist anything in automation.

If this holds I can get rid of IterateWindowListeners completely.

Comment 13

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - Extend timeout of in-frame gum browser test. r?johannh

This is timing out locally in a non-opt debug build.

Comment 14

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - Make runTests able to track nested iframes. r?johannh

This would allow us to track tests in e.g., a same-process iframe inside an
out-of-process iframe.

Comment 15

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - Remove the almost-duplicate gum_in_oop_frame.html. r?johannh

Comment 16

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - postMessage to the correct ancestor frame in getUserMedia chrome frame tests. r?johannh

Comment 17

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - Test gUM in a same-process nested iframe behind an oop iframe. r?johannh

Comment 18

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - Remove unnecessary IterateWindowListeners. r?jib

Comment 19

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1597451 - Remove IterateWindowListeners altogether. r?jib!,r?#geckoview-reviewers!

GeckoView is seemingly the last user depending on MediaManager iterating over
a all iframes of a window when queried for its capture state.

It is fine however, since GeckoViewMedia already iterates over all windows in
MediaManagerService.activeMediaCaptureWindows, which includes all subframes that
are actively captured.

This patch removes IterateWindowListeners altogether, and the last callsite is
simplified.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a84d54b203f
Extend timeout of in-frame gum browser test. r=johannh
https://hg.mozilla.org/integration/autoland/rev/c387b653c260
Make runTests able to track nested iframes. r=johannh
https://hg.mozilla.org/integration/autoland/rev/19d25de93964
Remove the almost-duplicate gum_in_oop_frame.html. r=johannh
https://hg.mozilla.org/integration/autoland/rev/11b26d74c205
postMessage to the correct ancestor frame in getUserMedia chrome frame tests. r=johannh
https://hg.mozilla.org/integration/autoland/rev/a0e8c3269c28
Test gUM in a same-process nested iframe behind an oop iframe. r=johannh
https://hg.mozilla.org/integration/autoland/rev/ec16fb789a04
Remove unnecessary IterateWindowListeners. r=jib
https://hg.mozilla.org/integration/autoland/rev/595369a8ef29
Remove IterateWindowListeners altogether. r=jib,geckoview-reviewers,agi

Comment 21

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/9a84d54b203f
https://hg.mozilla.org/mozilla-central/rev/c387b653c260
https://hg.mozilla.org/mozilla-central/rev/19d25de93964
https://hg.mozilla.org/mozilla-central/rev/11b26d74c205
https://hg.mozilla.org/mozilla-central/rev/a0e8c3269c28
https://hg.mozilla.org/mozilla-central/rev/ec16fb789a04
https://hg.mozilla.org/mozilla-central/rev/595369a8ef29