1646553 - Figure out if GetInProcessTop usage in MediaKeys::Init is OK

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, task, P2)

Description

[Kris Maglione [:kmag]]

This method is not Fission-compatible, and will not handle windows with OOP ancestors correctly, but that may not matter for MediaKeys' usage.

Comment 1

[Chris Peterson [:cpeterson]]

EME might be broken in OOP iframes. Tracking for Fission M6c Nightly because this is probably a rare use of EME.

Comment 2

[Chris Peterson [:cpeterson]]

Correction: Tracking all "Figure out GetInProcessTop usage" bugs for Fission M6b.

Comment 3

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

I'm going to be out for the next ~2 weeks, but as this is EME related it looks like something for me. NI on me so this gets picked up when I'm back.

Comment 4

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

I've been looking at this, thoughts:

• The current GetProcessInTop[0] call is used to get the top level window. With this window we do 2 related things
  1. We get the top level document[1]. We observe activity on this document to shutdown the media keys if the document becomes inactive[2].
  2. We get the principal of the document and top level document[3] which is used in our privacy strategy for EME.
• If we keep the code as is both of these will behave differently under fission for cross origin iframes. I.e. we will listen observe activity on a non-top-level doc and get a non-top level origin.
• The fix I'm proposing and working on for each case.
  1. We observe activity on the document the media keys are in to see if we should shut them down (rather than the top level doc). If the keys are associated with a top level document they will behave the same, but if they're in an iframe they will behave somewhat differently -- they will be shutdown when that iframe becomes inactive. This seems sensible to me, and also strikes me as a fairly niche case.
  2. We continue to use the top level principal as it's important to how we key GMPs. However, instead of obtaining it from the top level document (which we can't easily get a ref to in a cross origin context) we obtain it via current doc -> channel -> LoadInfo.

[0] https://searchfox.org/mozilla-central/rev/d54210d490ef335b13fc1fcac817525120c8c46b/dom/media/eme/MediaKeys.cpp#419
[1] https://searchfox.org/mozilla-central/rev/d54210d490ef335b13fc1fcac817525120c8c46b/dom/media/eme/MediaKeys.cpp#427
[2] https://searchfox.org/mozilla-central/rev/d54210d490ef335b13fc1fcac817525120c8c46b/dom/media/eme/MediaKeys.cpp#90-114
[3] https://searchfox.org/mozilla-central/rev/d54210d490ef335b13fc1fcac817525120c8c46b/dom/media/eme/MediaKeys.cpp#429

Comment 5

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

Attachment: Bug 1646553 - Rework MediaKeys to be fission friendly. r?alwu,kmag

This patch reworks how the MediaKeys does 2 things:

1. Listens for documents becoming inactive. Mediakeys will no longer listen
   on the top document in the process, and will now instead listen to their own
   document.
2. Obtains the top level principal used to create the media keys. Instead of
   grabbing the principal from the top document, the MediaKeys will now use their
   document's channel's LoadInfo and query the principal from that.

1 will change how the keys behave in iframes -- keys will now shutdown if their
iframe doc becomes inactive, rather than the top in process document. This is
likely to only matter in very niche cases as EME is almost always used in a top
level context.

2 seeks to retain the previous behaviour, but does so in a fission compatible
way.

Comment 6

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

Attachment: Bug 1646553 - Don't pass the main thread as an argument when creating CDM proxies. r?alwu

CDMProxy stores a reference to the main thread. We'd been passing this in as an
argument. This patch changes the proxy to just fetch the main thread upon
construction. This avoids passing arguments around and also removes the risk of
passing a non-main thread arg.

Depends on D87788

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7817bbb27673
Rework MediaKeys to be fission friendly. r=kmag
https://hg.mozilla.org/integration/autoland/rev/ae6d1fe93da0
Don't pass the main thread as an argument when creating CDM proxies. r=alwu

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/7817bbb27673
https://hg.mozilla.org/mozilla-central/rev/ae6d1fe93da0