Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SharedWorker support? #635

Closed
dalecurtis opened this issue Feb 1, 2023 · 12 comments · Fixed by web-platform-tests/wpt#38351
Closed

SharedWorker support? #635

dalecurtis opened this issue Feb 1, 2023 · 12 comments · Fixed by web-platform-tests/wpt#38351

Comments

@dalecurtis
Copy link
Contributor

I just noticed we have a WPT test checking for SharedWorker support:
https://wpt.fyi/results/webcodecs/videoFrame-serialization.crossAgentCluster.https.html?label=experimental&label=master&aligned

It looks like it was added by Firefox here:
https://hg.mozilla.org/integration/autoland/rev/5a2fc97cac78

@padenot @youennf should we enable SharedWorker support? No objections off hand on the Chromium side. I'm not very familiar with the technical limitations around SharedWorkers though.

@youennf
Copy link
Contributor

youennf commented Feb 1, 2023

Ah, VideoFrame is interesting since it is transferable/serializable but is only exposed in window/DedicatedWorker.
I am wondering what should happen if a VideoFrame is transferred to a SharedWorker then.
cc @jan-ivar with whom we were speaking of such kind of objects.

I do not see anything particularly wrong in having VideoFrames in all workers, except the use cases are probably smaller in non dedicated workers.
Technically speaking, this is no different in WebKit than in DedicatedWorker so should be straightforward.

should we enable SharedWorker support?

For VideoFrame or for all WebCodecs, i.e. encoders/decoders?

@dalecurtis
Copy link
Contributor Author

I think it would make sense to expose everything if we can for consistency. I'd be fine with just VideoFrame if there's reasons we shouldn't for the codecs though.

@jan-ivar
Copy link
Member

jan-ivar commented Feb 1, 2023

I am wondering what should happen if a VideoFrame is transferred to a SharedWorker then.

You'll get a DataCloneError when you deserialize:

I think it would make sense to expose everything if we can for consistency.

A more conservative approach would be to only expose what we need for the use cases we know of. I'd favor that since exposing more later is cheap, but once we've exposed something, it's much harder if not impossible to put back in the bottle.

For instance, the wpt mentioned verifies that "frames cannot be transferred to serviceworker".

@jan-ivar
Copy link
Member

jan-ivar commented Feb 1, 2023

For instance, VideoFrames may hold on to physical resources. So preventing their spread to realms that outlast where they came from may have consequences we haven't thought of.

@dalecurtis
Copy link
Contributor Author

dalecurtis commented Feb 1, 2023

Yeah, ServiceWorker exposure seems questionable, but SharedWorker seems like a logical extension and doesn't seem to have any consequences that two DedicatedWorker instances wouldn't have.

@jan-ivar
Copy link
Member

jan-ivar commented Feb 1, 2023

Well, DedicatedWorkers go away with the document, whereas a SharedWorker might outlive it. I also don't know for sure if it adds out of process requirements or not sometimes? Or are SharedWorkers implicitly same process?

That said, since the WPT from Mozilla allowed it, maybe @padenot has thoughts on it?

@dalecurtis
Copy link
Contributor Author

That's fair. Out of process requirements might be why we didn't add it originally since not all frames a backed in a way that's shareable without readback/copy.

@dalecurtis
Copy link
Contributor Author

If we decide we shouldn't have SharedWorker (or at least not yet) I'll land https://chromium-review.googlesource.com/c/chromium/src/+/4216102 to fix the wpt test.

@youennf
Copy link
Contributor

youennf commented Feb 2, 2023

Thanks for the link, this is good to know!

For instance, VideoFrames may hold on to physical resources. So preventing their spread to realms that outlast where they came from may have consequences we haven't thought of.

It is already a problem without workers.
You can postMessage a VideoFrame from one window to another and you have to deal with the fact that the other window might hook to it forever.

I also don't know for sure if it adds out of process requirements or not sometimes? Or are SharedWorkers implicitly same process?

Given you can post message a video frame to another tab, or a third-party iframe, the spec already mandates out of process support (implementations may lag though, Safari does not yet support this).

So far, I haven't seen any new issue/requirement related to SharedWorker/ServiceWorker exposure.
But I did not really hear anybody asking for it either, I would tend to wait for people asking for it.

I'll land https://chromium-review.googlesource.com/c/chromium/src/+/4216102 to fix the wpt test.

That sounds good to me.
I do not think one can expect SharedWorker to be in another process.
I would tend to use different tabs instead, maybe process isolation as well to be extra sure.

@dalecurtis
Copy link
Contributor Author

We've had only a single request for SharedWorker:
https://bugs.chromium.org/p/chromium/issues/detail?id=1410412

I think the use case can be handled in other ways though. I'll wait to hear from @padenot before landing the test change.

@padenot
Copy link
Collaborator

padenot commented Feb 3, 2023

I think this is a mistake on our side. This shouldn't work in SharedWorker, it's not exposed there.

@dalecurtis
Copy link
Contributor Author

Thanks Paul. I'll go ahead and land the test fix then. I'll close this for now and devs can +1 or chime in on it if this is something they'd like to see added.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 3, 2023
Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 3, 2023
Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
aarongable pushed a commit to chromium/chromium that referenced this issue Feb 3, 2023
Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4216102
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101160}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 3, 2023
Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4216102
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101160}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Feb 3, 2023
Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4216102
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101160}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 5, 2023
…rted in serialization., a=testonly

Automatic update from web-platform-tests
[WebCodecs] Mark SharedWorker as unsupported in serialization.

Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4216102
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101160}

--

wpt-commits: 5d401863b2d8363e1ea35558d5c10a9b38c557a6
wpt-pr: 38351
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Feb 7, 2023
…rted in serialization., a=testonly

Automatic update from web-platform-tests
[WebCodecs] Mark SharedWorker as unsupported in serialization.

Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4216102
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101160}

--

wpt-commits: 5d401863b2d8363e1ea35558d5c10a9b38c557a6
wpt-pr: 38351
marcoscaceres pushed a commit to web-platform-tests/wpt that referenced this issue Mar 28, 2023
Test incorrectly assumes SharedWorker support, so ensure it instead
tests that passing to a shared worker is unsupported.

Fixes w3c/webcodecs#635

Bug: 1412203
Change-Id: I596fa79508a3cd8e2ed253d9be378b142edca4d4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4216102
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1101160}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants