Bug 234238

Summary: FetchResponse::clone should use the relevant realm for the cloned response
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ashvayka, cdumez, esprehn+autocc, ews-watchlist, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=230941
Attachments:
Description Flags
Patch none

youenn fablet
Reported 2021-12-13 05:12:46 PST
FetchResponse::clone should use the relevant realm for the cloned response
Attachments
Patch (5.40 KB, patch)
2021-12-13 05:18 PST, youenn fablet 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
youenn fablet
Comment 1 2021-12-13 05:18:57 PST
Created attachment 446991 [details] Patch
Radar WebKit Bug Importer
Comment 2 2021-12-13 05:19:55 PST
<rdar://problem/86407662>
Alexey Shvayka
Comment 3 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?
youenn fablet
Comment 4 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.
Alexey Shvayka
Comment 5 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.
youenn fablet
Comment 6 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.
EWS
Comment 7 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].
youenn fablet
Comment 8 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.
Note You need to log in before you can comment on or make changes to this bug.