From 8e5b88e97b620159b1275a548f090ccc46c3529f Mon Sep 17 00:00:00 2001 From: Matt Wolenetz Date: Wed, 15 Jun 2022 19:06:27 -0700 Subject: [PATCH] Reland: MSE-in-Workers: srcObject part 5: Conditionally fail worker objectURL 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: > > 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} Bug: 878133 Change-Id: I56e4ecd4d8b58d9d58ed3c575b0fb52f596b6fae Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3708465 Reviewed-by: Will Cassella Commit-Queue: Matthew Wolenetz Cr-Commit-Position: refs/heads/main@{#1014755} --- .../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, 146 insertions(+), 48 deletions(-) create mode 100644 media-source/dedicated-worker/mediasource-worker-get-objecturl.js create mode 100644 media-source/dedicated-worker/mediasource-worker-handle.html create 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 247071db4f13f3..c62eb8e3f7dc9a 100644 --- 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 index 7b00c59a07be9f..0f74d953723a40 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 new file mode 100644 index 00000000000000..577b1facbc9fcd --- /dev/null +++ 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 index 5553b5c631e891..ae6019967252e5 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 revocation, with MediaSource in dedicated worker +Test MediaSource object and objectUrl creation and load via that url should fail, 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-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 index 9a7195fc5bda04..2e70d99418173d 100644 --- 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 index ca8b6970f89576..d6496afd6f4b29 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 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 index 07317e6d0c9f0b..0292b13d09ff6a 100644 --- 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 index 0312f16fd99e7a..e29b1b8de6397b 100644 --- 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 index 695d1179d39b18..7adaf82508d0d1 100644 --- 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;