1597450 - Audit nsIDocShellTreeItem usage in mozilla::dom::ApproverDocOf in dom/media/AutoplayPolicy.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Kannan Vijayan [:djvj]]

In file dom/media/AutoplayPolicy.cpp

Used only by autoplay code.

Returns Document of same-type root (the “approver” doc)

Three users

• IsWindowAllowedToPlay
  • Checks “IsExtensionPage” on approver.
    • IsExtensionPage accesses principal.
• IsAllowedToPlayInternal
  • Calls SiteAutoPlayPerm, which accesses principals of document, which is not fission-safe: https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/dom/media/AutoplayPolicy.cpp#69
• IsAllowedToPlay
  • Also calls SiteAutoPlayPerm on “approver” document, as with IsAllowedToPlayInternal

IPC is likely OK here. Autoplay requests are not frequent. When the same-type-root is in process, we can still use this code, and otherwise use IPC to get the information from the other document.

Change to use BrowsingContext, checking for out-of-process “approver” doc, and if so forwarding to an IPC-call to perform “IsExtensionPage” and “SiteAutoPlayPerm” instead of performing in-process.

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

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

Alastor, is this something you're able to take a look at?

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:bryce, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Alastor Wu [:alwu]]

We use the top-level document's principle to check if this page is in our white/blacklist of autoplay [1] because one page can contain multiple frames which have different principles. However, after enable Fission, the top-level document might be in other process so we can't do that synchrously because asking that attribute of top-level document in another content process is an async operation.

The reason we have to use the top-level document's principle is because, when checking if the page is in the white/blacklist of autoplay, we want to use the top-level document's origin, not the iframe's.

So I wonder if it's proper to add a sync IPC method to acquire this information from another content process. If not, then we have to propagate this information to iframe when (1) creating the iframe in another process (2) monitor the permission change if it's possible, then propagate the value to the iframe whenever the value changes.

[1] https://searchfox.org/mozilla-central/rev/2a10f4812f3f7c7645d253a4fe52f26bf58e20e8/dom/media/AutoplayPolicy.cpp#78-79

---

Hi, nika,
Would you mind give me some suggestion about this issue?
Thank you.

Comment 7

[Matt Woodrow (:mattwoodrow)]

We don't have IPDL connections from one content process to another, and we can't do sync ipc of any sort between them.

We also don't really want to expose the principal of ancestor documents to content process for subdocs.

Can we add a synchronized field on WindowContext [1] that stores a boolean of whether that WindowContext has permission to do autoplay?

I think we could compute that when running WindowGlobalParent::Init and set it, then it'll be available to all processes.

Subdoc content processes can do BrowsingContext::GetTopWindowContext to access it.

[1] https://searchfox.org/mozilla-central/source/docshell/base/WindowContext.h#21

Comment 8

[Alastor Wu [:alwu]]

Hi, Matt,

Because I'm not familiar with when those things would be created and initiated. At the time of calling WindowGlobalParent::Init(), have we already known the URL of the document we are going to load and created the corresponding principle?

In addition, as I am not sure I fully understand the mechanism of synchronising the fields between different window contexts. If I have a page containing an iframe, when I create that iframe, would the field of the parent's window context be automatically synced to the child's window context? And when the iframe replaces its document, would the window context for the new created document also have same value of that field?

Thank you.

Comment 9

[Matt Woodrow (:mattwoodrow)]

(In reply to Alastor Wu [:alwu] from comment #8)

Hi, Matt,

Because I'm not familiar with when those things would be created and initiated. At the time of calling WindowGlobalParent::Init(), have we already known the URL of the document we are going to load and created the corresponding principle?

Yes, the WindowGlobalParent constructor copies the principal from WindowGlobalInit into WindowGlobalParent::mDocumentPrincipal.

In addition, as I am not sure I fully understand the mechanism of synchronising the fields between different window contexts. If I have a page containing an iframe, when I create that iframe, would the field of the parent's window context be automatically synced to the child's window context? And when the iframe replaces its document, would the window context for the new created document also have same value of that field?

WindowGlobalParent is-a WindowContext, and we create synchronized WindowContext instances in all processes that might have a reference to that window.

When we start loading an iframe (in any process), we can guarantee that we already have WindowContext instances for all ancestor Documents available in that process.

So if you take the BrowsingContext for the iframe, and then look at BrowsingContext::GetTopWindowContext, which will be the WindowContext for the top-most ancestor Document, and will have the values set in WindowGlobalParent when that ancestor Document was created.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

It seems like :mattwoodrow is giving feedback, so clearing my ni? :-)

Comment 11

[Alastor Wu [:alwu]]

Attachment: Bug 1597450 - part1 : store autoplay permission on the WindowContext.

Comment 12

[Alastor Wu [:alwu]]

Attachment: Bug 1597450 - part2 : check autoplay permission via the top-level window context.

Comment 13

[Alastor Wu [:alwu]]

Attachment: Bug 1597450 - part3 : add test.

Comment 14

[Chris Peterson [:cpeterson]]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36fb11829200
part1 : store autoplay permission on the WindowContext. r=nika
https://hg.mozilla.org/integration/autoland/rev/d504c1d80a91
part2 : check autoplay permission via the top-level window context. r=bryce
https://hg.mozilla.org/integration/autoland/rev/8c0e44fad2b0
part3 : add test. r=bryce

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d22268a63da7
Fix trailing whitespace. r=fix

Comment 17

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/36fb11829200
https://hg.mozilla.org/mozilla-central/rev/d504c1d80a91
https://hg.mozilla.org/mozilla-central/rev/8c0e44fad2b0
https://hg.mozilla.org/mozilla-central/rev/d22268a63da7