1675360 - mediaKeySystemAccess.createMediaKeys() fails for dynamically generated iframes

Status: RESOLVED FIXED

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

Description

[Selcuk]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/86.0.4240.183 Safari/537.36

Steps to reproduce:

1. Create an iframe with src set to javacript:false
2. Populate content of iframe with script that calls createMediaKeys

Actual results:

createMediaKeys fails with DOMException: Couldn't get channel in MediaKeys::Init

(The following change may have caused this regression:
https://github.com/mozilla/gecko-dev/commit/1a16e898b29ff439b3e82d6d74dc8fbc6a221666#diff-e5399c60b5868e1812563c18c4197628051fb710b0bd8d5a279a7eb86d922d55)

Expected results:

createMediaKeys() should succeed.

Comment 1

[Alastor Wu [:alwu]]

Hi, bryce, could you take a look and assign the priority for this issue?
Thank you.

Comment 2

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

I can repro this and it looks like a regression from the fission handling changes. We use the channel on a document to grab principals, but my educated guess is that we don't have a channel for iframes lacking sources so we fall over.

Selcuk, is this something that is causing issues in the wild?

Comment 3

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

Testcase: https://codepen.io/SingingTree/pen/bGeQwzx?editors=1010

Comment 4

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

:kmag, is there any existing machinery we can use to work around this?

Comment 5

[Selcuk]

(In reply to Bryce Seager van Dyk (:bryce) from comment #2)

I can repro this and it looks like a regression from the fission handling changes. We use the channel on a document to grab principals, but my educated guess is that we don't have a channel for iframes lacking sources so we fall over.

Selcuk, is this something that is causing issues in the wild?

Yes, this is my observation as well.

Comment 6

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

(In reply to Selcuk from comment #5)

(In reply to Bryce Seager van Dyk (:bryce) from comment #2)

I can repro this and it looks like a regression from the fission handling changes. We use the channel on a document to grab principals, but my educated guess is that we don't have a channel for iframes lacking sources so we fall over.

Selcuk, is this something that is causing issues in the wild?

Yes, this is my observation as well.

Are you able to say where the issues in the wild are taking place?

Comment 7

[Selcuk]

The sample on codepen.io is an accurate representation of how we reproduced the issue; createMediaKeys fails for iframe without source while it works for iframe with a src URL.

Comment 8

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

Understood, but is this a problem that was noticed because it took place in a production website?

Comment 9

[Selcuk]

That's correct. We have noticed this issue on a client's DRM enabled video portal on production (requires authentication; can't share here). We have temporarily remedied the issue by converting dynamic iframe based workflow to shadow DOM using an existing flag on our side.

Comment 10

[Selcuk]

Looking at the code I wonder if it is possible to use parent document's channel as a fallback. Such iframes do not even require feature policy for encrypted-media (regarded as same domain).

Also is it possible to include the fix in the next Firefox release? (Multiple clients reported this issue; we are enhancing our workarounds for them. Static iframe (with a cross-domain src) workaround brings a burden on them to set Feature-Policy response headers.)

Comment 11

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

(In reply to Selcuk from comment #10)

Looking at the code I wonder if it is possible to use parent document's channel as a fallback. Such iframes do not even require feature policy for encrypted-media (regarded as same domain).

Also is it possible to include the fix in the next Firefox release? (Multiple clients reported this issue; we are enhancing our workarounds for them. Static iframe (with a cross-domain src) workaround brings a burden on them to set Feature-Policy response headers.)

I'm planning on investigating something like what you suggest. I've got some work in progress, but have some other work superseding it. I hope to be able to return to it by early next week. If you could email me to discuss in more detail the sites that are having problems that would be useful to make a case for uplifting the fix to release once it lands.

Comment 12

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

It looks like this is mitigated if we wait for the load on the load of the iframe https://codepen.io/SingingTree/pen/XWKoJKo?editors=1011

Looking further at what's going on I was incorrect above -- we will set the channel and load info with a blank src, but because of bug 543435 we actually do another load. This also means the mitigation mentioned in comment 10 and 11 will work initially, but you'll run into problems later because your media keys object will be associated with a document that disappears due to our async load.

Selcuk, can you confirm this is mitigated by waiting on the load event?

Comment 13

[Selcuk]

I gave a try, here is my observation:

Experiment #1:
after load event, injecting script (inline or with src) into the iframe makes the iframe's location.href about:blank. Possibly this causes DOMException: Key system is unsupported for even clearkey and widevine. Looks to be an expected behavior since chrome does the same.

Experiment #2:
after load event, injected script content using document.open -> write -> close sequence. This sets iframe's location to the parent window location. This works initially (4-5 seconds of playback) and then video element gets stuck in waiting state. In comparison, Chrome plays w/o getting stuck.

Might be irrelevant but firefox also does not seem to execute immediate async function calls in the injected script in either experiment while chrome executes as expected:

//inject a script
const newScript = iframe.contentDocument.createElement('script');
newScript.src = 'test.js';
frame.contentDocument.body.appendChild(newScript);

//test.js content:

console.log('this gets printed');
setTimeout(main, 100);
function main() {
console.log('this does not get called.');
}

Comment 14

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

If you can attach those test cases or link to them hosted somewhere, that would be useful. I've tried to repro the injected script logging from a timeout and that seems to work as expected in Fx and Chrome (everything gets printed).

injectScriptInIframe.html -

<html>
<head>
<script>
async function doOnLoad() {
    let div = document.getElementById("myDiv");
    let iframe = document.createElement("iframe");
    let p = new Promise(r => {
      iframe.onload = () => {
        console.log("iframe loaded");
        r();
      };
    });
    iframe.src = "";
    div.appendChild(iframe);
    await p;

    const newScript = iframe.contentDocument.createElement("script");
    newScript.src = "timeoutLog.js";
    iframe.contentDocument.body.appendChild(newScript);
}
window.onload = doOnLoad;
</script>
<body>
    <div id="myDiv"></div>
</body>
</head>
</html>

timeoutLog.js -

console.log('this gets printed');
setTimeout(callback, 100);
function callback() {
    console.log('this does not get called.');
}

---

I have another plan to see if we can mitigate this, will report back once I've tested it.

Comment 15

[Selcuk]

I will try to isolate that async method execution issue and provide a demo, obviously there are some contributing factors in my trials. Regardless, it might be irrelevant to the additional load event for iframe; I can report that with a separate ticket once isolated demo is ready on my end.

Comment 16

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

Attachment: Bug 1675360 - Add test coverage for createMediaKeys in iframes.

Provide coverage the ensures we can:

• Call navigator.requestMediaKeySystemAccess() and receive access
• Call createMediaKeys on the access object
  in iframes that are same and different origin.

This should work when waiting for an iframe to fire the load event, but I also
provide a case for if we do not wait for load. It's undesirable to not wait for
the load, but we've historically worked in this case (if this was intentional is
not clear to me). So providing such a test allows for coverage of this case as
long as we want to continue supporting it. Said test will be red as of this
patch, but an immediate follow up will restore our compat with this case.

Comment 17

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

Attachment: Bug 1675360 - Have MediaKeys::Init search for load info on the parent if the current doc doesn't have any.

This restores some functionality to work more closely to how we'd done so in a
pre-fission world. Previously we'd associated MediaKeys we created with the top
level in process window's doc and used that document's principal as the top level
principal for the keys.

To make this more fission compatible, this was changed so the keys are
associated with the document they're created in, as well as using that
document's channel to determine the top level principal via the load info.
However, this caused us to regress in the case where MediaKeys are created
within an iframe that hasn't yet fired the load event.

To prevent that regression, we move back to associating the keys with a document
further up the hierarchy -- we use the top in process window's doc again.
Because this document may not actually be the top level doc in fission, we use
that document's load info to try and determine the top level principal.

Depends on D97321

Comment 18

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

Try run for the above: https://treeherder.mozilla.org/jobs?repo=try&revision=af2692a076c9558da0018ae66d281795c83da12e

Comment 19

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

Attachment: Bug 1675360 - Shutdown MediaKeys when parent inner window is destroyed rather than document.

Rework the MediaKeys class to shutdown when its parent inner window is destroyed
rather than the document it's created in. This is done to mitigate the case
where a MediaKeys is created in an about:blank document that has not yet
undergone its async load (i.e. blank document that will stay blank, blank
documents loading to other pages will still clobber their keys on load). This
specifically addresses cases where sites could create an iframe, not wait for
load, create a MediaKeys in the iframe, and then find the keys had become
unusable.

This is achieved by associating MediaKeys instances with their inner window and
having the window notify keys when a window is going to be destroyed. I decided
to use this approach rather than have MediaKeys observe for window destruction
for a few reasons:

• The keys would need to support weak references to use the observer service in
  the desired way. Implementing this interface on the MediaKeys adds a
  non-trivial level of complexity and means the keys would implement the WeakPtr
  interface (already in place prior to this patch) and also the NS weak
  reference interface, which I found confusing.
• If the inner window stores pointers to MediaKeys created in it, it can be self
  aware of if EME activity is taking place within it. The allows us to better
  identify EME activity in documents. Historically one of the ways we'd
  determined EME activity by checking if media elements have MediaKeys attached,
  but this had lead to issues where if MediaKeys are created but not attached
  to a media element we overlook them. With this patch's changes, we can also
  have a document check its inner window to see if there are any MediaKeys. This
  patch uses this to extend our check to avoid bfcaching pages with EME content.
• There appears to be prior art using a similar approach for audio contexts and
  peer connections, which I assume is sensible and I'm not reinventing the wheel
  by following.

Comment 20

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

Try push with (hopefully) close to final patches https://treeherder.mozilla.org/jobs?repo=try&revision=449a075f9790d2a8f0b5b510efd0eabac0e0b06f&selectedTaskRun=ba3WsGoUQ5GhJWGvYZjTtQ.0

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b18095c8f8a
Add test coverage for createMediaKeys in iframes. r=karlt
https://hg.mozilla.org/integration/autoland/rev/cd8c20e521f5
Have MediaKeys::Init search for load info on the parent if the current doc doesn't have any. r=karlt
https://hg.mozilla.org/integration/autoland/rev/7b5facb4df3a
Shutdown MediaKeys when parent inner window is destroyed rather than document. r=karlt

Comment 22

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/2b18095c8f8a
https://hg.mozilla.org/mozilla-central/rev/cd8c20e521f5
https://hg.mozilla.org/mozilla-central/rev/7b5facb4df3a

Comment 23

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

Follow ups

• bug 1681544 tracks adding a test to ensure MediaKeys are destroyed when we navigate away from a page.
• bug 1681741 tracks annotating CDMProxy::Shutdown with MOZ_CAN_RUN_SCRIPT.