Reland: MSE-in-Workers: srcObject part 5: Conditionally fail worker o…

…bjectURL With the underlying leak fixed by https://chromium-review.googlesource.com/c/chromium/src/+/3704191, this change can be relanded now. Previous revert: > Revert "MSE-in-Workers: srcObject part 5: Conditionally fail worker objectURL" > > This reverts commit 6315549b8c2ece3dbbf3062c1a87347589a5e115. > > Reason for revert: This is causing failures on the WebKit Linux Leak builder > i.e. https://ci.chromium.org/ui/p/chromium/builders/ci/WebKit%20Linux%20Leak/39394/overview > > Original change's description: > > MSE-in-Workers: srcObject part 5: Conditionally fail worker objectURL > > > > If the MediaSourceInWorkersUsingHandle feature is enabled, this change > > prevents successful ability of obtaining an objectURL that would succeed > > in loading a worker-owned MediaSource. > > > > It changes the wpt tests to use handle for attachment and verifies > > expected new behavior of getHandle and that worker objectURL attachment > > fails (these tests run on experimental builds of Chromium with > > currently-experimental MediaSourceInWorkersUsingHandle feature enabled, > > just like the currently-experimental MediaSourceInWorkers feature.) > > > > References: > > Full prototype CL for the parts 1-4 that have already landed: > > https://chromium-review.googlesource.com/c/chromium/src/+/3515334 > > MSE spec issue: > > w3c/media-source#175 > > MSE spec feature updates switching from worker MSE attachment via > > object URL to attachment via srcObject MediaSourceHandle: > > * w3c/media-source#305 > > * further clarifications in discussion at > > w3c/media-source#306 (comment) > > > > BUG=878133 > > > > Change-Id: I60ddca79ee0f95c87b6d84e4f44ad9c283f359a3 > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3698231 > > Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> > > Auto-Submit: Matthew Wolenetz <wolenetz@chromium.org> > > Reviewed-by: Will Cassella <cassew@chromium.org> > > Cr-Commit-Position: refs/heads/main@{#1012712} > > Bug: 878133 > Change-Id: I1e405ae1de146d1f3183592b00a43bd3c38d849d > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3695890 > Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> > Commit-Queue: Nidhi Jaju <nidhijaju@chromium.org> > Owners-Override: Nidhi Jaju <nidhijaju@chromium.org> > Cr-Commit-Position: refs/heads/main@{#1012823} Bug: 878133 Change-Id: I56e4ecd4d8b58d9d58ed3c575b0fb52f596b6fae Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708465 Reviewed-by: Will Cassella <cassew@chromium.org> Commit-Queue: Matthew Wolenetz <wolenetz@chromium.org> Cr-Commit-Position: refs/heads/main@{#1014755}
web-platform-tests · Jun 16, 2022 · 8e5b88e · 8e5b88e
1 parent 3f4defa
commit 8e5b88e
Show file tree

Hide file tree

Showing 14 changed files with 146 additions and 48 deletions.
diff --git a/media-source/dedicated-worker/mediasource-message-util.js b/media-source/dedicated-worker/mediasource-message-util.js
@@ -4,6 +4,7 @@
 const messageSubject = {
   ERROR: "error",  // info field may contain more detail
   OBJECT_URL: "object url", // info field contains object URL
+  HANDLE: "handle", // info field contains the MediaSourceHandle
   STARTED_BUFFERING: "started buffering",
   FINISHED_BUFFERING: "finished buffering",
   VERIFY_DURATION: "verify duration", // info field contains expected duration

diff --git a/media-source/dedicated-worker/mediasource-worker-detach-element.html b/media-source/dedicated-worker/mediasource-worker-detach-element.html
@@ -7,11 +7,11 @@
 <body>
 <script>
 
-const AFTER_SETTING_SRC = "after setting src";
+const AFTER_SETTING_SRCOBJECT = "after setting srcObject";
 const AFTER_STARTED_BUFFERING = "after receiving Started Buffering message from worker";
 const AFTER_FINISHED_BUFFERING = "after receiving Finished Buffering message from worker";
 
-[ AFTER_SETTING_SRC, AFTER_STARTED_BUFFERING, AFTER_FINISHED_BUFFERING ].forEach(when => {
+[ AFTER_SETTING_SRCOBJECT, AFTER_STARTED_BUFFERING, AFTER_FINISHED_BUFFERING ].forEach(when => {
   for (let timeouts = 0; timeouts < 5; ++timeouts) {
     async_test(test => { startWorkerAndDetachElement(test, when, timeouts); },
         "Test element detachment from worker MediaSource after at least " + timeouts +
@@ -22,9 +22,8 @@
 function detachElementAfterMultipleSetTimeouts(test, element, timeouts_remaining) {
   if (timeouts_remaining <= 0) {
     // While not the best way to detach, this triggers interoperable logic that
-    // includes detachment, though also begins a failed load(). We don't handle
-    // video error event, so the latter is not an issue in this test.
-    element.src='';
+    // includes detachment.
+    element.srcObject = null;
     test.step_timeout(() => { test.done(); }, 10);
   } else {
     test.step_timeout(() => {
@@ -51,11 +50,10 @@
       case messageSubject.ERROR:
         assert_unreached("Worker error: " + e.data.info);
         break;
-      case messageSubject.OBJECT_URL:
-        const url = e.data.info;
-        assert_true(url.match(/^blob:.+/) != null);
-        video.src = url;
-        if (when_to_start_timeouts == AFTER_SETTING_SRC) {
+      case messageSubject.HANDLE:
+        const handle = e.data.info;
+        video.srcObject = handle;
+        if (when_to_start_timeouts == AFTER_SETTING_SRCOBJECT) {
           detachElementAfterMultipleSetTimeouts(test, video, timeouts_to_await);
         }
         break;

diff --git a/media-source/dedicated-worker/mediasource-worker-detach-element.js b/media-source/dedicated-worker/mediasource-worker-detach-element.js
@@ -14,7 +14,6 @@ let util = new MediaSourceWorkerUtil();
 let sentStartedBufferingMessage = false;
 
 util.mediaSource.addEventListener("sourceopen", () => {
-  URL.revokeObjectURL(util.mediaSourceObjectUrl);
   let sourceBuffer;
   try {
     sourceBuffer = util.mediaSource.addSourceBuffer(util.mediaMetadata.type);
@@ -33,7 +32,7 @@ util.mediaSource.addEventListener("sourceopen", () => {
                              err => { postMessage({ subject: messageSubject.ERROR, info: err }) } );
 }, { once : true });
 
-postMessage({ subject: messageSubject.OBJECT_URL, info: util.mediaSourceObjectUrl} );
+postMessage({ subject: messageSubject.HANDLE, info: util.mediaSource.getHandle() } );
 
 // Append increasingly large pieces at a time, starting/continuing at |position|.
 // This allows buffering the test media without timeout, but also with enough

diff --git a/media-source/dedicated-worker/mediasource-worker-duration.html b/media-source/dedicated-worker/mediasource-worker-duration.html
@@ -46,13 +46,12 @@
       case messageSubject.ERROR:
         assert_unreached("Worker error: " + e.data.info);
         break;
-      case messageSubject.OBJECT_URL:
-        const url = e.data.info;
-        assert_true(url.match(/^blob:.+/) !== null);
+      case messageSubject.HANDLE:
+        const handle = e.data.info;
         assert_equals(video.duration, NaN, "initial video duration before attachment should still be NaN");
         assert_equals(video.readyState, HTMLMediaElement.HAVE_NOTHING,
                       "initial video readyState before attachment should still be HAVE_NOTHING");
-        video.src = url;
+        video.srcObject = handle;
         break;
       case messageSubject.VERIFY_DURATION:
         assert_equals(video.duration, e.data.info, "duration should match expectation");
@@ -73,7 +72,7 @@
         // This test is a worker-driven set of verifications, and it will send
         // this message when it is complete. See comment in the worker script
         // that describes the phases of this test case.
-        assert_not_equals(video.src, "", "test should at least have set src.");
+        assert_not_equals(video.srcObject, null, "test should at least have set srcObject.");
         t.done();
         break;
       default:

diff --git a/media-source/dedicated-worker/mediasource-worker-duration.js b/media-source/dedicated-worker/mediasource-worker-duration.js
@@ -16,9 +16,9 @@ const testPhase = {
   // main thread.
   kInitial: "Initial",
 
-  // Main thread receives object URL, re-verifies that the media element
+  // Main thread receives MediaSourceHandle, re-verifies that the media element
   // duration is still NaN and readyState is still HAVE_NOTHING, and then sets
-  // the URL as the src of the media element, eventually causing worker
+  // the handle as the srcObject of the media element, eventually causing worker
   // mediaSource 'onsourceopen' event dispatch.
   kAttaching: "Awaiting sourceopen event that signals attachment is setup",
 
@@ -58,7 +58,6 @@ let phase = testPhase.kInitial;
 
 // Setup handler for receipt of attachment completion.
 util.mediaSource.addEventListener("sourceopen", () => {
-  URL.revokeObjectURL(util.mediaSourceObjectUrl);
   assert(phase === testPhase.kAttaching, "Unexpected sourceopen received by Worker mediaSource.");
   phase = testPhase.kRequestNaNDurationCheck;
   processPhase();
@@ -181,7 +180,7 @@ function processPhase(isResponseToAck = false) {
     case testPhase.kInitial:
       assert(Number.isNaN(util.mediaSource.duration), "Initial unattached MediaSource duration must be NaN, but instead is " + util.mediaSource.duration);
       phase = testPhase.kAttaching;
-      postMessage({ subject: messageSubject.OBJECT_URL, info: util.mediaSourceObjectUrl });
+      postMessage({ subject: messageSubject.HANDLE, info: util.mediaSource.getHandle() });
       break;
 
     case testPhase.kAttaching:

diff --git a/media-source/dedicated-worker/mediasource-worker-get-objecturl.js b/media-source/dedicated-worker/mediasource-worker-get-objecturl.js
@@ -0,0 +1,13 @@
+importScripts("mediasource-worker-util.js");
+
+// Note, we do not use testharness.js utilities within the worker context
+// because it also communicates using postMessage to the main HTML document's
+// harness, and would confuse the test case message parsing there.
+
+onmessage = function(evt) {
+  postMessage({ subject: messageSubject.ERROR, info: "No message expected by Worker"});
+};
+
+let util = new MediaSourceWorkerUtil();
+
+postMessage({ subject: messageSubject.OBJECT_URL, info: URL.createObjectURL(util.mediaSource) });
diff --git a/media-source/dedicated-worker/mediasource-worker-handle.html b/media-source/dedicated-worker/mediasource-worker-handle.html
@@ -0,0 +1,48 @@
+<!DOCTYPE html>
+<html>
+<title>Test MediaSource object and handle creation, with MediaSource in dedicated worker</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script src="mediasource-message-util.js"></script>
+<script>
+
+async_test(t => {
+  // Fail fast if MSE-in-Workers is not supported.
+  assert_true(MediaSource.hasOwnProperty("canConstructInDedicatedWorker"), "MediaSource hasOwnProperty 'canConstructInDedicatedWorker'");
+  assert_true(MediaSource.canConstructInDedicatedWorker, "MediaSource.canConstructInDedicatedWorker");
+  assert_true(window.hasOwnProperty("MediaSourceHandle"), "window must have MediaSourceHandle visibility");
+
+  let worker = new Worker("mediasource-worker-play.js");
+  worker.onmessage = t.step_func(e => {
+    let subject = e.data.subject;
+    assert_true(subject != undefined, "message must have a subject field");
+    switch (subject) {
+      case messageSubject.ERROR:
+        assert_unreached("Worker error: " + e.data.info);
+        break;
+      case messageSubject.HANDLE:
+        const handle = e.data.info;
+        assert_not_equals(handle, null, "must have a non-null MediaSourceHandle");
+        assert_true(handle instanceof MediaSourceHandle, "must be a MediaSourceHandle");
+        t.done();
+        break;
+      default:
+        assert_unreached("Unexpected message subject: " + subject);
+
+    }
+  });
+}, "Test main context receipt of postMessage'd MediaSourceHandle from DedicatedWorker MediaSource");
+
+if (MediaSource.hasOwnProperty("canConstructInDedicatedWorker") && MediaSource.canConstructInDedicatedWorker === true) {
+  // If implementation claims support for MSE-in-Workers, then fetch and run
+  // some tests directly in another dedicated worker and get their results
+  // merged into those from this page.
+  fetch_tests_from_worker(new Worker("mediasource-worker-handle.js"));
+} else {
+  // Otherwise, fetch and run a test that verifies lack of support of
+  // MediaSource construction in another dedicated worker.
+  fetch_tests_from_worker(new Worker("mediasource-worker-must-fail-if-unsupported.js"));
+}
+
+</script>
+</html>
diff --git a/media-source/dedicated-worker/mediasource-worker-handle.js b/media-source/dedicated-worker/mediasource-worker-handle.js
@@ -0,0 +1,45 @@
+importScripts("/resources/testharness.js");
+
+test(t => {
+  // The Window test html conditionally fetches and runs these tests only if the
+  // implementation exposes a true-valued static canConstructInDedicatedWorker
+  // attribute on MediaSource in the Window context. So, the implementation must
+  // agree on support here in the dedicated worker context.
+
+  // Ensure we're executing in a dedicated worker context.
+  assert_true(self instanceof DedicatedWorkerGlobalScope, "self instanceof DedicatedWorkerGlobalScope");
+  assert_true(MediaSource.hasOwnProperty("canConstructInDedicatedWorker", "DedicatedWorker MediaSource hasOwnProperty 'canConstructInDedicatedWorker'"));
+  assert_true(MediaSource.canConstructInDedicatedWorker, "DedicatedWorker MediaSource.canConstructInDedicatedWorker");
+}, "MediaSource in DedicatedWorker context must have true-valued canConstructInDedicatedWorker if Window context had it");
+
+test(t => {
+  assert_true("getHandle" in MediaSource.prototype, "dedicated worker MediaSource must have getHandle");
+  assert_true(self.hasOwnProperty("MediaSourceHandle"), "dedicated worker must have MediaSourceHandle visibility");
+}, "MediaSource prototype in DedicatedWorker context must have getHandle, and worker must have MediaSourceHandle");
+
+test(t => {
+  const ms = new MediaSource();
+  assert_equals(ms.readyState, "closed");
+}, "MediaSource construction succeeds with initial closed readyState in DedicatedWorker");
+
+test(t => {
+  const ms = new MediaSource();
+  const handle = ms.getHandle();
+  assert_not_equals(handle, null, "must have a non-null getHandle result");
+  assert_true(handle instanceof MediaSourceHandle, "must be a MediaSourceHandle");
+}, "mediaSource.getHandle() in DedicatedWorker returns a MediaSourceHandle");
+
+test(t => {
+  const ms = new MediaSource();
+  const handle1 = ms.getHandle();
+  let handle2 = null;
+  assert_throws_dom("InvalidStateError", function()
+    {
+      handle2 = ms.getHandle();
+    }, "getting second handle from MediaSource instance");
+  assert_equals(handle2, null, "getting second handle from same MediaSource must have failed");
+  assert_not_equals(handle1, null, "must have a non-null result of the first getHandle");
+  assert_true(handle1 instanceof MediaSourceHandle, "first getHandle result must be a MediaSourceHandle");
+}, "mediaSource.getHandle() must not succeed more than precisely once for a MediaSource instance");
+
+done();
diff --git a/media-source/dedicated-worker/mediasource-worker-objecturl.html b/media-source/dedicated-worker/mediasource-worker-objecturl.html
@@ -1,6 +1,6 @@
 <!DOCTYPE html>
 <html>
-<title>Test MediaSource object and objectUrl creation and revocation, with MediaSource in dedicated worker</title>
+<title>Test MediaSource object and objectUrl creation and load via that url should fail, with MediaSource in dedicated worker</title>
 <script src="/resources/testharness.js"></script>
 <script src="/resources/testharnessreport.js"></script>
 <script src="mediasource-message-util.js"></script>
@@ -11,7 +11,7 @@
   assert_true(MediaSource.hasOwnProperty("canConstructInDedicatedWorker"), "MediaSource hasOwnProperty 'canConstructInDedicatedWorker'");
   assert_true(MediaSource.canConstructInDedicatedWorker, "MediaSource.canConstructInDedicatedWorker");
 
-  let worker = new Worker("mediasource-worker-play.js");
+  let worker = new Worker("mediasource-worker-get-objecturl.js");
   worker.onmessage = t.step_func(e => {
     let subject = e.data.subject;
     assert_true(subject != undefined, "message must have a subject field");
@@ -21,19 +21,21 @@
         break;
       case messageSubject.OBJECT_URL:
         const url = e.data.info;
-        assert_true(url.match(/^blob:.+/) != null);
-        URL.revokeObjectURL(url);
-        // TODO(crbug.com/1196040): Revoking MediaSource objectURLs on thread
-        // that didn't create them is at best a no-op. This test case is
-        // possibly obsolete.
-        t.done();
+        const video = document.createElement("video");
+        document.body.appendChild(video);
+        video.onerror = t.step_func((target) => {
+          assert_true(video.error != null);
+          assert_equals(video.error.code, MediaError.MEDIA_ERR_SRC_NOT_SUPPORTED);
+          t.done();
+        });
+        video.onended = t.unreached_func("video should not have successfully loaded and played to end");
+        video.src = url;
         break;
       default:
         assert_unreached("Unexpected message subject: " + subject);
-
     }
   });
-}, "Test main context revocation of DedicatedWorker MediaSource object URL");
+}, "Test main context load of a DedicatedWorker MediaSource object URL should fail");
 
 if (MediaSource.hasOwnProperty("canConstructInDedicatedWorker") && MediaSource.canConstructInDedicatedWorker === true) {
   // If implementation claims support for MSE-in-Workers, then fetch and run

diff --git a/media-source/dedicated-worker/mediasource-worker-objecturl.js b/media-source/dedicated-worker/mediasource-worker-objecturl.js
@@ -20,9 +20,7 @@ test(t => {
 test(t => {
   const ms = new MediaSource();
   const url = URL.createObjectURL(ms);
-  assert_true(url != null);
-  assert_true(url.match(/^blob:.+/) != null);
-}, "URL.createObjectURL(mediaSource) in DedicatedWorker returns a Blob URI");
+}, "URL.createObjectURL(mediaSource) in DedicatedWorker does not throw exception");
 
 test(t => {
   const ms = new MediaSource();

diff --git a/media-source/dedicated-worker/mediasource-worker-play-terminate-worker.html b/media-source/dedicated-worker/mediasource-worker-play-terminate-worker.html
@@ -40,7 +40,7 @@
     video.loop = true;
   }
 
-  if (when_to_start_timeouts == "before setting src") {
+  if (when_to_start_timeouts == "before setting srcObject") {
     terminateWorkerAfterMultipleSetTimeouts(test, worker, timeouts_to_await);
   }
 
@@ -51,11 +51,10 @@
       case messageSubject.ERROR:
         assert_unreached("Worker error: " + e.data.info);
         break;
-      case messageSubject.OBJECT_URL:
-        const url = e.data.info;
-        assert_true(url.match(/^blob:.+/) != null);
-        video.src = url;
-        if (when_to_start_timeouts == "after setting src") {
+      case messageSubject.HANDLE:
+        const handle = e.data.info;
+        video.srcObject = handle;
+        if (when_to_start_timeouts == "after setting srcObject") {
           terminateWorkerAfterMultipleSetTimeouts(test, worker, timeouts_to_await);
         }
         video.play().catch(error => {
@@ -73,7 +72,7 @@
   });
 }
 
-[ "before setting src", "after setting src", "after first ended event" ].forEach(when => {
+[ "before setting srcObject", "after setting srcObject", "after first ended event" ].forEach(when => {
   for (let timeouts = 0; timeouts < 10; ++timeouts) {
     async_test(test => { startWorkerAndTerminateWorker(test, when, timeouts); },
         "Test worker MediaSource termination after at least " + timeouts +

diff --git a/media-source/dedicated-worker/mediasource-worker-play.html b/media-source/dedicated-worker/mediasource-worker-play.html
@@ -26,10 +26,9 @@
       case messageSubject.ERROR:
         assert_unreached("Worker error: " + e.data.info);
         break;
-      case messageSubject.OBJECT_URL:
-        const url = e.data.info;
-        assert_true(url.match(/^blob:.+/) != null);
-        video.src = url;
+      case messageSubject.HANDLE:
+        const handle = e.data.info;
+        video.srcObject = handle;
         video.play();
         break;
       default:

diff --git a/media-source/dedicated-worker/mediasource-worker-play.js b/media-source/dedicated-worker/mediasource-worker-play.js
@@ -11,7 +11,6 @@ onmessage = function(evt) {
 let util = new MediaSourceWorkerUtil();
 
 util.mediaSource.addEventListener("sourceopen", () => {
-  URL.revokeObjectURL(util.mediaSourceObjectUrl);
   sourceBuffer = util.mediaSource.addSourceBuffer(util.mediaMetadata.type);
   sourceBuffer.onerror = (err) => {
     postMessage({ subject: messageSubject.ERROR, info: err });
@@ -43,4 +42,4 @@ util.mediaSource.addEventListener("sourceopen", () => {
                              err => { postMessage({ subject: messageSubject.ERROR, info: err }) });
 }, { once : true });
 
-postMessage({ subject: messageSubject.OBJECT_URL, info: util.mediaSourceObjectUrl });
+postMessage({ subject: messageSubject.HANDLE, info: util.mediaSource.getHandle() });
diff --git a/media-source/dedicated-worker/mediasource-worker-util.js b/media-source/dedicated-worker/mediasource-worker-util.js
@@ -22,7 +22,6 @@ let MEDIA_LIST = [
 class MediaSourceWorkerUtil {
   constructor() {
     this.mediaSource = new MediaSource();
-    this.mediaSourceObjectUrl = URL.createObjectURL(this.mediaSource);
 
     // Find supported test media, if any.
     this.foundSupportedMedia = false;