Bug 154729 - [Fetch API] Make FetchRequest and FetchResponse ActiveDOMObject
Summary: [Fetch API] Make FetchRequest and FetchResponse ActiveDOMObject
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: 155022
Blocks: 151937 154959
  Show dependency treegraph
 
Reported: 2016-02-26 09:50 PST by youenn fablet
Modified: 2016-03-08 01:10 PST (History)
7 users (show)

See Also:


Attachments
Patch (56.27 KB, patch)
2016-02-26 09:57 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (56.68 KB, patch)
2016-02-29 09:59 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (56.99 KB, patch)
2016-03-03 00:30 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch based on fix 155022 (14.44 KB, patch)
2016-03-04 09:49 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Updating style (14.31 KB, patch)
2016-03-06 23:03 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (14.29 KB, patch)
2016-03-08 00:12 PST, 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-02-26 09:50:26 PST
FetchRequest and FetchResponse may need to trigger loading of resources.
This is limited to blob data for FetchRequest.
FetchResponse might also load blob data and may also receive data from loaders.
Comment 1 youenn fablet 2016-02-26 09:57:28 PST
Created attachment 272331 [details]
Patch
Comment 2 WebKit Commit Bot 2016-02-26 10:00:18 PST
Attachment 272331 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructorAndCallWith.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestClassWithJSBuiltinConstructorAndCallWith.cpp:181:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 youenn fablet 2016-02-29 09:59:55 PST
Created attachment 272498 [details]
Patch
Comment 4 WebKit Commit Bot 2016-02-29 10:01:02 PST
Attachment 272498 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructorAndCallWith.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestClassWithJSBuiltinConstructorAndCallWith.cpp:181:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Alex Christensen 2016-03-01 09:32:14 PST
Comment on attachment 272498 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchResponse.cpp:165
> +    return true;

Does the comment in XMLHttpRequest::canSuspendForDocumentSuspension apply here?

> Source/WebCore/bindings/js/JSDOMConstructor.h:224
> +    if (!object)
> +        return throwConstructorDocumentUnavailableError(*state, info()->className);

Why is this needed now for the fetch api and it wasn't needed before?

> Source/WebCore/bindings/scripts/test/TestClassWithJSBuiltinConstructorAndCallWith.idl:2
> + * Copyright (C) 2015 Canon Inc.

2016
Comment 6 youenn fablet 2016-03-01 11:32:29 PST
(In reply to comment #5)
> Comment on attachment 272498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272498&action=review
> 
> > Source/WebCore/Modules/fetch/FetchResponse.cpp:165
> > +    return true;
> 
> Does the comment in XMLHttpRequest::canSuspendForDocumentSuspension apply
> here?

It seems to make sense when doing network loads from fetch.
For blob (i.e. converting a blob stored in a body as a string or array buffer), I am less sure about it.
Plan is to implement Blob conversion first, using the simple approach (if loading, do not suspend).
This should be revisited when doing network loads.

> 
> > Source/WebCore/bindings/js/JSDOMConstructor.h:224
> > +    if (!object)
> > +        return throwConstructorDocumentUnavailableError(*state, info()->className);
> 
> Why is this needed now for the fetch api and it wasn't needed before?

This mimicks the checks for regular constructors called with ScriptExecutionContext.
This check was not needed as createJSObject could not return a null pointer in any valid case before that patch.

> > Source/WebCore/bindings/scripts/test/TestClassWithJSBuiltinConstructorAndCallWith.idl:2
> > + * Copyright (C) 2015 Canon Inc.
> 
> 2016

OK
Comment 7 youenn fablet 2016-03-01 11:44:35 PST
> It seems to make sense when doing network loads from fetch.
> For blob (i.e. converting a blob stored in a body as a string or array
> buffer), I am less sure about it.
> Plan is to implement Blob conversion first, using the simple approach (if
> loading, do not suspend).
> This should be revisited when doing network loads.

This probably only applies to FetchResponse, FetchRequest having only to handle Blob.
Comment 8 youenn fablet 2016-03-03 00:30:31 PST
Created attachment 272740 [details]
Rebasing
Comment 9 WebKit Commit Bot 2016-03-03 00:31:44 PST
Attachment 272740 [details] did not pass style-queue:


ERROR: Source/WebCore/bindings/scripts/test/ObjC/DOMTestClassWithJSBuiltinConstructorAndCallWith.mm:30:  Found header this file implements after other header. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
ERROR: Source/WebCore/bindings/scripts/test/JS/JSTestClassWithJSBuiltinConstructorAndCallWith.cpp:181:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 youenn fablet 2016-03-03 00:33:12 PST
Thanks for the feedback.
I rebased the patch and made some updates according your comments/questions.

(In reply to comment #5)
> Comment on attachment 272498 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272498&action=review
> 
> > Source/WebCore/Modules/fetch/FetchResponse.cpp:165
> > +    return true;
> 
> Does the comment in XMLHttpRequest::canSuspendForDocumentSuspension apply
> here?

Added some info in Changelog about suspending request and response.

> 
> > Source/WebCore/bindings/js/JSDOMConstructor.h:224
> > +    if (!object)
> > +        return throwConstructorDocumentUnavailableError(*state, info()->className);
> 
> Why is this needed now for the fetch api and it wasn't needed before?

Added some info in the Changelog also.

> 
> > Source/WebCore/bindings/scripts/test/TestClassWithJSBuiltinConstructorAndCallWith.idl:2
> > + * Copyright (C) 2015 Canon Inc.
> 
> 2016

Fixed.
Comment 11 Alex Christensen 2016-03-03 01:35:08 PST
Comment on attachment 272740 [details]
Rebasing

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

> Source/WebCore/bindings/js/JSDOMConstructor.h:224
> +    if (!object)
> +        return throwConstructorDocumentUnavailableError(*state, info()->className);

This patch looks good to me except this.  This adds a branch to some pretty hot code, and I imagine it would significantly affect performance.  I'd like someone more familiar with the bindings to look at this.  If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings.
Comment 12 youenn fablet 2016-03-03 02:51:55 PST
Comment on attachment 272740 [details]
Rebasing

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

>> Source/WebCore/bindings/js/JSDOMConstructor.h:224
>> +        return throwConstructorDocumentUnavailableError(*state, info()->className);
> 
> This patch looks good to me except this.  This adds a branch to some pretty hot code, and I imagine it would significantly affect performance.  I'd like someone more familiar with the bindings to look at this.  If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings.

It indeed adds a check for all DOM objects having a JS built-in constructor, while there were none previously.
It has no effect for other DOM objects though.
The number of JS built-in constructed DOM classes is fairly low at the moment but it may increase in the future.

The ususal way of fixing this would be to update the binding generator to generate that check when needed but that might require the binding generator to generate the whole JSBuiltinConstructor::construct.
An alternative would be to update JSBuiltinConstructor by adding a template/compile-time boolean parameter to activate or not this check.
If that is found worthwhile, I can tackle this as a follow-up patch.
Comment 13 youenn fablet 2016-03-03 06:14:03 PST
Comment on attachment 272740 [details]
Rebasing

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

> Source/WebCore/Modules/fetch/FetchRequest.h:87
> +    FetchRequest(FetchBody&&, Ref<FetchHeaders>&&, InternalRequest&&, ScriptExecutionContext&);

ScriptExecutionContext& seems usually to be the first constructor parameter.
I plan to update this in a next patch or when landing this one.
Comment 14 Darin Adler 2016-03-03 08:50:06 PST
Comment on attachment 272740 [details]
Rebasing

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

>> Source/WebCore/Modules/fetch/FetchRequest.h:87
>> +    FetchRequest(FetchBody&&, Ref<FetchHeaders>&&, InternalRequest&&, ScriptExecutionContext&);
> 
> ScriptExecutionContext& seems usually to be the first constructor parameter.
> I plan to update this in a next patch or when landing this one.

Contexts like this one, GraphicsContext, ExecState, Document, Frame, Page, etc. are conventionally the first parameter to any function. That’s the pattern you are seeing and considering following.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:54
> -Ref<FetchResponse> FetchResponse::error()
> +Ref<FetchResponse> FetchResponse::error(ScriptExecutionContext* context)
>  {
> -    return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse()));
> +    return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context));
>  }

This code assumes the context is non-null, so the argument should be ScriptExecutionContext&. If it can’t be a reference, then instead we at least have to figure out where the guarantee that the argument is non-null comes from.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:68
> +    RefPtr<FetchResponse> redirectResponse = adoptRef(*new FetchResponse(Type::Default, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context));

Same issue here.

> Source/WebCore/Modules/fetch/FetchResponse.cpp:127
> +    RefPtr<FetchResponse> cloned = adoptRef(*new FetchResponse(m_type, FetchBody(m_body), FetchHeaders::create(headers()), ResourceResponse(m_response), *scriptExecutionContext()));

Variation on the same issue here. What guarantees scriptExecutionContext() is non-null, and if nothing does, then why does it return a pointer rather than a reference?

>>> Source/WebCore/bindings/js/JSDOMConstructor.h:224
>>> +        return throwConstructorDocumentUnavailableError(*state, info()->className);
>> 
>> This patch looks good to me except this.  This adds a branch to some pretty hot code, and I imagine it would significantly affect performance.  I'd like someone more familiar with the bindings to look at this.  If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings.
> 
> It indeed adds a check for all DOM objects having a JS built-in constructor, while there were none previously.
> It has no effect for other DOM objects though.
> The number of JS built-in constructed DOM classes is fairly low at the moment but it may increase in the future.
> 
> The ususal way of fixing this would be to update the binding generator to generate that check when needed but that might require the binding generator to generate the whole JSBuiltinConstructor::construct.
> An alternative would be to update JSBuiltinConstructor by adding a template/compile-time boolean parameter to activate or not this check.
> If that is found worthwhile, I can tackle this as a follow-up patch.

Another way to do it is to change the createJSObject function so that in some classes it returns a pointer and in others it returns a reference. Then we can use a private member function that is overloaded:

    return callConstructor(*state, castedThis->createJSObject(), *castedThis->initializeFunction());

There are two inline callConstructor functions, one that takes a JSObject& and another that takes a JSObject*. The JSObject* one does this:

    if (!object)
        return throwConstructorDocumentUnavailableError(state, info()->className);
    return callConstructor(state, object, function);

And the JSObject& one does this:

    callFunctionWithCurrentArguments(state, object,function);
    return JSC::JSValue::encode(&object);

All we have to do is make the createJSObject function return a reference when known to be non-null, and a pointer when it might be null.
Comment 15 youenn fablet 2016-03-03 23:37:19 PST
> > Source/WebCore/Modules/fetch/FetchResponse.cpp:54
> > -Ref<FetchResponse> FetchResponse::error()
> > +Ref<FetchResponse> FetchResponse::error(ScriptExecutionContext* context)
> >  {
> > -    return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse()));
> > +    return adoptRef(*new FetchResponse(Type::Error, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context));
> >  }
> 
> This code assumes the context is non-null, so the argument should be
> ScriptExecutionContext&. If it can’t be a reference, then instead we at
> least have to figure out where the guarantee that the argument is non-null
> comes from.

The binding generated code does the check and should pass a reference.
This is done for constructors but not yet for methods.

> > Source/WebCore/Modules/fetch/FetchResponse.cpp:68
> > +    RefPtr<FetchResponse> redirectResponse = adoptRef(*new FetchResponse(Type::Default, { }, FetchHeaders::create(FetchHeaders::Guard::Immutable), ResourceResponse(), *context));
> 
> Same issue here.

Ditto.

> > Source/WebCore/Modules/fetch/FetchResponse.cpp:127
> > +    RefPtr<FetchResponse> cloned = adoptRef(*new FetchResponse(m_type, FetchBody(m_body), FetchHeaders::create(headers()), ResourceResponse(m_response), *scriptExecutionContext()));
> 
> Variation on the same issue here. What guarantees scriptExecutionContext()
> is non-null, and if nothing does, then why does it return a pointer rather
> than a reference?

Right, for consistency, clone should be set as CallWith=ScriptExecutionContext
I will fix that as a follow-up patch.

> >>> Source/WebCore/bindings/js/JSDOMConstructor.h:224
> >>> +        return throwConstructorDocumentUnavailableError(*state, info()->className);
> >> 
> >> This patch looks good to me except this.  This adds a branch to some pretty hot code, and I imagine it would significantly affect performance.  I'd like someone more familiar with the bindings to look at this.  If this is indeed newly needed, then I think there should be a new inline function that has this branch so we don't add this branch to all existing bindings.
> > 
> > It indeed adds a check for all DOM objects having a JS built-in constructor, while there were none previously.
> > It has no effect for other DOM objects though.
> > The number of JS built-in constructed DOM classes is fairly low at the moment but it may increase in the future.
> > 
> > The ususal way of fixing this would be to update the binding generator to generate that check when needed but that might require the binding generator to generate the whole JSBuiltinConstructor::construct.
> > An alternative would be to update JSBuiltinConstructor by adding a template/compile-time boolean parameter to activate or not this check.
> > If that is found worthwhile, I can tackle this as a follow-up patch.
> 
> Another way to do it is to change the createJSObject function so that in
> some classes it returns a pointer and in others it returns a reference. Then
> we can use a private member function that is overloaded:
> 
>     return callConstructor(*state, castedThis->createJSObject(),
> *castedThis->initializeFunction());
> 
> There are two inline callConstructor functions, one that takes a JSObject&
> and another that takes a JSObject*. The JSObject* one does this:
> 
>     if (!object)
>         return throwConstructorDocumentUnavailableError(state,
> info()->className);
>     return callConstructor(state, object, function);
> 
> And the JSObject& one does this:
> 
>     callFunctionWithCurrentArguments(state, object,function);
>     return JSC::JSValue::encode(&object);
> 
> All we have to do is make the createJSObject function return a reference
> when known to be non-null, and a pointer when it might be null.

I will handle this as a separated bug.
Comment 16 youenn fablet 2016-03-04 09:49:48 PST
Created attachment 273007 [details]
Patch based on fix 155022
Comment 17 youenn fablet 2016-03-06 23:03:16 PST
Created attachment 273162 [details]
Updating style
Comment 18 Darin Adler 2016-03-07 09:56:23 PST
Comment on attachment 273162 [details]
Updating style

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

> Source/WebCore/Modules/fetch/FetchRequest.h:91
> +    const char* activeDOMObjectName() const override;
> +    bool canSuspendForDocumentSuspension() const override;

Should say final instead of override.

> Source/WebCore/Modules/fetch/FetchResponse.h:84
> +    const char* activeDOMObjectName() const override;
> +    bool canSuspendForDocumentSuspension() const override;

Should be final, not override.
Comment 19 youenn fablet 2016-03-08 00:12:42 PST
Created attachment 273279 [details]
Patch for landing
Comment 20 WebKit Commit Bot 2016-03-08 01:09:59 PST
Comment on attachment 273279 [details]
Patch for landing

Clearing flags on attachment: 273279

Committed r197744: <http://trac.webkit.org/changeset/197744>
Comment 21 WebKit Commit Bot 2016-03-08 01:10:05 PST
All reviewed patches have been landed.  Closing bug.