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 is racy #5362

Open
elkurin opened this issue Mar 17, 2020 · 3 comments
Open

SharedWorker is racy #5362

elkurin opened this issue Mar 17, 2020 · 3 comments

Comments

@elkurin
Copy link
Contributor

elkurin commented Mar 17, 2020

Shared worker instances with the same url, name and constructor origin must point to the same shared worker. However, since the spec says "in parallel run a worker", the shared worker instances might point to different shared workers.

Step 11.6: Otherwise, in parallel, run a worker given worker, urlRecord, outside settings, outside port, and options.
https://html.spec.whatwg.org/multipage/workers.html#dom-sharedworker

This happens because SharedWorkerGlobalScope is created after the algorithm is parallelized.

Step 6: For the global object, if is shared is true, create a new SharedWorkerGlobalScope object. Otherwise, create a new DedicatedWorkerGlobalScope object.
https://html.spec.whatwg.org/multipage/workers.html#worker-processing-model

Maybe we should create global scope before parallelizing the algorithm at #dom-sharedworker Step 11.6.

This issue is discussed partially at #5323.

@gterzian
Copy link
Member

gterzian commented Mar 18, 2020

If I understand what you mean correctly: since Step 11.6 starts a parallel algorithm, we loose the ordering guarantees of the parallel queue on which the algorithm has been running so far.

So in theory a "second set of steps" enqueued on the parallel queue could run before the SharedWorkerGlobalScope has been created in the parallel steps started at 11.6 of the previous "set of steps" running on the shared worker manager, and then start a new parallel algorithm and end-up with two sets of shared workers.

I agree that the spec is not precise on this, and while the SharedWorkerGlobalScope is only created at step 6 of worker-processing-model, I'm pretty sure that in practice the "shared worker manager" will add some sort of identifier to the new shared worker and store it somewhere, and this will not be racy with respect to other steps that would have been enqueued on the "shared worker manager".

Yet in other words, the "shared worker manager" is likely to keep track of "run a worker" algorithms that would have been launched from Step 11.6, so that creating the actual shared worker is not racy.

Maybe we should create global scope before parallelizing the algorithm at #dom-sharedworker Step 11.6.

The problem with that is that when you are on the parallel queue, you're not on the event-loop of the shared worker(which perhaps hasn't even been created yet), so you wouldn't be able to create or manipulate artifacts from that world, see https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-for-spec-authors

@gterzian
Copy link
Member

gterzian commented Mar 18, 2020

By the way the close flag check at step 11.2 of https://html.spec.whatwg.org/multipage/workers.html#dom-sharedworker, is a good example of the spec assuming the parallel steps have sync access to the globalscope, somewhat similar to the assumption of cross-event-loop sync access discussed at #1371, #3691, and #5327

@annevk
Copy link
Member

annevk commented Jul 8, 2020

See #4734 (comment) for the latest discussion here. What needs to happen is that we fetch first and then once the response is in (though not necessarily the body) we create the new agent (and possibly an encompassing agent cluster). From @domenic:

It would be particularly bad for modules, because parsing requires a realm to exist, but you need to parse before you can continue fetching dependencies. So we'd need to split it up into something like

  1. Fetch top-level source text. (Technically all we need are the headers.... maybe stop at getting the response.)
  2. Create the realm
  3. Create top-level script
  4. Fetch and create dependency scripts

Funnnnn.

annevk added a commit that referenced this issue Jul 8, 2020
A top-level navigation response with Cross-Origin-Opener-Policy set to same-origin and Cross-Origin-Embedder-Policy set to require-corp will create a cross-origin isolated browsing context group. And all agent clusters therein will be cross-origin isolated as well (shared and service workers can still not be, as they sit on the side).

This change also:

* Gates SharedArrayBuffer exposure behind that primitive for web compatibility reasons.
* Gates SharedArrayBuffer sharing behind that primitive.
* Exposes it through self.crossOriginIsolated.
* Makes document.domain return before it mutates the origin.
* Makes agent clusters keyed on origin.

Tests:

* web-platform-tests/wpt#17719
* web-platform-tests/wpt#17760
* web-platform-tests/wpt#17761
* web-platform-tests/wpt#17802
* web-platform-tests/wpt#17909
* web-platform-tests/wpt#18543
* web-platform-tests/wpt#20116
* web-platform-tests/wpt#22358

Closes #4732. Closes #5122. Closes #5444.

Follow-up: #5435 (and #5362).
mfreed7 pushed a commit to mfreed7/html that referenced this issue Sep 11, 2020
A top-level navigation response with Cross-Origin-Opener-Policy set to same-origin and Cross-Origin-Embedder-Policy set to require-corp will create a cross-origin isolated browsing context group. And all agent clusters therein will be cross-origin isolated as well (shared and service workers can still not be, as they sit on the side).

This change also:

* Gates SharedArrayBuffer exposure behind that primitive for web compatibility reasons.
* Gates SharedArrayBuffer sharing behind that primitive.
* Exposes it through self.crossOriginIsolated.
* Makes document.domain return before it mutates the origin.
* Makes agent clusters keyed on origin.

Tests:

* web-platform-tests/wpt#17719
* web-platform-tests/wpt#17760
* web-platform-tests/wpt#17761
* web-platform-tests/wpt#17802
* web-platform-tests/wpt#17909
* web-platform-tests/wpt#18543
* web-platform-tests/wpt#20116
* web-platform-tests/wpt#22358

Closes whatwg#4732. Closes whatwg#5122. Closes whatwg#5444.

Follow-up: whatwg#5435 (and whatwg#5362).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants