Bug 234238 - FetchResponse::clone should use the relevant realm for the cloned response
Summary: FetchResponse::clone should use the relevant realm for the cloned response
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-12-13 05:12 PST by youenn fablet
Modified: 2021-12-14 04:52 PST (History)
7 users (show)

See Also:


Attachments
Patch (5.40 KB, patch)
2021-12-13 05:18 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 2021-12-13 05:12:46 PST
FetchResponse::clone should use the relevant realm for the cloned response
Comment 1 youenn fablet 2021-12-13 05:18:57 PST
Created attachment 446991 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-12-13 05:19:55 PST
<rdar://problem/86407662>
Comment 3 Alexey Shvayka 2021-12-13 09:02:12 PST
Comment on attachment 446991 [details]
Patch

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

This is marvelous! I was looking into Fetch stuff during https://bugs.webkit.org/show_bug.cgi?id=230941 yet didn't come up with a test case.

With that said, are there any fetch methods we can fix in the same way? Namely the SEC FetchResponse's error() / redirect() are using to create another response?

> LayoutTests/http/wpt/fetch/clone-realm.html:1
> +<!doctype html>

There is no way we can WPT export this test, right?
Comment 4 youenn fablet 2021-12-13 10:46:11 PST
> With that said, are there any fetch methods we can fix in the same way?
> Namely the SEC FetchResponse's error() / redirect() are using to create
> another response?

These are static methods and I am not sure there are some observable behavior so maybe it is just fine

> > LayoutTests/http/wpt/fetch/clone-realm.html:1
> > +<!doctype html>
> 
> There is no way we can WPT export this test, right?

We could but, as written it only passes in Chrome and Safari, not Firefox.
Comment 5 Alexey Shvayka 2021-12-13 11:02:36 PST
(In reply to youenn fablet from comment #4)
> > With that said, are there any fetch methods we can fix in the same way?
> > Namely the SEC FetchResponse's error() / redirect() are using to create
> > another response?
> 
> These are static methods and I am not sure there are some observable
> behavior so maybe it is just fine

Oh, I've totally missed that. It's a bit weird that spec uses "this's relevant Realm" prose for non-platform objects.

In https://bugs.webkit.org/show_bug.cgi?id=230941 I've ignored both static methods and constructors regarding _current_ vs _relevant_. I guess you are right and there might be some weird test case involving Object.setPrototypeOf() for constructors. Not expecting anyone to investigate it though.

> 
> > > LayoutTests/http/wpt/fetch/clone-realm.html:1
> > > +<!doctype html>
> > 
> > There is no way we can WPT export this test, right?
> 
> We could but, as written it only passes in Chrome and Safari, not Firefox.

The test looks solid, and the spec normatively requires _relevant_ realm. I would say Firefox's failure is +1 reason to export.
Comment 6 youenn fablet 2021-12-13 11:41:49 PST
> > > > LayoutTests/http/wpt/fetch/clone-realm.html:1
> > > > +<!doctype html>
> > > 
> > > There is no way we can WPT export this test, right?
> > 
> > We could but, as written it only passes in Chrome and Safari, not Firefox.
> 
> The test looks solid, and the spec normatively requires _relevant_ realm. I
> would say Firefox's failure is +1 reason to export.

I'll check again but I think Firefox is generating a valid Response, whose body is in a somewhat special state, calling text() will return a promise that never resolves for instance.
Let's land this one for now and I'll follow up with a WPT proper upstream with WPT review.
Comment 7 EWS 2021-12-13 12:29:18 PST
Committed r286970 (245192@main): <https://commits.webkit.org/245192@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446991 [details].
Comment 8 youenn fablet 2021-12-14 04:52:06 PST
Digging further, Chrome is not always throwing when cloning in detached contexts.
This might create a backward compatibility risk so we might want to reduce our throwing as well.