Closed Bug 1597450 Opened 1 year ago Closed 7 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla78
Fission Milestone M6c
Tracking Status
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: djvj, Assigned: alwu)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files)

In file dom/media/AutoplayPolicy.cpp

Used only by autoplay code.

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

Three users

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.

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::dom::ApproverDocOf in dom/media/AutoplayPolicy.cpp → Audit nsIDocShellTreeItem usage in mozilla::dom::ApproverDocOf in dom/media/AutoplayPolicy.cpp

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

Flags: needinfo?(bvandyk)

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

Flags: needinfo?(bvandyk) → needinfo?(alwu)
Assignee: nobody → alwu
Flags: needinfo?(alwu)

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

For more information, please visit auto_nag documentation.

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

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.

Flags: needinfo?(nika)

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

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.

Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(nika)
Severity: normal → S4

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

Fission Milestone: M6 → M6c
Pushed by alwu@mozilla.com:
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
Blocks: 1650179
You need to log in before you can comment on or make changes to this bug.