Bug 173295 – [Fetch API] TypeError when called with body === {}

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Aleksandr Motsjonov]

Created attachment 312740 [details]
Stack trace in developer tools

I am not sure what is causing my problem and how to prepare a proper code sample.

Error I have is "TypeError: Type error"
Call-stack is not helpful, but still:

[native code]
initializeFetchRequest
[native code]
fetch

I am calling fetch function with arguments: url, params
url is a string,
params is an object with:

{
  method: 'POST', //(or 'DELETE')
  headers: new Headers({
    'Authorization': 'JWT asdasdasdasdasdasadas'
  })
}

It works in Chrome.
I tried using simplest xhr implementation and it works in Safari.

[Radar WebKit Bug Importer]

<rdar://problem/32746733>

[youenn fablet]

Thanks for the bug report.
Is there a live web site where this can be tried?
Can you try with Safari Tech Preview?

[Aleksandr Motsjonov]

I tried on Safari Tech Preview. Same error.

I am not sure how to do a live demo for that, unfortunately. =\ I'll think more on that.

[Aleksandr Motsjonov]

I think I found what was the problem. We were setting body === {}.
As soon as I removed "body" property for the second argument in these cases - it started working again.

It seems like you guys are ok within requirements:

> body: Any body that you want to add to your request: this can be a Blob, BufferSource, FormData, URLSearchParams, or USVString object. Note that a request using the GET or HEAD method cannot have a body.

But error message could be more descriptive. + the fact all other browsers work with actual object didn't help =)

I think this can be closed if you decide so.

[youenn fablet]

(In reply to Aleksandr Motsjonov from comment #4)
> I think I found what was the problem. We were setting body === {}.
> As soon as I removed "body" property for the second argument in these cases
> - it started working again.
> 
> It seems like you guys are ok within requirements:
> 
> > body: Any body that you want to add to your request: this can be a Blob, BufferSource, FormData, URLSearchParams, or USVString object. Note that a request using the GET or HEAD method cannot have a body.
> 
> But error message could be more descriptive. + the fact all other browsers
> work with actual object didn't help =)
> 
> I think this can be closed if you decide so.

Since other browsers are not doing the same, there should be some more investigation on our side.
Is it possible that passing {} will convert it to a USVString?

Now that the binding generator has good support for unions, we should probably update FetchRequest implementation to use it directly.

[Chris Dumez]

(In reply to youenn fablet from comment #5)
> (In reply to Aleksandr Motsjonov from comment #4)
> > I think I found what was the problem. We were setting body === {}.
> > As soon as I removed "body" property for the second argument in these cases
> > - it started working again.
> > 
> > It seems like you guys are ok within requirements:
> > 
> > > body: Any body that you want to add to your request: this can be a Blob, BufferSource, FormData, URLSearchParams, or USVString object. Note that a request using the GET or HEAD method cannot have a body.
> > 
> > But error message could be more descriptive. + the fact all other browsers
> > work with actual object didn't help =)
> > 
> > I think this can be closed if you decide so.
> 
> Since other browsers are not doing the same, there should be some more
> investigation on our side.
> Is it possible that passing {} will convert it to a USVString?
> 
> Now that the binding generator has good support for unions, we should
> probably update FetchRequest implementation to use it directly.

I was going to help but then noticed FetchRequest is using a JSBuiltinConstructor :(

[youenn fablet]

Created attachment 313100 [details]
Patch

[youenn fablet]

> I was going to help but then noticed FetchRequest is using a
> JSBuiltinConstructor :(

Yeah, this helps handling headers iteration.

[Build Bot]

Comment on attachment 313100 [details]
Patch

Attachment 313100 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3943046

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 313107 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313100 [details]
Patch

Attachment 313100 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3943067

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 313110 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 313100 [details]
Patch

Attachment 313100 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3943035

New failing tests:
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.html
imported/w3c/web-platform-tests/fetch/api/basic/request-headers.any.worker.html

[Build Bot]

Created attachment 313113 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5

[youenn fablet]

Created attachment 313116 [details]
Patch

[Sam Weinig]

Comment on attachment 313116 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +
> +        Handling of ReadableStream bodies remains in JS builtin for Response.
> +        This allows easier handling cloning and consumption of body.
> +        Adding setBodyAsReadableStream since this is no longer handled by extractBody.

While not necessary for this change, which is great, I'd like to restart my effort to understand why the streams code has to be so different than the rest of the bindings.

> Source/WebCore/Modules/fetch/FetchBody.cpp:47
> +    if (WTF::holds_alternative<RefPtr<Blob>>(value)) {

For these kind of things, we usually use WTF::switchOn(..)

> Source/WebCore/Modules/fetch/FetchBody.h:69
> +    using DataType = Variant<RefPtr<Blob>, RefPtr<ArrayBufferView>, RefPtr<ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>, String>;

I think the names Data and DataType are too close for there not to be more information about what they are.  Can they be named more precisely?

> Source/WebCore/Modules/fetch/FetchRequest.js:52
>      else if (!@isObject(init))
>          @throwTypeError("Request init must be an object");
>  
> -    let headers = this.@initializeWith(input, init);
> +    const headers = this.@initializeWith(input, init);
>      @assert(headers instanceof @Headers);
>  
> -    let inputIsRequest = input instanceof @Request;
> +    const inputIsRequest = input instanceof @Request;
>      if ("headers" in init)
>          @fillFetchHeaders(headers, init.headers)
>      else if (inputIsRequest)
>          @fillFetchHeaders(headers, input.headers)
>  
> -    let hasInitBody = init.body !== @undefined && init.body !== null;
> -    this.@setBody(hasInitBody ? init.body : null, inputIsRequest ? input : null);
> +    const hasInitBody = init.body !== @undefined && init.body !== null;
> +    if (hasInitBody)
> +        this.@setBody(init.body)
> +    else
> +        this.@setBodyFromInputRequest(inputIsRequest ? input : null);
>  
>      return this;

Does this need to remain a builtin?

> LayoutTests/webrtc/test.html:8
> +<!doctype html>
> +<html>
> +    <head>
> +        <meta charset="utf-8">
> +        <title>Testing basic video exchange from offerer to receiver</title>
> +        <script src="../resources/testharness.js"></script>
> +        <script src="../resources/testharnessreport.js"></script>
> +    </head>

Seems unrelated.

[youenn fablet]

Thanks for the review.

> While not necessary for this change, which is great, I'd like to restart my
> effort to understand why the streams code has to be so different than the
> rest of the bindings.

We would need to add code for a ReadableStream DOMGuarded, add binding code for it.
Probably also need to be able to create/clone a ReadableStream from C++.
Given there is just one usage for ReadableStream right now and given how FetchResponse works, it is much easier to implement it through JS builtin.

We can reevaluate this when adding support for ReadableStream to Request.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:47
> > +    if (WTF::holds_alternative<RefPtr<Blob>>(value)) {
> 
> For these kind of things, we usually use WTF::switchOn(..)

Will check this.

> > Source/WebCore/Modules/fetch/FetchBody.h:69
> > +    using DataType = Variant<RefPtr<Blob>, RefPtr<ArrayBufferView>, RefPtr<ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>, String>;
> 
> I think the names Data and DataType are too close for there not to be more
> information about what they are.  Can they be named more precisely?

Ideally there should be just one really.
I'll change DataType to BindingDataType.

> > Source/WebCore/Modules/fetch/FetchRequest.js:52
> >      else if (!@isObject(init))
> >          @throwTypeError("Request init must be an object");
> >  
> > -    let headers = this.@initializeWith(input, init);
> > +    const headers = this.@initializeWith(input, init);
> >      @assert(headers instanceof @Headers);
> >  
> > -    let inputIsRequest = input instanceof @Request;
> > +    const inputIsRequest = input instanceof @Request;
> >      if ("headers" in init)
> >          @fillFetchHeaders(headers, init.headers)
> >      else if (inputIsRequest)
> >          @fillFetchHeaders(headers, input.headers)
> >  
> > -    let hasInitBody = init.body !== @undefined && init.body !== null;
> > -    this.@setBody(hasInitBody ? init.body : null, inputIsRequest ? input : null);
> > +    const hasInitBody = init.body !== @undefined && init.body !== null;
> > +    if (hasInitBody)
> > +        this.@setBody(init.body)
> > +    else
> > +        this.@setBodyFromInputRequest(inputIsRequest ? input : null);
> >  
> >      return this;
> 
> Does this need to remain a builtin?

The main purpose of this JS builtin is the initialization of the headers.
Spec changed and is now using Sequence/Record, see https://fetch.spec.whatwg.org/#concept-headers-fill.
We should update the implementation to match the spec.
We could probably remove this JS built-in when adding support for Record in the binding generator.
Note that we would have a sequence of a sequence of two items which might not be great.

> > LayoutTests/webrtc/test.html:8
> > +<!doctype html>
> > +<html>
> > +    <head>
> > +        <meta charset="utf-8">
> > +        <title>Testing basic video exchange from offerer to receiver</title>
> > +        <script src="../resources/testharness.js"></script>
> > +        <script src="../resources/testharnessreport.js"></script>
> > +    </head>
> 
> Seems unrelated.

Right.

[youenn fablet]

Created attachment 313581 [details]
Patch for landing

[youenn fablet]

> > Source/WebCore/Modules/fetch/FetchBody.cpp:47
> > +    if (WTF::holds_alternative<RefPtr<Blob>>(value)) {
> 
> For these kind of things, we usually use WTF::switchOn(..)

I tried to use WTF::switchOn with r-value references so as to avoid count churning.
I was not successful so sticked to the holds_alternative approach.

I guess we could improve switchOn to enable that case but I am not sure to have time right now for that.


> > Source/WebCore/Modules/fetch/FetchBody.h:69
> > +    using DataType = Variant<RefPtr<Blob>, RefPtr<ArrayBufferView>, RefPtr<ArrayBuffer>, RefPtr<DOMFormData>, RefPtr<URLSearchParams>, String>;
> 
> I think the names Data and DataType are too close for there not to be more
> information about what they are.  Can they be named more precisely?

Changed DataType to BindingDataType.
Would be great if we could use Ref here.

> > LayoutTests/webrtc/test.html:8
> > +<!doctype html>
> > +<html>
> > +    <head>
> > +        <meta charset="utf-8">
> > +        <title>Testing basic video exchange from offerer to receiver</title>
> > +        <script src="../resources/testharness.js"></script>
> > +        <script src="../resources/testharnessreport.js"></script>
> > +    </head>
> 
> Seems unrelated.

Removed.

[WebKit Commit Bot]

Comment on attachment 313581 [details]
Patch for landing

Clearing flags on attachment: 313581

Committed r218677: <http://trac.webkit.org/changeset/218677>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.