Summary: | [Fetch API] Add a runtime flag to fetch API and related constructs | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, barraclough, buildbot, cdumez, commit-queue, darin, esprehn+autocc, ggaren, kondapallykalyan, mjs, rniwa | ||||||||||
Priority: | P2 | ||||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 156272, 156291 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
youenn fablet
2016-04-01 09:17:05 PDT
Our earliest reports were for sites using the Fetch polyfill at <https://github.com/github/fetch>. It sounds like those who have complained about this are just checking for presence of window.fetch. Created attachment 275469 [details]
Patch
Comment on attachment 275469 [details] Patch Attachment 275469 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1087515 Number of test failures exceeded the failure limit. Created attachment 275470 [details]
Archive of layout-test-results from ews103 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 275469 [details] Patch Attachment 275469 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/1087522 Number of test failures exceeded the failure limit. Created attachment 275471 [details]
Archive of layout-test-results from ews112 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 275480 [details]
Fixing mac wk1
(In reply to comment #2) > It sounds like those who have complained about this are just checking for > presence of window.fetch. Patch should take care of this as well as removing access to Request, Response and Headers. Comment on attachment 275480 [details] Fixing mac wk1 View in context: https://bugs.webkit.org/attachment.cgi?id=275480&action=review Looks good to me except I'm not sure about the JSDOMGlobalObject change. It feels like there ought to be a better way to not make the window.fetch object in the first place. I'd prefer someone else look at that. > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 > +#if ENABLE(FETCH_API) > + if (!RuntimeEnabledFeatures::sharedFeatures().fetchAPIEnabled()) { > + JSObject* prototype = getPrototypeDirect().getObject(); > + ASSERT(prototype); > + prototype->putDirect(vm, Identifier::fromString(&vm, "fetch"), jsUndefined()); > + } > +#endif I'm not sure this is correct. I'd prefer someone else look at this. > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundle.cpp:207 > + if (preference == "WebKitFetchAPIEnabled") These should probably all be else if. We now also need this to compile: Index: Source/JavaScriptCore/runtime/CommonIdentifiers.h =================================================================== --- Source/JavaScriptCore/runtime/CommonIdentifiers.h (revision 199071) +++ Source/JavaScriptCore/runtime/CommonIdentifiers.h (working copy) @@ -46,6 +46,7 @@ macro(GamepadButton) \ macro(GamepadEvent) \ macro(GeneratorFunction) \ + macro(Headers) \ macro(HTMLAudioElement) \ macro(HTMLSlotElement) \ macro(IDBCursor) \ @@ -76,6 +77,8 @@ macro(ReferenceError) \ macro(Reflect) \ macro(RegExp) \ + macro(Response) \ + macro(Request) \ macro(Set)\ macro(SetIterator)\ macro(ShadowRoot) \ (In reply to comment #10) > Comment on attachment 275480 [details] > Fixing mac wk1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275480&action=review > > Looks good to me except I'm not sure about the JSDOMGlobalObject change. It > feels like there ought to be a better way to not make the window.fetch > object in the first place. I'd prefer someone else look at that. > > > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 > > +#if ENABLE(FETCH_API) > > + if (!RuntimeEnabledFeatures::sharedFeatures().fetchAPIEnabled()) { > > + JSObject* prototype = getPrototypeDirect().getObject(); > > + ASSERT(prototype); > > + prototype->putDirect(vm, Identifier::fromString(&vm, "fetch"), jsUndefined()); > > + } > > +#endif > > I'm not sure this is correct. I'd prefer someone else look at this. It is consistent with the way other flags are handled. The binding generator is disabling runtime-flag methods in the generated finishCreation method. In the case of JSDOMGlobalObject, the binding generator is not generating the finishCreation method, hence why this code is added there. I also think the binding generator should handle runtime-flags consistently with compile flags. Currently runtime-flag disabled properties remain visible while compile-flag disabled properties are not visible. (In reply to comment #11) > We now also need this to compile: > > Index: Source/JavaScriptCore/runtime/CommonIdentifiers.h Do you know the reason of this? Committed with a fix me to http://trac.webkit.org/changeset/199081 (In reply to comment #13) > (In reply to comment #11) > > We now also need this to compile: > > > > Index: Source/JavaScriptCore/runtime/CommonIdentifiers.h > > Do you know the reason of this? It doesn't compile without it after https://bugs.webkit.org/show_bug.cgi?id=156136 (In reply to comment #12) > (In reply to comment #10) > > Comment on attachment 275480 [details] > > Fixing mac wk1 > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=275480&action=review > > > > Looks good to me except I'm not sure about the JSDOMGlobalObject change. It > > feels like there ought to be a better way to not make the window.fetch > > object in the first place. I'd prefer someone else look at that. > > > > > Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 > > > +#if ENABLE(FETCH_API) > > > + if (!RuntimeEnabledFeatures::sharedFeatures().fetchAPIEnabled()) { > > > + JSObject* prototype = getPrototypeDirect().getObject(); > > > + ASSERT(prototype); > > > + prototype->putDirect(vm, Identifier::fromString(&vm, "fetch"), jsUndefined()); > > > + } > > > +#endif > > > > I'm not sure this is correct. I'd prefer someone else look at this. > > It is consistent with the way other flags are handled. > The binding generator is disabling runtime-flag methods in the generated > finishCreation method. > In the case of JSDOMGlobalObject, the binding generator is not generating > the finishCreation method, hence why this code is added there. > > I also think the binding generator should handle runtime-flags consistently > with compile flags. > Currently runtime-flag disabled properties remain visible while compile-flag > disabled properties are not visible. This is not correct. window.__proto__.fetch returns undefined, but "fetch" in window.__proto__ still returns true. There is still a key called "fetch" in the window object, and this is *probably* not commonly used, but it needs to be fixed. Comment on attachment 275480 [details] Fixing mac wk1 View in context: https://bugs.webkit.org/attachment.cgi?id=275480&action=review >>>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 >>>> +#endif >>> >>> I'm not sure this is correct. I'd prefer someone else look at this. >> >> It is consistent with the way other flags are handled. >> The binding generator is disabling runtime-flag methods in the generated finishCreation method. >> In the case of JSDOMGlobalObject, the binding generator is not generating the finishCreation method, hence why this code is added there. >> >> I also think the binding generator should handle runtime-flags consistently with compile flags. >> Currently runtime-flag disabled properties remain visible while compile-flag disabled properties are not visible. > > This is not correct. window.__proto__.fetch returns undefined, but "fetch" in window.__proto__ still returns true. There is still a key called "fetch" in the window object, and this is *probably* not commonly used, but it needs to be fixed. Actually, this is not the way we usually disable operations at runtime, see the bindings generator: foreach my $function (@runtimeEnabledFunctions) { my $conditionalString = $codeGenerator->GenerateConditionalString($function->signature); push(@implContent, "#if ${conditionalString}\n") if $conditionalString; AddToImplIncludes("RuntimeEnabledFeatures.h"); my $signature = $function->signature; my $enable_function = GetRuntimeEnableFunctionName($signature); my $name = $signature->name; push(@implContent, " if (!${enable_function}()) {\n"); push(@implContent, " Identifier propertyName = Identifier::fromString(&vm, reinterpret_cast<const LChar*>(\"$name\"), strlen(\"$name\"));\n"); push(@implContent, " removeDirect(vm, propertyName);\n"); push(@implContent, " }\n"); push(@implContent, "#endif\n") if $conditionalString; } We *remove* it from the prototype in finishCreation(). Comment on attachment 275480 [details] Fixing mac wk1 View in context: https://bugs.webkit.org/attachment.cgi?id=275480&action=review >>>>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 >>>>> +#endif >>>> >>>> I'm not sure this is correct. I'd prefer someone else look at this. >>> >>> It is consistent with the way other flags are handled. >>> The binding generator is disabling runtime-flag methods in the generated finishCreation method. >>> In the case of JSDOMGlobalObject, the binding generator is not generating the finishCreation method, hence why this code is added there. >>> >>> I also think the binding generator should handle runtime-flags consistently with compile flags. >>> Currently runtime-flag disabled properties remain visible while compile-flag disabled properties are not visible. >> >> This is not correct. window.__proto__.fetch returns undefined, but "fetch" in window.__proto__ still returns true. There is still a key called "fetch" in the window object, and this is *probably* not commonly used, but it needs to be fixed. > > Actually, this is not the way we usually disable operations at runtime, see the bindings generator: > foreach my $function (@runtimeEnabledFunctions) { > my $conditionalString = $codeGenerator->GenerateConditionalString($function->signature); > push(@implContent, "#if ${conditionalString}\n") if $conditionalString; > AddToImplIncludes("RuntimeEnabledFeatures.h"); > my $signature = $function->signature; > my $enable_function = GetRuntimeEnableFunctionName($signature); > my $name = $signature->name; > push(@implContent, " if (!${enable_function}()) {\n"); > push(@implContent, " Identifier propertyName = Identifier::fromString(&vm, reinterpret_cast<const LChar*>(\"$name\"), strlen(\"$name\"));\n"); > push(@implContent, " removeDirect(vm, propertyName);\n"); > push(@implContent, " }\n"); > push(@implContent, "#endif\n") if $conditionalString; > } > > We *remove* it from the prototype in finishCreation(). What's missing in our bindings generator to support this BTW? We generally support RuntimeEnabled operations I thought. Comment on attachment 275480 [details] Fixing mac wk1 View in context: https://bugs.webkit.org/attachment.cgi?id=275480&action=review >>>>>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 >>>>>> +#endif >>>>> >>>>> I'm not sure this is correct. I'd prefer someone else look at this. >>>> >>>> It is consistent with the way other flags are handled. >>>> The binding generator is disabling runtime-flag methods in the generated finishCreation method. >>>> In the case of JSDOMGlobalObject, the binding generator is not generating the finishCreation method, hence why this code is added there. >>>> >>>> I also think the binding generator should handle runtime-flags consistently with compile flags. >>>> Currently runtime-flag disabled properties remain visible while compile-flag disabled properties are not visible. >>> >>> This is not correct. window.__proto__.fetch returns undefined, but "fetch" in window.__proto__ still returns true. There is still a key called "fetch" in the window object, and this is *probably* not commonly used, but it needs to be fixed. >> >> Actually, this is not the way we usually disable operations at runtime, see the bindings generator: >> foreach my $function (@runtimeEnabledFunctions) { >> my $conditionalString = $codeGenerator->GenerateConditionalString($function->signature); >> push(@implContent, "#if ${conditionalString}\n") if $conditionalString; >> AddToImplIncludes("RuntimeEnabledFeatures.h"); >> my $signature = $function->signature; >> my $enable_function = GetRuntimeEnableFunctionName($signature); >> my $name = $signature->name; >> push(@implContent, " if (!${enable_function}()) {\n"); >> push(@implContent, " Identifier propertyName = Identifier::fromString(&vm, reinterpret_cast<const LChar*>(\"$name\"), strlen(\"$name\"));\n"); >> push(@implContent, " removeDirect(vm, propertyName);\n"); >> push(@implContent, " }\n"); >> push(@implContent, "#endif\n") if $conditionalString; >> } >> >> We *remove* it from the prototype in finishCreation(). > > What's missing in our bindings generator to support this BTW? We generally support RuntimeEnabled operations I thought. Not to mention, we probably do not want Window-specific code in JSDOMGlobalObject.cpp. Comment on attachment 275480 [details] Fixing mac wk1 View in context: https://bugs.webkit.org/attachment.cgi?id=275480&action=review >>>>>>> Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:122 >>>>>>> +#endif >>>>>> >>>>>> I'm not sure this is correct. I'd prefer someone else look at this. >>>>> >>>>> It is consistent with the way other flags are handled. >>>>> The binding generator is disabling runtime-flag methods in the generated finishCreation method. >>>>> In the case of JSDOMGlobalObject, the binding generator is not generating the finishCreation method, hence why this code is added there. >>>>> >>>>> I also think the binding generator should handle runtime-flags consistently with compile flags. >>>>> Currently runtime-flag disabled properties remain visible while compile-flag disabled properties are not visible. >>>> >>>> This is not correct. window.__proto__.fetch returns undefined, but "fetch" in window.__proto__ still returns true. There is still a key called "fetch" in the window object, and this is *probably* not commonly used, but it needs to be fixed. >>> >>> Actually, this is not the way we usually disable operations at runtime, see the bindings generator: >>> foreach my $function (@runtimeEnabledFunctions) { >>> my $conditionalString = $codeGenerator->GenerateConditionalString($function->signature); >>> push(@implContent, "#if ${conditionalString}\n") if $conditionalString; >>> AddToImplIncludes("RuntimeEnabledFeatures.h"); >>> my $signature = $function->signature; >>> my $enable_function = GetRuntimeEnableFunctionName($signature); >>> my $name = $signature->name; >>> push(@implContent, " if (!${enable_function}()) {\n"); >>> push(@implContent, " Identifier propertyName = Identifier::fromString(&vm, reinterpret_cast<const LChar*>(\"$name\"), strlen(\"$name\"));\n"); >>> push(@implContent, " removeDirect(vm, propertyName);\n"); >>> push(@implContent, " }\n"); >>> push(@implContent, "#endif\n") if $conditionalString; >>> } >>> >>> We *remove* it from the prototype in finishCreation(). >> >> What's missing in our bindings generator to support this BTW? We generally support RuntimeEnabled operations I thought. > > Not to mention, we probably do not want Window-specific code in JSDOMGlobalObject.cpp. I have just checked and it seems our RuntimeEnabled support for operation does not seem to work for non-window object. That said, Window should not need any special treatment here so I don't see why we could not generate the same code for the Window. Hack fixed properly in https://bugs.webkit.org/show_bug.cgi?id=156272 Thanks Chris! (In reply to comment #21) > Hack fixed properly in https://bugs.webkit.org/show_bug.cgi?id=156272 > Thanks Chris! Thanks to both of you for landing the patch :) > Not to mention, we probably do not want Window-specific code in > JSDOMGlobalObject.cpp. fetch is exposed to Worker as well, hence why I put the code in JSDOMGlobalObject.cpp. If bug 156272 is only about DOMWindow, fetch will still be exposed in Workers. > > I also think the binding generator should handle runtime-flags consistently
> > with compile flags.
> > Currently runtime-flag disabled properties remain visible while compile-flag
> > disabled properties are not visible.
> This is not correct. window.__proto__.fetch returns undefined, but "fetch"
> in window.__proto__ still returns true. There is still a key called "fetch"
> in the window object, and this is *probably* not commonly used, but it needs
> to be fixed.
Agreed. I was meaning that "fetch" was treated similarly to "Headers", "Request" and "Response" which were visible even though returning "undefined" when being accessed while runtime-flag disabled.
The issue for these constructors seems to have been solved recently (not sure exactly when), which is very good.
Once fetch in worker is no longer accessible, it seems to me that there will be no difference between disabling the fetch API using the compile flag and using the runtime flag. If so, can we get rid off the FETCH_API compile flag?
(In reply to comment #23) > > Not to mention, we probably do not want Window-specific code in > > JSDOMGlobalObject.cpp. > > fetch is exposed to Worker as well, hence why I put the code in > JSDOMGlobalObject.cpp. > > If bug 156272 is only about DOMWindow, fetch will still be exposed in > Workers. Right, I missed that fetch was exposed to workers as well. I will extend my new support for [EnabledAtRuntime] attributes / operations from DOMWindow to any global object then. This should not be too difficult, so I'll try and make this happen today. (In reply to comment #24) > > > I also think the binding generator should handle runtime-flags consistently > > > with compile flags. > > > Currently runtime-flag disabled properties remain visible while compile-flag > > > disabled properties are not visible. > > This is not correct. window.__proto__.fetch returns undefined, but "fetch" > > in window.__proto__ still returns true. There is still a key called "fetch" > > in the window object, and this is *probably* not commonly used, but it needs > > to be fixed. > > Agreed. I was meaning that "fetch" was treated similarly to "Headers", > "Request" and "Response" which were visible even though returning > "undefined" when being accessed while runtime-flag disabled. > The issue for these constructors seems to have been solved recently (not > sure exactly when), which is very good. Yes, I fixed it recently in r199017. > > Once fetch in worker is no longer accessible, it seems to me that there will > be no difference between disabling the fetch API using the compile flag and > using the runtime flag. If so, can we get rid off the FETCH_API compile flag? fetch() is no longer accessible in workers as of r199103. I cannot comment about the removal of the compile time flag though, maybe Alex can. (In reply to comment #26) > I cannot comment about the removal of the compile time flag though, maybe > Alex can. I think we should leave the compile time flag while it's under development. |