Fetch API: Fix behavior when Request constructor is passed an undefined referrer
When the Request constructor was called with referrer set to undefined, the
resulting referrer used "undefined" as a string. After this code change,
an undefined referrer will be considered as not present and the default referrer
"about:client" will instead be used.
BUG=624278
Review-Url: https://codereview.chromium.org/2772013002
Cr-Commit-Position: refs/heads/master@{#460049}
Committed: https://chromium.googlesource.com/chromium/src/+/617eed180347e987f47df9bf74ae4eb20ac04cb1
Description was changed from ========== fix tests fix undefined value BUG= ========== to ========== [Service ...
3 years, 9 months ago
(2017-03-24 05:44:28 UTC)
#1
Description was changed from
==========
fix tests
fix undefined value
BUG=
==========
to
==========
[Service Worker] Fix fetch behavior when referrer is set to undefined
When fetch is called with referrer set to undefined, fetch used to consider
"undefined" as a string and fetch for this referrer. After this code change,
undefined will be considered as null and fetch uses the default referrer
"about:client" instead.
BUG=624278
==========
yiyix
Description was changed from ========== [Service Worker] Fix fetch behavior when referrer is set to ...
3 years, 9 months ago
(2017-03-24 05:59:17 UTC)
#2
Description was changed from
==========
[Service Worker] Fix fetch behavior when referrer is set to undefined
When fetch is called with referrer set to undefined, fetch used to consider
"undefined" as a string and fetch for this referrer. After this code change,
undefined will be considered as null and fetch uses the default referrer
"about:client" instead.
BUG=624278
==========
to
==========
[Service Worker] Fix fetch behavior when referrer is set to undefined
When fetch is called with referrer set to undefined, fetch request used
"undefined" as a string and fetch for this referrer. After this code change,
undefined will be considered as null and fetch uses the default referrer
"about:client" instead.
BUG=624278
==========
yiyix
Description was changed from ========== [Service Worker] Fix fetch behavior when referrer is set to ...
3 years, 9 months ago
(2017-03-24 07:09:12 UTC)
#3
Description was changed from
==========
[Service Worker] Fix fetch behavior when referrer is set to undefined
When fetch is called with referrer set to undefined, fetch request used
"undefined" as a string and fetch for this referrer. After this code change,
undefined will be considered as null and fetch uses the default referrer
"about:client" instead.
BUG=624278
==========
to
==========
[Service Worker] Fix fetch behavior when referrer is set to undefined
When fetch is called with referrer set to undefined, fetch request used
"undefined" as a string and fetch for this referrer. After this code change,
undefined will be considered as null and fetch uses the default referrer
"about:client" instead to create the request.
BUG=624278
==========
This treats {referrer: undefined} the same as {referrer: null} which I don't think is right. ...
3 years, 9 months ago
(2017-03-27 01:41:59 UTC)
#8
This treats {referrer: undefined} the same as {referrer: null} which I don't
think is right.
We could work around this by manually doing what getWithUndefinedOrNullCheck()
does, except only for Undefined. It seems strange though if the Request
constructor is the only feature doing this. Even stranger,
getWithUndefinedOrNullCheck() is not used by any existing code.
bashi@ or domenic@ do you have any insight why Request() is the only API that's
running into this issue. The background is we are failing a web platform test
that essentially tests that the following are equivalent:
1) new Referrer('', {referrer: undefined})
2) new Referrer('', {})
This is because in spec parlance, a dictionary value is "not present" if it's
undefined. Does Chrome take a different view?
falken
On 2017/03/27 01:41:59, falken wrote: > This treats {referrer: undefined} the same as {referrer: null} ...
3 years, 9 months ago
(2017-03-27 01:53:43 UTC)
#9
On 2017/03/27 01:41:59, falken wrote:
> This treats {referrer: undefined} the same as {referrer: null} which I don't
> think is right.
>
> We could work around this by manually doing what getWithUndefinedOrNullCheck()
> does, except only for Undefined. It seems strange though if the Request
> constructor is the only feature doing this. Even stranger,
> getWithUndefinedOrNullCheck() is not used by any existing code.
>
> bashi@ or domenic@ do you have any insight why Request() is the only API
that's
> running into this issue. The background is we are failing a web platform test
> that essentially tests that the following are equivalent:
>
> 1) new Referrer('', {referrer: undefined})
> 2) new Referrer('', {})
>
> This is because in spec parlance, a dictionary value is "not present" if it's
> undefined. Does Chrome take a different view?
OK I guess this whole matter is caused by RequestInit having to be a dictionary
and not an IDL due to a limitation with Blink IDL.
So the workaround of using getWithUndefinedOrNullCheck() is probably OK as it
should generally not be needed by most features.
But I think we should change getWithUndefinedOrNullCheck() to
getWithUndefinedCheck() if there are indeed no other callsites of it.
yiyix@ can you also add tests in
LayoutTests/http/tests/fetch/script-tests/referrer.js for {referrer:undefined}
and {referrer:null}?
yiyix
On 2017/03/27 01:53:43, falken wrote: > On 2017/03/27 01:41:59, falken wrote: > > This treats ...
3 years, 9 months ago
(2017-03-27 06:55:32 UTC)
#10
On 2017/03/27 01:53:43, falken wrote:
> On 2017/03/27 01:41:59, falken wrote:
> > This treats {referrer: undefined} the same as {referrer: null} which I don't
> > think is right.
> >
> > We could work around this by manually doing what
getWithUndefinedOrNullCheck()
> > does, except only for Undefined. It seems strange though if the Request
> > constructor is the only feature doing this. Even stranger,
> > getWithUndefinedOrNullCheck() is not used by any existing code.
> >
> > bashi@ or domenic@ do you have any insight why Request() is the only API
> that's
> > running into this issue. The background is we are failing a web platform
test
> > that essentially tests that the following are equivalent:
> >
> > 1) new Referrer('', {referrer: undefined})
> > 2) new Referrer('', {})
> >
> > This is because in spec parlance, a dictionary value is "not present" if
it's
> > undefined. Does Chrome take a different view?
>
> OK I guess this whole matter is caused by RequestInit having to be a
dictionary
> and not an IDL due to a limitation with Blink IDL.
>
> So the workaround of using getWithUndefinedOrNullCheck() is probably OK as it
> should generally not be needed by most features.
>
> But I think we should change getWithUndefinedOrNullCheck() to
> getWithUndefinedCheck() if there are indeed no other callsites of it.
>
> yiyix@ can you also add tests in
> LayoutTests/http/tests/fetch/script-tests/referrer.js for {referrer:undefined}
> and {referrer:null}?
I agree that we should add test cases to avoid regression. As we have clarified
offline, I have added test cases in
LayoutTests/http/tests/fetch/script-tests/request.js for both referrer=undefined
and referrer=null
I am really sorry that i for the behavior for null and undefined are different.
I modified getWithUndefinedCheck as you have suggested.
Thank you very much.
yiyix
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-27 06:57:31 UTC)
#11
On 2017/03/27 at 06:55:32, yiyix wrote: > I agree that we should add test cases ...
3 years, 9 months ago
(2017-03-27 07:16:53 UTC)
#15
On 2017/03/27 at 06:55:32, yiyix wrote:
> I agree that we should add test cases to avoid regression. As we have
clarified offline, I have added test cases in
LayoutTests/http/tests/fetch/script-tests/request.js for both referrer=undefined
and referrer=null
Should the tests be in external/wpt so they can get automatically upstreamed?
falken
On 2017/03/27 07:16:53, domenic wrote: > On 2017/03/27 at 06:55:32, yiyix wrote: > > > ...
3 years, 9 months ago
(2017-03-27 07:22:27 UTC)
#16
On 2017/03/27 07:16:53, domenic wrote:
> On 2017/03/27 at 06:55:32, yiyix wrote:
>
> > I agree that we should add test cases to avoid regression. As we have
> clarified offline, I have added test cases in
> LayoutTests/http/tests/fetch/script-tests/request.js for both
referrer=undefined
> and referrer=null
>
> Should the tests be in external/wpt so they can get automatically upstreamed?
This is a great question. The WPT Fetch API tests are not yet imported into
external/wpt. I think we'll keep committing to http/tests/fetch until they are
imported.
yiyix
Patchset #2 (id:20001) has been deleted
3 years, 9 months ago
(2017-03-27 07:33:45 UTC)
#17
Patchset #2 (id:20001) has been deleted
yiyix
Patchset #2 (id:40001) has been deleted
3 years, 9 months ago
(2017-03-27 08:30:42 UTC)
#18
Patchset #2 (id:40001) has been deleted
yiyix
Patchset #2 (id:60001) has been deleted
3 years, 9 months ago
(2017-03-27 08:30:48 UTC)
#19
Patchset #2 (id:60001) has been deleted
yiyix
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-27 08:30:54 UTC)
#20
On 2017/03/27 07:22:27, falken wrote: > On 2017/03/27 07:16:53, domenic wrote: > > On 2017/03/27 ...
3 years, 9 months ago
(2017-03-27 09:01:28 UTC)
#22
On 2017/03/27 07:22:27, falken wrote:
> On 2017/03/27 07:16:53, domenic wrote:
> > On 2017/03/27 at 06:55:32, yiyix wrote:
> >
> > > I agree that we should add test cases to avoid regression. As we have
> > clarified offline, I have added test cases in
> > LayoutTests/http/tests/fetch/script-tests/request.js for both
> referrer=undefined
> > and referrer=null
> >
> > Should the tests be in external/wpt so they can get automatically
upstreamed?
>
> This is a great question. The WPT Fetch API tests are not yet imported into
> external/wpt. I think we'll keep committing to http/tests/fetch until they are
> imported.
@falken, could you please review this change? Thank you so much
falken
lgtm nits for the CL description: this change is about the Request constructor, not "fetch ...
3 years, 9 months ago
(2017-03-27 09:09:19 UTC)
#23
lgtm
nits for the CL description: this change is about the Request constructor, not
"fetch behavior" which would usually mean the behavior of the fetch() function.
Also, it's more about the Fetch API than the Service Worker API. So I would
revise to talk about the Request constructor and RequestInit options, something
like:
Fetch API: Fix behavior when Request constructor is passed an undefined referrer
When the Request constructor was called with referrer set to undefined, the
resulting referrer used "undefined" as a string. After this code change,
an undefined referrer will be considered as not present and the default referrer
"about:client" will instead be used.
https://codereview.chromium.org/2772013002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js
(right):
https://codereview.chromium.org/2772013002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:413:
"/null";
nit: use single-quotes for the string literals (because that is what the rest of
the file does)
Adding OWNERs too: +bashi for bindings +yhirano for fetch
3 years, 9 months ago
(2017-03-27 09:11:02 UTC)
#25
Adding OWNERs too:
+bashi for bindings
+yhirano for fetch
yiyix
Description was changed from ========== [Service Worker] Fix fetch behavior when referrer is set to ...
3 years, 9 months ago
(2017-03-27 09:16:50 UTC)
#26
Description was changed from
==========
[Service Worker] Fix fetch behavior when referrer is set to undefined
When fetch is called with referrer set to undefined, fetch request used
"undefined" as a string and fetch for this referrer. After this code change,
undefined will be considered as null and fetch uses the default referrer
"about:client" instead to create the request.
BUG=624278
==========
to
==========
Fetch API: Fix behavior when Request constructor is passed an undefined referrer
When the Request constructor was called with referrer set to undefined, the
resulting referrer used "undefined" as a string. After this code change,
an undefined referrer will be considered as not present and the default referrer
"about:client" will instead be used.
BUG=624278
==========
yiyix
https://codereview.chromium.org/2772013002/diff/80001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js File third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js (right): https://codereview.chromium.org/2772013002/diff/80001/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js#newcode413 third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js:413: "/null"; On 2017/03/27 09:09:19, falken wrote: > nit: use ...
3 years, 9 months ago
(2017-03-27 09:19:48 UTC)
#27
3 years, 9 months ago
(2017-03-27 09:23:26 UTC)
#28
LGTM
yhirano
lgtm https://codereview.chromium.org/2772013002/diff/100001/third_party/WebKit/Source/modules/fetch/RequestInit.cpp File third_party/WebKit/Source/modules/fetch/RequestInit.cpp (right): https://codereview.chromium.org/2772013002/diff/100001/third_party/WebKit/Source/modules/fetch/RequestInit.cpp#newcode31 third_party/WebKit/Source/modules/fetch/RequestInit.cpp:31: : areAnyMembersSet(false) { Can you put a TODO ...
3 years, 9 months ago
(2017-03-27 10:38:52 UTC)
#29
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/417722)
3 years, 9 months ago
(2017-03-28 04:03:17 UTC)
#36
On 2017/03/28 04:03:17, commit-bot: I haz the power wrote: > Try jobs failed on following ...
3 years, 9 months ago
(2017-03-28 05:56:01 UTC)
#37
On 2017/03/28 04:03:17, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
> linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
You should be able to fix these errors by running run-webkit-tests with
--reset-results. This will add the new lines for the new test cases to all the
-expected.txt files. Then reupload the patch with those changed files.
However we shouldn't need these -expected.txt files, since all the test cases
are passing. They are only needed because of the error about "duplicate test
names" at the top of the -expected.txt files. For this patch, I would use
--reset-results. A follow-up patch could fix this error and remove all the
-expected.txt files.
yiyix
On 2017/03/28 05:56:01, falken wrote: > On 2017/03/28 04:03:17, commit-bot: I haz the power wrote: ...
3 years, 9 months ago
(2017-03-28 06:37:32 UTC)
#38
On 2017/03/28 05:56:01, falken wrote:
> On 2017/03/28 04:03:17, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> > linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
>
> You should be able to fix these errors by running run-webkit-tests with
> --reset-results. This will add the new lines for the new test cases to all the
> -expected.txt files. Then reupload the patch with those changed files.
>
> However we shouldn't need these -expected.txt files, since all the test cases
> are passing. They are only needed because of the error about "duplicate test
> names" at the top of the -expected.txt files. For this patch, I would use
> --reset-results. A follow-up patch could fix this error and remove all the
> -expected.txt files.
I did some similar investigation. I believe all the failed tests are due to the
addition of new tests.
I will address the issue of duplicate test names in a new cl as you suggested.
Thank you so much for helping.
yiyix
The CQ bit was checked by yiyix@chromium.org to run a CQ dry run
3 years, 9 months ago
(2017-03-28 06:37:44 UTC)
#39
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490691592816950, "parent_rev": "98c614330c5ea34ff43692394e63dcb2168fe54b", "commit_rev": "617eed180347e987f47df9bf74ae4eb20ac04cb1"}
3 years, 9 months ago
(2017-03-28 09:24:17 UTC)
#46
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490691592816950,
"parent_rev": "98c614330c5ea34ff43692394e63dcb2168fe54b", "commit_rev":
"617eed180347e987f47df9bf74ae4eb20ac04cb1"}
commit-bot: I haz the power
Description was changed from ========== Fetch API: Fix behavior when Request constructor is passed an ...
3 years, 9 months ago
(2017-03-28 09:25:05 UTC)
#47
Message was sent while issue was closed.
Description was changed from
==========
Fetch API: Fix behavior when Request constructor is passed an undefined referrer
When the Request constructor was called with referrer set to undefined, the
resulting referrer used "undefined" as a string. After this code change,
an undefined referrer will be considered as not present and the default referrer
"about:client" will instead be used.
BUG=624278
==========
to
==========
Fetch API: Fix behavior when Request constructor is passed an undefined referrer
When the Request constructor was called with referrer set to undefined, the
resulting referrer used "undefined" as a string. After this code change,
an undefined referrer will be considered as not present and the default referrer
"about:client" will instead be used.
BUG=624278
Review-Url: https://codereview.chromium.org/2772013002
Cr-Commit-Position: refs/heads/master@{#460049}
Committed:
https://chromium.googlesource.com/chromium/src/+/617eed180347e987f47df9bf74ae...
==========
commit-bot: I haz the power
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/617eed180347e987f47df9bf74ae4eb20ac04cb1
3 years, 9 months ago
(2017-03-28 09:25:06 UTC)
#48
Issue 2772013002: Fetch API: Fix behavior when Request constructor is passed an undefined referrer
(Closed)
Created 3 years, 9 months ago by yiyix
Modified 3 years, 9 months ago
Reviewers: falken, bashi, domenic, yhirano
Base URL:
Comments: 4