Closed Bug 1264595 Opened 6 years ago Closed 6 years ago

test isolation by mediaSource URI by first party domain (Tor 15703)

Categories

(Core :: DOM: Security, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: allstars.chh, Assigned: jhao)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1])

Attachments

(3 files, 1 obsolete file)

We need to have some tests for verifying isolation by mediaSource URI by first party domain.

TOR bug: 15703.
Whiteboard: [tor][OA]
Whiteboard: [tor][OA] → [tor][OA][domsecurity-backlog]
Here's a link that tracks the Tor Browser regression test patch for media source isolation:
https://torpat.ch/15703
Summary: isolation by mediaSource URI by first party domain → isolation by mediaSource URI by first party domain (Tor 15703)
Whiteboard: [tor][OA][domsecurity-backlog] → [tor][OA-testing][domsecurity-backlog]
Summary: isolation by mediaSource URI by first party domain (Tor 15703) → test isolation by mediaSource URI by first party domain (Tor 15703)
Attachment #8746845 - Attachment description: mediasourcetest.patch → WIP patch to add tests from Tor Browser.
Attachment #8746845 - Attachment filename: mediasourcetest.patch → WIP patch from the Tor Browser
Duplicate of this bug: 1264598
Whiteboard: [tor][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog]
Priority: -- → P1
Depends on: 1289319
Priority: P1 → P3
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog] → [tor-testing][OA-testing][domsecurity-backlog1]
Priority: P3 → P1
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1] → [tor-testing][OA-testing][domsecurity-backlog1][ETA 10/10]
Priority: P1 → P2
Assignee: nobody → jhao
Status: NEW → ASSIGNED
Attached patch Test mediaSource URL isolation. (obsolete) — Splinter Review
WIP patch.  I don't know why I always get video element error.
I can't make the original Tor's test work in no-isolation mode either.

It turns out the media source object is auto revoked when the event loop reaches a "stable state" according to bug 1116382.  The code is here: http://searchfox.org/mozilla-central/rev/064025c802c22cd5ad122746733cbd34ea47393c/dom/url/URL.cpp#279

That's why by the time I pass that url to the other tab, it's already revoked.

I don't think we can test this anymore.  Arthur, do you agree?
Flags: needinfo?(arthuredelstein)
According to https://developer.mozilla.org/zh-TW/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIAppShell#runInStableState()

The runnable will be executed once current event finishes.  I don't know if we can force the first tab to be stuck in the same event.
(In reply to Jonathan Hao [:jhao] from comment #5)
> I can't make the original Tor's test work in no-isolation mode either.
> 
> It turns out the media source object is auto revoked when the event loop
> reaches a "stable state" according to bug 1116382.  The code is here:
> http://searchfox.org/mozilla-central/rev/
> 064025c802c22cd5ad122746733cbd34ea47393c/dom/url/URL.cpp#279
> 
> That's why by the time I pass that url to the other tab, it's already
> revoked.
> 
> I don't think we can test this anymore.  Arthur, do you agree?

Yes, I think you are right. This behavior was introduced in bug 1116382 and was designed to follow the W3C recommendation to autorevoke mediasource URIs. Recently, however, the media-source draft spec has been changed to recommend manual revocation instead, to mirror the File API behavior:
Old: https://www.w3.org/TR/2014/CR-media-source-20140717/#widl-URL-createObjectURL-DOMString-MediaSource-mediaSource
New: https://www.w3.org/TR/2016/CR-media-source-20160705/#widl-URL-createObjectURL-DOMString-MediaSource-mediaSource
Here is the committee's discussion on this change:
https://github.com/w3c/media-source/issues/10

As a manual test of the autorevocation behavior, I forked the example here:
https://developer.mozilla.org/en-US/docs/Web/API/MediaSource#Examples
Here is my fork:
https://arthuredelstein.github.io/netfix/demo/bufferAll.html
And here is what I changed:
https://github.com/arthuredelstein/netfix/commit/5d015a4ccd7e1b1dfab38c8e233a569b2a8931a7

The video fails on Firefox because the mediaSource URI has been revoked by the time the function passed to setTimeout runs. It succeeds on Google Chrome.

I think we should leave this bug open given that it seems somewhat likely that autorevocation will be removed, to reflect the new recommendations in the W3C spec. Karl, do you have an opinion on this?
Flags: needinfo?(arthuredelstein) → needinfo?(karlt)
(In reply to Arthur Edelstein [:arthuredelstein] from comment #7)
Yes, and thanks for the links.

I don't think autorevocation should be removed and filed
https://github.com/w3c/media-source/issues/156
to revert that change.

I expect async messages will be processed after the auto-revocation happens (in a single content process world), unless we have some bugs with nested event loops.
I don't know, but I wonder whether there is a way for one page to trigger synchronous execution of JS in an iframe.
Flags: needinfo?(karlt)
Thanks to Arthur and Karl.  I'll put this bug into backlog.
Priority: P2 → P3
Whiteboard: [tor-testing][OA-testing][domsecurity-backlog1][ETA 10/10] → [tor-testing][OA-testing][domsecurity-backlog1]
Status: ASSIGNED → NEW
Assignee: jhao → nobody
This bug keeps showing in bug tracking pages, but it's kind of annoying that we can't do anything right now.  I propose that we wrote a test to check that mediaSource URL is indeed revoked in the next event, and close this bug.  If the auto-revokation is removed in the future, that test will break and we'll know that it's time to do the isolation test.
Assignee: nobody → jhao
Status: NEW → ASSIGNED
Priority: P3 → P1
Priority: P3 → P1
This test is to track the auto-revocation behavior, so that if auto-revocation is removed, we should be noticed.

Karl, may I ask you to review this patch?
Attachment #8799645 - Flags: review?(karlt)
Hi Arthur and Baku, do you think we should land this test even though we have to skip it?
Attachment #8799658 - Flags: review?(arthuredelstein)
Attachment #8799658 - Flags: review?(amarchesini)
Attachment #8786290 - Attachment is obsolete: true
Comment on attachment 8799658 [details] [diff] [review]
Test mediaSource URL isolation, adapted from Tor Browser patch 15703.

Review of attachment 8799658 [details] [diff] [review]:
-----------------------------------------------------------------

no, we should have a valid test.

::: browser/components/originattributes/test/browser/browser_mediaSourceURLIsolation.js
@@ +52,5 @@
> +  return result;
> +}
> +
> +IsolationTestTools.runTests(TEST_PAGE, doTest);
> +

extra line
Attachment #8799658 - Flags: review?(amarchesini) → review-
Attachment #8799658 - Flags: review?(arthuredelstein)
Hi, Jonathan. One possibility that occurs to me would be to introduce some code that experiments at runtime to see if autorevocation is in effect. If the experiment determines that autorevocation is not happening, then the code would proceed with isolation tests. That way, in the future, if Firefox is patched to use manual revocation again, then the isolation tests will be already present.
That's an interesting idea.  So you mean we write a test that checks the auto-revocation behavior first.  If it's still there, then we do nothing.  Otherwise we run our isolation test.

Baku, do you think this is valid?
Flags: needinfo?(amarchesini)
Comment on attachment 8799645 [details] [diff] [review]
Test whether auto-revocation is removed.

This is also tested by https://hg.mozilla.org/mozilla-central/rev/d71b2d0ce994 but having a Mozilla-specific tests with a Mozilla-specific message seems reasonable.

There was also talk of removing stalled events with MSE, but that is only in the failure path, which should not be reached, so not a problem.
Attachment #8799645 - Flags: review?(karlt) → review+
Let's land the auto-revocation test only.
Flags: needinfo?(amarchesini)
Pushed by jhao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/336d0983b749
Test whether auto-revocation is removed. r=karlt
https://hg.mozilla.org/mozilla-central/rev/336d0983b749
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.