From 9589b7d7210eb65f30af294f454912a5e90a7d53 Mon Sep 17 00:00:00 2001 From: Nidhi Jaju Date: Thu, 9 Jun 2022 20:04:07 -0700 Subject: [PATCH] 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: > https://github.com/w3c/media-source/issues/175 > MSE spec feature updates switching from worker MSE attachment via > object URL to attachment via srcObject MediaSourceHandle: > * https://github.com/w3c/media-source/pull/305 > * further clarifications in discussion at > https://github.com/w3c/media-source/pull/306#issuecomment-1144180822 > > BUG=878133 > > Change-Id: I60ddca79ee0f95c87b6d84e4f44ad9c283f359a3 > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3698231 > Commit-Queue: Matthew Wolenetz > Auto-Submit: Matthew Wolenetz > Reviewed-by: Will Cassella > 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 Commit-Queue: Nidhi Jaju Owners-Override: Nidhi Jaju Cr-Commit-Position: refs/heads/main@{#1012823} --- .../mediasource-message-util.js | 1 - .../mediasource-worker-detach-element.html | 18 +++---- .../mediasource-worker-detach-element.js | 3 +- .../mediasource-worker-duration.html | 9 ++-- .../mediasource-worker-duration.js | 7 +-- .../mediasource-worker-get-objecturl.js | 13 ----- .../mediasource-worker-handle.html | 48 ------------------- .../mediasource-worker-handle.js | 45 ----------------- .../mediasource-worker-objecturl.html | 22 ++++----- .../mediasource-worker-objecturl.js | 4 +- ...iasource-worker-play-terminate-worker.html | 13 ++--- .../mediasource-worker-play.html | 7 +-- .../mediasource-worker-play.js | 3 +- .../mediasource-worker-util.js | 1 + 14 files changed, 48 insertions(+), 146 deletions(-) delete mode 100644 media-source/dedicated-worker/mediasource-worker-get-objecturl.js delete mode 100644 media-source/dedicated-worker/mediasource-worker-handle.html delete mode 100644 media-source/dedicated-worker/mediasource-worker-handle.js diff --git a/media-source/dedicated-worker/mediasource-message-util.js b/media-source/dedicated-worker/mediasource-message-util.js index c62eb8e3f7dc9a..247071db4f13f3 100644 --- a/media-source/dedicated-worker/mediasource-message-util.js +++ b/media-source/dedicated-worker/mediasource-message-util.js @@ -4,7 +4,6 @@ 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 index 0f74d953723a40..7b00c59a07be9f 100644 --- a/media-source/dedicated-worker/mediasource-worker-detach-element.html +++ b/media-source/dedicated-worker/mediasource-worker-detach-element.html @@ -7,11 +7,11 @@ - - - - diff --git a/media-source/dedicated-worker/mediasource-worker-handle.js b/media-source/dedicated-worker/mediasource-worker-handle.js deleted file mode 100644 index 577b1facbc9fcd..00000000000000 --- a/media-source/dedicated-worker/mediasource-worker-handle.js +++ /dev/null @@ -1,45 +0,0 @@ -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 index ae6019967252e5..5553b5c631e891 100644 --- a/media-source/dedicated-worker/mediasource-worker-objecturl.html +++ b/media-source/dedicated-worker/mediasource-worker-objecturl.html @@ -1,6 +1,6 @@ -Test MediaSource object and objectUrl creation and load via that url should fail, with MediaSource in dedicated worker +Test MediaSource object and objectUrl creation and revocation, with MediaSource in dedicated worker @@ -11,7 +11,7 @@ assert_true(MediaSource.hasOwnProperty("canConstructInDedicatedWorker"), "MediaSource hasOwnProperty 'canConstructInDedicatedWorker'"); assert_true(MediaSource.canConstructInDedicatedWorker, "MediaSource.canConstructInDedicatedWorker"); - let worker = new Worker("mediasource-worker-get-objecturl.js"); + 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"); @@ -21,21 +21,19 @@ break; case messageSubject.OBJECT_URL: const url = e.data.info; - 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; + 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(); break; default: assert_unreached("Unexpected message subject: " + subject); + } }); -}, "Test main context load of a DedicatedWorker MediaSource object URL should fail"); +}, "Test main context revocation of DedicatedWorker MediaSource object URL"); 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 index 2e70d99418173d..9a7195fc5bda04 100644 --- a/media-source/dedicated-worker/mediasource-worker-objecturl.js +++ b/media-source/dedicated-worker/mediasource-worker-objecturl.js @@ -20,7 +20,9 @@ test(t => { test(t => { const ms = new MediaSource(); const url = URL.createObjectURL(ms); -}, "URL.createObjectURL(mediaSource) in DedicatedWorker does not throw exception"); + assert_true(url != null); + assert_true(url.match(/^blob:.+/) != null); +}, "URL.createObjectURL(mediaSource) in DedicatedWorker returns a Blob URI"); 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 index d6496afd6f4b29..ca8b6970f89576 100644 --- 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 srcObject") { + if (when_to_start_timeouts == "before setting src") { terminateWorkerAfterMultipleSetTimeouts(test, worker, timeouts_to_await); } @@ -51,10 +51,11 @@ case messageSubject.ERROR: assert_unreached("Worker error: " + e.data.info); break; - case messageSubject.HANDLE: - const handle = e.data.info; - video.srcObject = handle; - if (when_to_start_timeouts == "after setting srcObject") { + 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") { terminateWorkerAfterMultipleSetTimeouts(test, worker, timeouts_to_await); } video.play().catch(error => { @@ -72,7 +73,7 @@ }); } -[ "before setting srcObject", "after setting srcObject", "after first ended event" ].forEach(when => { +[ "before setting src", "after setting src", "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 index 0292b13d09ff6a..07317e6d0c9f0b 100644 --- a/media-source/dedicated-worker/mediasource-worker-play.html +++ b/media-source/dedicated-worker/mediasource-worker-play.html @@ -26,9 +26,10 @@ case messageSubject.ERROR: assert_unreached("Worker error: " + e.data.info); break; - case messageSubject.HANDLE: - const handle = e.data.info; - video.srcObject = handle; + case messageSubject.OBJECT_URL: + const url = e.data.info; + assert_true(url.match(/^blob:.+/) != null); + video.src = url; video.play(); break; default: diff --git a/media-source/dedicated-worker/mediasource-worker-play.js b/media-source/dedicated-worker/mediasource-worker-play.js index e29b1b8de6397b..0312f16fd99e7a 100644 --- a/media-source/dedicated-worker/mediasource-worker-play.js +++ b/media-source/dedicated-worker/mediasource-worker-play.js @@ -11,6 +11,7 @@ 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 }); @@ -42,4 +43,4 @@ util.mediaSource.addEventListener("sourceopen", () => { err => { postMessage({ subject: messageSubject.ERROR, info: err }) }); }, { once : true }); -postMessage({ subject: messageSubject.HANDLE, info: util.mediaSource.getHandle() }); +postMessage({ subject: messageSubject.OBJECT_URL, info: util.mediaSourceObjectUrl }); diff --git a/media-source/dedicated-worker/mediasource-worker-util.js b/media-source/dedicated-worker/mediasource-worker-util.js index 7adaf82508d0d1..695d1179d39b18 100644 --- a/media-source/dedicated-worker/mediasource-worker-util.js +++ b/media-source/dedicated-worker/mediasource-worker-util.js @@ -22,6 +22,7 @@ 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;