Bug 168043 - [Fetch API] fetch fails when undefined is passed as headers
Summary: [Fetch API] fetch fails when undefined is passed as headers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
: 169970 (view as bug list)
Depends on:
Blocks:
 
Reported: 2017-02-09 01:34 PST by Torjus
Modified: 2017-05-08 22:08 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.76 KB, patch)
2017-02-10 08:22 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 Torjus 2017-02-09 01:34:44 PST
When explicitly passing `undefined` to the `headers` option of `fetch()`, the promise is rejected with 'TypeError: undefined is not an object'.

To reproduce, run `fetch('/my-awesome-api', { headers: undefined })`.

Both Chrome and Firefox will accept explicitly passing `{ headers: undefined }`.
As the fetch API has been available in other browsers for quite some time now, I think passing `undefined` should be supported, as existing libraries or application may break when doesn't fall back to using a polyfill.
Comment 1 youenn fablet 2017-02-10 08:01:41 PST
Hi Torjus and thanks for the feedback.
Your understanding seems indeed correct.
Comment 2 youenn fablet 2017-02-10 08:22:40 PST
Created attachment 301160 [details]
Patch
Comment 3 Geoffrey Garen 2017-02-10 08:58:34 PST
Comment on attachment 301160 [details]
Patch

r=me

What should happen if 'headers' is null? How about a string or a number? You should add tests for those cases.
Comment 4 Chris Dumez 2017-02-10 09:18:04 PST
Comment on attachment 301160 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchInternals.js:33
> +    if (headersInit === @undefined)

What about null?
Comment 5 youenn fablet 2017-02-10 10:32:22 PST
We need IDL tests for all of them I guess.
Checking with Chris, null should be fine (empty header), other values should not.
Comment 6 Chris Dumez 2017-02-10 10:36:22 PST
(In reply to comment #5)
> We need IDL tests for all of them I guess.
> Checking with Chris, null should be fine (empty header), other values should
> not.

https://heycam.github.io/webidl/#es-union (Step 3.2.)
then
https://heycam.github.io/webidl/#es-record (Step 2)

So passing null or undefined should behave the same here.
Comment 7 Chris Dumez 2017-02-10 10:37:42 PST
(In reply to comment #6)
> (In reply to comment #5)
> > We need IDL tests for all of them I guess.
> > Checking with Chris, null should be fine (empty header), other values should
> > not.
> 
> https://heycam.github.io/webidl/#es-union (Step 3.2.)
> then
> https://heycam.github.io/webidl/#es-record (Step 2)
> 
> So passing null or undefined should behave the same here.

I would expect other non-object types to throw a TypeError based on https://heycam.github.io/webidl/#es-union (Step 17)
Comment 8 youenn fablet 2017-02-10 15:43:48 PST
(In reply to comment #6)
> (In reply to comment #5)
> > We need IDL tests for all of them I guess.
> > Checking with Chris, null should be fine (empty header), other values should
> > not.
> 
> https://heycam.github.io/webidl/#es-union (Step 3.2.)
> then
> https://heycam.github.io/webidl/#es-record (Step 2)
> 
> So passing null or undefined should behave the same here.

This feels a bit odd though in that case to have the same behavior.
Somehow {headers: undefined} means that there is no headers in the dictionary and we should reuse the RequestInfo headers if any.
{headers: null} might mean to create a request without any header.

Let's get more time for fetch+WebIDL spec to stabilise on that.
Comment 9 youenn fablet 2017-02-10 15:51:34 PST
Landing this patch as is since it is a step in the right direction.
We will probably complement it with additional patches.
Comment 10 WebKit Commit Bot 2017-02-10 15:55:06 PST
Comment on attachment 301160 [details]
Patch

Clearing flags on attachment: 301160

Committed r212162: <http://trac.webkit.org/changeset/212162>
Comment 11 WebKit Commit Bot 2017-02-10 15:55:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Alexey Proskuryakov 2017-03-28 17:32:23 PDT
*** Bug 169970 has been marked as a duplicate of this bug. ***
Comment 13 youenn fablet 2017-05-08 22:08:55 PDT
(In reply to youenn fablet from comment #8)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > We need IDL tests for all of them I guess.
> > > Checking with Chris, null should be fine (empty header), other values should
> > > not.
> > 
> > https://heycam.github.io/webidl/#es-union (Step 3.2.)
> > then
> > https://heycam.github.io/webidl/#es-record (Step 2)
> > 
> > So passing null or undefined should behave the same here.

WebIDL spec has changed and we should now throw in case of null passed to a Record.