Closed Bug 1597451 Opened 1 year ago Closed 1 month ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
84 Branch
Fission Milestone M6c
Tracking Status
firefox84 --- fixed

People

(Reporter: djvj, Assigned: pehrsons)

References

(Blocks 1 open bug)

Details

(Whiteboard: [rm-docshell-tree-item:hard])

Attachments

(7 files)

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.

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

Fission Milestone: --- → M6
Priority: -- → P3

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

Component: DOM: Navigation → Audio/Video
Priority: P3 → --
Summary: Fix uses of mozilla::MediaManager::IterateWindowListeners in dom/media/MediaManager.cpp → Audit nsIDocShellTreeItem usage in mozilla::MediaManager::IterateWindowListeners in dom/media/MediaManager.cpp

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

Flags: needinfo?(bvandyk)
Priority: -- → P2

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

Fission Milestone: M6 → M6c

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

Flags: needinfo?(bvandyk) → needinfo?(jib)

Yeah MediaManager is WebRTC. Thanks for the heads up.

Component: Audio/Video → WebRTC: Audio/Video
Flags: needinfo?(jib)

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

-> 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

Assignee: nobody → jib

Nils, FYI, this needs some attention.

Flags: needinfo?(drno)
Flags: needinfo?(jib)

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

Flags: needinfo?(jib) → needinfo?(apehrson)
Assignee: jib → apehrson
Status: NEW → ASSIGNED
Flags: needinfo?(drno)
Flags: needinfo?(apehrson)

(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.

Depends on: 1595618

(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.

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

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

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.

Pushed by pehrsons@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.