Spec update: Support multiple & cross-origin worklets #131

xyaoinum · 2024-01-18T20:24:20Z

selectURL() and run() will be exposed to the shared storage worklet object. When calling on the default scoped worklet (i.e. sharedStorage.worklet.selectURL()/run()), the behavior is equivalent to sharedStorage.selectURL()/run().
Users can create new worklets via const worklet = await sharedStorage.createWorklet(url, options). This can be used to start multiple and potentially cross-origin worklets from a single document.
User settings error won't be exposed to the caller (i.e. will be treated as success) in the case of creating or using a cross-origin worklet. This is to prevent leaking user's settings for an arbitrary site.

- selectURL() and run() will be exposed to the shared storage worklet object. When calling on the default scoped worklet (i.e. sharedStorage.worklet.selectURL()/run()), the behavior is equivalent to sharedStorage.selectURL()/run(). - Users can create new worklets via `const worklet = await sharedStorage.createWorklet(url, options)`. This can be used to start multiple and potentially cross-origin worklets from a single document. - User settings error won't be exposed to the caller (i.e. will be treated as success) in the case of creating or using a cross-origin worklet. This is to prevent leaking user's settings for an arbitrary site.

xyaoinum · 2024-01-18T20:47:37Z

@pythagoraskitty: PTAL. Thanks! (After it's looking okay from your end, I will also request review from a spec mentor.)

spec.bs

pythagoraskitty · 2024-01-23T01:06:10Z

Suggest we move the {#reporting} and {#budgets} subsubsections of {#window} up to be subsections of the {#worklet} section instead. I have no strong preference about the ordering within that section, but you could probably just tack them on the end of it after {#scope-algo}.

pythagoraskitty · 2024-01-23T02:00:44Z

I've looked at most, except need to give a closer read to the new versions of selectURL() and run() sometime when I am less tired. :)

Hopefully will get to in the next couple of days. Otherwise looks good % comments.

spec.bs

pythagoraskitty · 2024-01-24T22:38:16Z

Finished checking. Looks good % comments. :)

xyaoinum · 2024-01-31T00:33:50Z

Thanks Cammie. I did some more changes around dividing the permissions checks, as explained in the comment above. PTAL again!

@domfarolino : PTAL from the spec's perspective. Thanks!

For the sharedStorage.createWorklet() API, relax the restriction to allow cross-origin script, in which case a cross-origin worklet will be created. We leverage the existing process allocation and management logic from service workers and directly re-use SiteInstanceImpl::CreateForServiceWorker(). To keep this CL focused, renaming will occur in a separate CL, as it involves renaming other downstream components like 'UnmatchedServiceWorkerProcessTracker'. Explainer: WICG/shared-storage#130 Spec: WICG/shared-storage#131 Design doc: https://docs.google.com/document/d/1QTaaroCMeFVZVghI6JkUcDvmDQEacjvpyTfk6mpvQhA/edit?usp=sharing&resourcekey=0-dqfVkB2DJcTzHLfJkern0Q Bug: 325302836 Change-Id: I11c1fc87bc76f4400c54d9fa809349d1d1781247

For the sharedStorage.createWorklet() API, relax the restriction to allow cross-origin script, in which case a cross-origin worklet will be created. We leverage the existing process allocation and management logic from service workers and directly re-use SiteInstanceImpl::CreateForServiceWorker(). To keep this CL focused, renaming will occur in a separate CL, as it involves renaming other downstream components like 'UnmatchedServiceWorkerProcessTracker'. Explainer: WICG/shared-storage#130 Spec: WICG/shared-storage#131 Design doc: https://docs.google.com/document/d/1QTaaroCMeFVZVghI6JkUcDvmDQEacjvpyTfk6mpvQhA/edit?usp=sharing Bug: 325302836 Change-Id: I11c1fc87bc76f4400c54d9fa809349d1d1781247

For the sharedStorage.createWorklet() API, relax the same-origin restriction to allow cross-origin script, in which case a cross-origin worklet will be created. How: - Rely on CORS for the worklet to be loaded/used by the embedder. - Leverage the existing process allocation and management logic from service workers and directly re-use SiteInstanceImpl::CreateForServiceWorker(). To keep this CL focused, renaming will occur in a separate CL, as it will involve renaming other downstream components like 'UnmatchedServiceWorkerProcessTracker'. Explainer: WICG/shared-storage#130 Spec: WICG/shared-storage#131 Design doc: https://docs.google.com/document/d/1QTaaroCMeFVZVghI6JkUcDvmDQEacjvpyTfk6mpvQhA/edit?usp=sharing Bug: 325302836 Change-Id: I11c1fc87bc76f4400c54d9fa809349d1d1781247

xyaoinum · 2024-02-15T01:43:10Z

@domfarolino : friendly ping! Also adding @wanderview as an alternative reviewer, who may be able to cover the review this week when Dominic is OOO. Thanks! The idea is to make the spec good enough to unblock launch. We can track non-blocking issues separately if applicable.

For the sharedStorage.createWorklet() API, relax the same-origin restriction to allow cross-origin script, in which case a cross-origin worklet will be created. How: - Rely on CORS for the worklet to be loaded/used by the embedder. - Leverage the existing process allocation and management logic from service workers and directly re-use SiteInstanceImpl::CreateForServiceWorker(). To keep this CL focused, renaming will occur in a separate CL, as it will involve renaming other downstream components like 'UnmatchedServiceWorkerProcessTracker'. Explainer: WICG/shared-storage#130 Spec: WICG/shared-storage#131 Design doc: https://docs.google.com/document/d/1QTaaroCMeFVZVghI6JkUcDvmDQEacjvpyTfk6mpvQhA/edit?usp=sharing Bug: 325302836 Change-Id: I11c1fc87bc76f4400c54d9fa809349d1d1781247

xyaoinum · 2024-03-27T20:45:39Z

@domfarolino friendly ping. Thanks! (Note also that I just updated this patch to include a "Shared-Storage-Worklet-Allowed: ?1" response header check, which was based on a security review feedback (i.e. relying solely on CORS isn't considered safe enough)).

…e header check When creating a cross-origin worklet, require the "Shared-Storage-Worklet-Allowed: ?1" response header, or the request should fail (similar to the handling for CORS failure). Note that shared storage worklet request doesn't allow redirects, so it's sufficient to check inside `OnReceiveResponse` only. PR: WICG/shared-storage#131 Bug: 332564979 Change-Id: I6c2a07473527ede995cf4bd337d293f3168351bb

…lowed response header check When creating a cross-origin worklet, require the "Shared-Storage-Cross-Origin-Worklet-Allowed: ?1" response header, or the request should fail (similar to the handling for CORS failure). Note that shared storage worklet request doesn't allow redirects, so it's sufficient to check inside `OnReceiveResponse` only. PR: WICG/shared-storage#131 Bug: 332564979 Change-Id: I6c2a07473527ede995cf4bd337d293f3168351bb

domfarolino · 2024-04-10T14:35:14Z

Can you separate out the header code from this PR to review separately? I just want to avoid piling more things onto this CL which is already massive and difficult to review.

spec.bs

domfarolino · 2024-04-10T15:42:33Z

Btw has #131 (comment) been address anywhere? I don't see a way to respond to it directly right now, but I can't tell if it has been addressed or commented on really.

Edit: Yes it has, per #131 (comment).

- Addressed review comments - Took out the header change, to make this patch shorter

xyaoinum · 2024-04-10T17:07:28Z

@domfarolino I addressed the comments, and took out the header part to make this patch smaller. PTAL again. Thanks! Also, responding to your comments below that I'm unable to respond inline:

For comment #131 (comment): I updated it to [=shared storage navigation budget charged=]" which I feel is more clear.

domfarolino

Nothing too major structurally-speaking, which is good. This will probably be the last substantial pass.

spec.bs

domfarolino · 2024-04-12T18:26:50Z

spec.bs

+
+    - For each method under [[#window-setter]], |environment| is the current context, and |origin| is |environment|'s [=environment settings object/origin=].
+    - For creating a worklet, |environment| is the [=environment settings object=] associated with the {{Window}} that created the worklet, and |origin| is the module scirpt url's [=url/origin=].
+    - For initiating (from a {{Window}}) and for running (from {{SharedStorageWorkletGlobalScope}}) operations on a worklet, |environment| is the [=environment settings object=] associated with the {{Window}} that created the worklet, and |origin| is the worklet's [=global scopes=][0]'s [=global object/realm=]'s \[[HostDefined]] field's [=environment settings object/origin=].


for running (from SharedStorageWorkletGlobalScope)

Is this why step 6 of https://pr-preview.s3.amazonaws.com/WICG/shared-storage/pull/131.html#determine-whether-shared-storage-is-allowed-by-context asserts that the current global might be a SharedStorageWorkletGlobalScope? I guess that confuses me a little bit: does it mean that a shared storage worklet can create new shared storage worklets? I certainly don't think so, but just want to triple check.

Ack that this was indeed confusing. I updated it to "For running operations on a worklet (from a {{Window}}), and for each method under [[#worklet-setter]] (from {{SharedStorageWorkletGlobalScope}}), ..."

spec.bs

xyaoinum · 2024-04-13T03:09:19Z

@domfarolino I've addressed / responded to your comments. PTAL again. Thanks!
@gtanzer Please confirm if my understanding in #131 (comment) is correct. Thanks!

domfarolino

OK I think we've addressed the meat of worklet internals with the previous reviews here so far, so I'm happy moving forward with this due to the urgency of this work, and thank you very much for sticking with it as well as filing the follow-up issues for the small remaining things that can be addressed separately.

Please send PRs soon to fix the following:

[spec] "name" and "operationCtor" parameter cannot be missing during register() #151
[spec] Move the call [=charge shared storage reporting budget=] to the fenced frame spec. #150
[spec] Add boolean "shared storage navigation/reporting budget charged" to [=fenced frame config instance=] #149
[spec] Move the definition of [=pending shared storage budget debit=] to [=fenced frame config instance=] #148
[spec] Many definitions under "Entropy Budgets" section needs improving #147
Window setter/deleter methods don't match up with any Web IDL #146
[spec] Add "sharedstorageworklet" destination to RequestDestination #145
Use of function is wrong in the operation map definition #135
Use of object is wrong in the register() method #134

More smaller PRs would be better than larger PRs combining the issues above.

domfarolino · 2024-04-18T19:33:42Z

spec.bs

+    1. Let |globalScope| be [=this=]'s [=global scopes=][0].
+    1. Let |workletOrigin| be |globalScope|'s [=global object/realm=]'s \[[HostDefined]] field's [=environment settings object/origin=].
+    1. If the result of running [=Is feature enabled in document for origin?=] on "[=PermissionsPolicy/shared-storage-select-url=]", |document|, and |workletOrigin| returns false, return a [=promise rejected=] with a {{TypeError}}.
+    1. If [=this=]'s [=global scopes=] is [=list/empty=], then return a [=promise rejected=] with a {{TypeError}}.


Ah, you're saying selectURL() cannot be called after createWorklet() then, right?

spec.bs

xyaoinum · 2024-04-18T20:25:57Z

Thanks for the thorough review @domfarolino . (For the last few comments, I think you might looked at an outdated version of the patch? The comments in question should have already been addressed.)

…ader check This is a follow-up patch for #131. For starting a cross-origin worklet, this response header is needed.

jkarlin · 2024-04-19T17:49:47Z

Applies to issue #2

xyaoinum requested a review from pythagoraskitty January 18, 2024 20:47

pythagoraskitty reviewed Jan 23, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 23, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 23, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

pythagoraskitty reviewed Jan 24, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

Address review comments

6606cc0

xyaoinum requested a review from domfarolino January 31, 2024 00:34

chromium-wpt-export-bot mentioned this pull request Feb 14, 2024

[shared storage] Support cross-origin worklet web-platform-tests/wpt#44596

Merged

xyaoinum requested a review from wanderview February 15, 2024 01:43

xyaoinum added 2 commits March 27, 2024 16:32

Add "Shared-Storage-Worklet-Allowed" response header check

4ddc5e7

Move and improve a "Note"

8183640

chromium-wpt-export-bot mentioned this pull request Apr 2, 2024

[shared storage] Implement the Shared-Storage-Cross-Origin-Worklet-Allowed response header check web-platform-tests/wpt#45481

Merged

xyaoinum added 2 commits April 4, 2024 11:42

Rename the header to "Shared-Storage-Cross-Origin-Worklet-Allowed"

931713d

Update the request's boolean field name

9507e05

domfarolino reviewed Apr 10, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

spec.bs Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

spec.bs Outdated Show resolved Hide resolved

domfarolino reviewed Apr 10, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

domfarolino reviewed Apr 10, 2024

View reviewed changes

spec.bs Outdated Show resolved Hide resolved

Address review comments

cdb209d

- Addressed review comments - Took out the header change, to make this patch shorter

xyaoinum mentioned this pull request Apr 11, 2024

[spec] Add "sharedstorageworklet" destination to RequestDestination #145

Closed

domfarolino reviewed Apr 12, 2024

View reviewed changes

Address comments

9100129

domfarolino approved these changes Apr 18, 2024

View reviewed changes

xyaoinum merged commit 1d8d4af into main Apr 18, 2024
1 check passed

domfarolino deleted the yao-spec branch April 18, 2024 20:35

xyaoinum added a commit that referenced this pull request Apr 18, 2024

Include the "Shared-Storage-Cross-Origin-Worklet-Allowed" response he…

df02011

…ader check This is a follow-up patch for #131. For starting a cross-origin worklet, this response header is needed.

xyaoinum mentioned this pull request Apr 18, 2024

Include the "Shared-Storage-Cross-Origin-Worklet-Allowed" response header check #152

Merged

jkarlin mentioned this pull request Apr 24, 2024

Shared Storage API w3ctag/design-reviews#747

Closed

xyaoinum mentioned this pull request Jul 3, 2024

Spec: Allow cross-origin script in addModule & align createWorklet #161

Merged

Elchi3 mentioned this pull request Dec 17, 2024

Remove obsolete SharedStorage interfaces mdn/browser-compat-data#25450

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spec update: Support multiple & cross-origin worklets #131

Spec update: Support multiple & cross-origin worklets #131

xyaoinum commented Jan 18, 2024 •

edited by pr-preview bot

Loading

xyaoinum commented Jan 18, 2024

pythagoraskitty commented Jan 23, 2024

pythagoraskitty commented Jan 23, 2024

pythagoraskitty commented Jan 24, 2024

xyaoinum commented Jan 31, 2024

xyaoinum commented Feb 15, 2024

xyaoinum commented Mar 27, 2024

domfarolino commented Apr 10, 2024

domfarolino commented Apr 10, 2024 •

edited

Loading

xyaoinum commented Apr 10, 2024

domfarolino left a comment

domfarolino Apr 12, 2024

xyaoinum Apr 13, 2024

xyaoinum commented Apr 13, 2024

domfarolino left a comment

domfarolino Apr 18, 2024

xyaoinum commented Apr 18, 2024

jkarlin commented Apr 19, 2024

Spec update: Support multiple & cross-origin worklets #131

Spec update: Support multiple & cross-origin worklets #131

Conversation

xyaoinum commented Jan 18, 2024 • edited by pr-preview bot Loading

xyaoinum commented Jan 18, 2024

pythagoraskitty commented Jan 23, 2024

pythagoraskitty commented Jan 23, 2024

pythagoraskitty commented Jan 24, 2024

xyaoinum commented Jan 31, 2024

xyaoinum commented Feb 15, 2024

xyaoinum commented Mar 27, 2024

domfarolino commented Apr 10, 2024

domfarolino commented Apr 10, 2024 • edited Loading

xyaoinum commented Apr 10, 2024

domfarolino left a comment

Choose a reason for hiding this comment

domfarolino Apr 12, 2024

Choose a reason for hiding this comment

xyaoinum Apr 13, 2024

Choose a reason for hiding this comment

xyaoinum commented Apr 13, 2024

domfarolino left a comment

Choose a reason for hiding this comment

domfarolino Apr 18, 2024

Choose a reason for hiding this comment

xyaoinum commented Apr 18, 2024

jkarlin commented Apr 19, 2024

xyaoinum commented Jan 18, 2024 •

edited by pr-preview bot

Loading

domfarolino commented Apr 10, 2024 •

edited

Loading