Bug 159672

Created attachment 283403 [details] Patch

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

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

Created attachment 283407 [details] Removing WK1 expectation

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?

(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.

Ping review?

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 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

Created attachment 284096 [details] Patch

Created attachment 284099 [details] Patch

Created attachment 284327 [details] Rebasing

Created attachment 284329 [details] Fixing CMake build

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 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.

Created attachment 284408 [details] Patch for landing

Comment on attachment 284408 [details] Patch for landing Clearing flags on attachment: 284408 Committed r203641: <http://trac.webkit.org/changeset/203641>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 160116

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 on attachment 284408 [details] Patch for landing Clearing flags on attachment: 284408 Committed r203675: <http://trac.webkit.org/changeset/203675>

All reviewed patches have been landed. Closing bug.