Bug 159672 - [Fetch API] Request should be created with any HeadersInit data
Summary: [Fetch API] Request should be created with any HeadersInit data
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:
Depends on: 159932 160116
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-07-12 05:16 PDT by youenn fablet
Modified: 2016-07-24 23:29 PDT (History)
9 users (show)

See Also:


Attachments
Patch (9.66 KB, patch)
2016-07-12 05:58 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (887.26 KB, application/zip)
2016-07-12 06:58 PDT, Build Bot
no flags Details
Removing WK1 expectation (11.08 KB, patch)
2016-07-12 07:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (45.13 KB, patch)
2016-07-20 05:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (46.55 KB, patch)
2016-07-20 06:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (46.68 KB, patch)
2016-07-22 05:59 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing CMake build (46.68 KB, patch)
2016-07-22 07:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (46.57 KB, patch)
2016-07-23 00:47 PDT, 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 2016-07-12 05:16:11 PDT
Currently, it only supports Headers objects
Comment 1 youenn fablet 2016-07-12 05:58:10 PDT
Created attachment 283403 [details]
Patch
Comment 2 Build Bot 2016-07-12 06:58:24 PDT
Comment on attachment 283403 [details]
Patch

Attachment 283403 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1668591

New failing tests:
imported/w3c/web-platform-tests/fetch/api/credentials/authentication-basic.html
Comment 3 Build Bot 2016-07-12 06:58:28 PDT
Created attachment 283406 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 4 youenn fablet 2016-07-12 07:03:09 PDT
Created attachment 283407 [details]
Removing WK1 expectation
Comment 5 Alex Christensen 2016-07-12 11:10:20 PDT
Comment on attachment 283407 [details]
Removing WK1 expectation

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

> Source/WebCore/bindings/js/JSDictionary.cpp:259
> +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));

Is it possible to do anything tricky with the prototype to make this give us the wrong constructor?  Is that correct?
Comment 6 youenn fablet 2016-07-12 23:50:25 PDT
(In reply to comment #5)
> Comment on attachment 283407 [details]
> Removing WK1 expectation
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283407&action=review
> 
> > Source/WebCore/bindings/js/JSDictionary.cpp:259
> > +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));
> 
> Is it possible to do anything tricky with the prototype to make this give us
> the wrong constructor?  Is that correct?

I am not sure to follow.

This code is what is used to create a Headers constructor object when calling window.Header or window.@Header. Or when accessing to the constructor from Headers prototype.

The constructor object is not created upfront but only when being accessed the first time. I don't think user script can modify the related JSGlobalObject map to add wrong entries.

User script may modify window prototype, override window.Header or change Header.prototype, add new properties to Header constructor.
But these lines should remain functional.
Comment 7 youenn fablet 2016-07-19 05:14:21 PDT
Ping review?
Comment 8 Chris Dumez 2016-07-19 08:58:13 PDT
Comment on attachment 283407 [details]
Removing WK1 expectation

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

>>> Source/WebCore/bindings/js/JSDictionary.cpp:259
>>> +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));
>> 
>> Is it possible to do anything tricky with the prototype to make this give us the wrong constructor?  Is that correct?
> 
> I am not sure to follow.
> 
> This code is what is used to create a Headers constructor object when calling window.Header or window.@Header. Or when accessing to the constructor from Headers prototype.
> 
> The constructor object is not created upfront but only when being accessed the first time. I don't think user script can modify the related JSGlobalObject map to add wrong entries.
> 
> User script may modify window prototype, override window.Header or change Header.prototype, add new properties to Header constructor.
> But these lines should remain functional.

It seems wasteful to construct a temporary JSFetchHeaders object. Can we construct a FetchHeaders directly from he JSValue without going through a JSFetchHeaders?
Comment 9 youenn fablet 2016-07-19 09:21:13 PDT
Comment on attachment 283407 [details]
Removing WK1 expectation

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

>>>> Source/WebCore/bindings/js/JSDictionary.cpp:259
>>>> +        JSValue headersConstructor = JSFetchHeaders::getConstructor(exec->vm(), jsCast<JSDOMGlobalObject*>(exec->lexicalGlobalObject()));
>>> 
>>> Is it possible to do anything tricky with the prototype to make this give us the wrong constructor?  Is that correct?
>> 
>> I am not sure to follow.
>> 
>> This code is what is used to create a Headers constructor object when calling window.Header or window.@Header. Or when accessing to the constructor from Headers prototype.
>> 
>> The constructor object is not created upfront but only when being accessed the first time. I don't think user script can modify the related JSGlobalObject map to add wrong entries.
>> 
>> User script may modify window prototype, override window.Header or change Header.prototype, add new properties to Header constructor.
>> But these lines should remain functional.
> 
> It seems wasteful to construct a temporary JSFetchHeaders object. Can we construct a FetchHeaders directly from he JSValue without going through a JSFetchHeaders?

OK, let's create a JS-builtin that will implement https://fetch.spec.whatwg.org/#concept-headers-fill
Comment 10 youenn fablet 2016-07-20 05:33:32 PDT
Created attachment 284096 [details]
Patch
Comment 11 youenn fablet 2016-07-20 06:24:46 PDT
Created attachment 284099 [details]
Patch
Comment 12 youenn fablet 2016-07-22 05:59:38 PDT
Created attachment 284327 [details]
Rebasing
Comment 13 youenn fablet 2016-07-22 07:11:03 PDT
Created attachment 284329 [details]
Fixing CMake build
Comment 14 Sam Weinig 2016-07-22 14:18:32 PDT
Comment on attachment 284329 [details]
Fixing CMake build

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

> Source/WebCore/Modules/fetch/WorkerGlobalScopeFetch.idl:33
> +    [JSBuiltin] Promise fetch(any input, optional Dictionary init);

Is there anyway to take advantage of our WebIDL Dictionary support for init?
Comment 15 youenn fablet 2016-07-23 00:38:08 PDT
Comment on attachment 284329 [details]
Fixing CMake build

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

>> Source/WebCore/Modules/fetch/WorkerGlobalScopeFetch.idl:33
>> +    [JSBuiltin] Promise fetch(any input, optional Dictionary init);
> 
> Is there anyway to take advantage of our WebIDL Dictionary support for init?

The init dictionary is passed to FetchRequest constructor, which was and still is heavily based on using Dictionary (see FetchRequest::initializeWith).
That said, moving part of this initializeWith code in FetchRequest JS built-in might ease maintenance and readability.
Comment 16 youenn fablet 2016-07-23 00:47:26 PDT
Created attachment 284408 [details]
Patch for landing
Comment 17 WebKit Commit Bot 2016-07-23 01:56:39 PDT
Comment on attachment 284408 [details]
Patch for landing

Clearing flags on attachment: 284408

Committed r203641: <http://trac.webkit.org/changeset/203641>
Comment 18 WebKit Commit Bot 2016-07-23 01:56:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Commit Bot 2016-07-23 02:51:50 PDT
Re-opened since this is blocked by bug 160116
Comment 20 WebKit Commit Bot 2016-07-23 02:53:56 PDT
Comment on attachment 284408 [details]
Patch for landing

Rejecting attachment 284408 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 284408, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
0000000000000000
|--- LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/fetch/api/credentials/authentication-basic-expected.txt	(revision 0)
|+++ LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/fetch/api/credentials/authentication-basic-expected.txt	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.webkit.org/results/1738806
Comment 21 WebKit Commit Bot 2016-07-24 23:29:39 PDT
Comment on attachment 284408 [details]
Patch for landing

Clearing flags on attachment: 284408

Committed r203675: <http://trac.webkit.org/changeset/203675>
Comment 22 WebKit Commit Bot 2016-07-24 23:29:48 PDT
All reviewed patches have been landed.  Closing bug.