Bug 156113

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 Flags
Patch
none
Archive of layout-test-results from ews103 for mac-yosemite
none
Archive of layout-test-results from ews112 for mac-yosemite
none
Fixing mac wk1 cdumez: review-

Description youenn fablet 2016-04-01 09:17:05 PDT
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.
Comment 1 Darin Adler 2016-04-01 09:29:16 PDT
Our earliest reports were for sites using the Fetch polyfill at <https://github.com/github/fetch>.
Comment 2 Maciej Stachowiak 2016-04-01 11:46:14 PDT
It sounds like those who have complained about this are just checking for presence of window.fetch.
Comment 3 youenn fablet 2016-04-02 09:11:19 PDT
Created attachment 275469 [details]
Patch
Comment 4 Build Bot 2016-04-02 09:53:15 PDT
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.
Comment 5 Build Bot 2016-04-02 09:53:18 PDT
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 6 Build Bot 2016-04-02 10:04:17 PDT
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.
Comment 7 Build Bot 2016-04-02 10:04:20 PDT
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
Comment 8 youenn fablet 2016-04-02 13:33:49 PDT
Created attachment 275480 [details]
Fixing mac wk1
Comment 9 youenn fablet 2016-04-02 13:34:35 PDT
(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 10 Alex Christensen 2016-04-04 15:10:55 PDT
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.
Comment 11 Alex Christensen 2016-04-05 15:16:25 PDT
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) \
Comment 12 youenn fablet 2016-04-05 15:50:28 PDT
(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.
Comment 13 youenn fablet 2016-04-05 15:50:54 PDT
(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?
Comment 14 Alex Christensen 2016-04-05 16:23:56 PDT
Committed with a fix me to http://trac.webkit.org/changeset/199081
Comment 15 Alex Christensen 2016-04-05 16:25:43 PDT
(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
Comment 16 Alex Christensen 2016-04-05 16:27:25 PDT
(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 17 Chris Dumez 2016-04-05 16:34:08 PDT
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 18 Chris Dumez 2016-04-05 16:34:58 PDT
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 19 Chris Dumez 2016-04-05 16:36:46 PDT
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 20 Chris Dumez 2016-04-05 16:43:39 PDT
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.
Comment 21 Alex Christensen 2016-04-05 22:16:17 PDT
Hack fixed properly in https://bugs.webkit.org/show_bug.cgi?id=156272
Thanks Chris!
Comment 22 youenn fablet 2016-04-06 00:30:58 PDT
(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 :)
Comment 23 youenn fablet 2016-04-06 00:31:43 PDT
> 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.
Comment 24 youenn fablet 2016-04-06 03:57:08 PDT
> > 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?
Comment 25 Chris Dumez 2016-04-06 08:00:42 PDT
(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.
Comment 26 Chris Dumez 2016-04-06 11:07:39 PDT
(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.
Comment 27 Alex Christensen 2016-04-07 09:13:31 PDT
(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.