Bug 156113 – [Fetch API] Add a runtime flag to fetch API and related constructs

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Following on https://lists.webkit.org/pipermail/webkit-dev/2016-March/028108.html, it might be good to have the fetch API being enabled/disabled at runtime.

[Darin Adler]

Our earliest reports were for sites using the Fetch polyfill at <https://github.com/github/fetch>.

[Maciej Stachowiak]

It sounds like those who have complained about this are just checking for presence of window.fetch.

[youenn fablet]

Created attachment 275469 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[youenn fablet]

Created attachment 275480 [details]
Fixing mac wk1

[youenn fablet]

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

[Alex Christensen]

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.

[Alex Christensen]

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

[youenn fablet]

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

[youenn fablet]

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

[Alex Christensen]

Committed with a fix me to http://trac.webkit.org/changeset/199081

[Alex Christensen]

(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

[Alex Christensen]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Alex Christensen]

Hack fixed properly in https://bugs.webkit.org/show_bug.cgi?id=156272
Thanks Chris!

[youenn fablet]

(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 :)

[youenn fablet]

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

[youenn fablet]

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

[Chris Dumez]

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

[Chris Dumez]

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

[Alex Christensen]

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