Bug 235107 - Improve computation of service worker FetchEvent.resultingClientId
Summary: Improve computation of service worker FetchEvent.resultingClientId
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 235070
Blocks: 235310
  Show dependency treegraph
 
Reported: 2022-01-12 03:47 PST by youenn fablet
Modified: 2022-02-03 22:09 PST (History)
20 users (show)

See Also:


Attachments
Patch (35.81 KB, patch)
2022-01-12 03:50 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (44.30 KB, patch)
2022-01-18 00:03 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (44.34 KB, patch)
2022-01-18 07:55 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (45.01 KB, patch)
2022-01-19 04:58 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2022-01-12 03:47:19 PST
Improve computation of service worker FetchEvent.resultingClientId
Comment 1 youenn fablet 2022-01-12 03:50:31 PST
Created attachment 448925 [details]
Patch
Comment 2 youenn fablet 2022-01-18 00:03:53 PST
Created attachment 449365 [details]
Patch
Comment 3 youenn fablet 2022-01-18 00:35:50 PST
Comment on attachment 449365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=449365&action=review

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-get-resultingClientId.https-expected.txt:-5
> -PASS get(resultingClientId) for document sandboxed by CSP header

Regression fixed in https://bugs.webkit.org/show_bug.cgi?id=235310
Comment 4 youenn fablet 2022-01-18 07:55:26 PST
Created attachment 449390 [details]
Patch
Comment 5 Darin Adler 2022-01-18 14:17:39 PST
Comment on attachment 449390 [details]
Patch

The code should use ScriptExecutionContextIdentifier, not const std::optional<ScriptExecutionIdentifier>&. These identifiers are not particularly huge structures and don’t need to be passed by reference. And also, there is already a null value for identifiers, so we don’t need to wrap them in std::optional just to have a null meaning nothing is specified. We can create the null value with the default constructor, and detect whether it’s null using the operator bool.

I’m not sure how we decided in which cases to have a default value and in which cases callers have to explicitly pass { }. Since specifying a pre-determined identifier is fairly unusual, we should consider adding a default everywhere, and these calls to HTMLDocument::create would not have to change.
Comment 6 youenn fablet 2022-01-19 00:57:35 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 449390 [details]
> Patch
> 
> The code should use ScriptExecutionContextIdentifier, not const
> std::optional<ScriptExecutionIdentifier>&. These identifiers are not
> particularly huge structures and don’t need to be passed by reference. And
> also, there is already a null value for identifiers, so we don’t need to
> wrap them in std::optional just to have a null meaning nothing is specified.
> We can create the null value with the default constructor, and detect
> whether it’s null using the operator bool.

OK, will change.

> I’m not sure how we decided in which cases to have a default value and in
> which cases callers have to explicitly pass { }. Since specifying a
> pre-determined identifier is fairly unusual, we should consider adding a
> default everywhere, and these calls to HTMLDocument::create would not have
> to change.

I had the HTMLDocument constructor parameter optional initially.
I discovered I had to pass it to TextDocument as well. At that point, I moved it to a mandatory parameter so that  I was sure to make am explicit decision for this parameter.
Comment 7 Radar WebKit Bug Importer 2022-01-19 03:48:16 PST
<rdar://problem/87763610>
Comment 8 youenn fablet 2022-01-19 04:58:34 PST
Created attachment 449475 [details]
Patch for landing
Comment 9 Darin Adler 2022-01-19 08:48:52 PST
Comment on attachment 449475 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=449475&action=review

> LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-get-resultingClientId.https-expected.txt:5
> +FAIL get(resultingClientId) for document sandboxed by CSP header assert_equals: promiseValue expected "undefinedValue" but got "client"

Do you know why this is now failing?
Comment 10 youenn fablet 2022-01-19 08:53:18 PST
(In reply to Darin Adler from comment #9)
> Comment on attachment 449475 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=449475&action=review
> 
> > LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/clients-get-resultingClientId.https-expected.txt:5
> > +FAIL get(resultingClientId) for document sandboxed by CSP header assert_equals: promiseValue expected "undefinedValue" but got "client"
> 
> Do you know why this is now failing?

Yes, we are starting to always reuse IDs while we were never doing it previously.
The test will pass after https://bugs.webkit.org/show_bug.cgi?id=235310.
Comment 11 EWS 2022-01-19 09:13:47 PST
Committed r288201 (246173@main): <https://commits.webkit.org/246173@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 449475 [details].