1330297 - Share code between Headers and setRequestHeader

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Anne (:annevk)]

Headers fails on values 0x00, 0x0A, and 0x0D. setRequestHeader() only fails on 0x00. I suggest we make the latter fail on the same as Headers at the very least.

See ongoing discussion at https://github.com/whatwg/fetch/issues/332.

Comment 1

[Anne (:annevk)]

https://github.com/w3c/web-platform-tests/pull/4525 has tests.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Those tests don't actually check 0x0A or 0x0D behavior, right?  In that the pull request there removes those tests?

Comment 3

[Anne (:annevk)]

It removes tests for \b and \x7F throwing, as those should no longer throw. It does not change \r and \n tests and adds a whole bunch of tests for both XMLHttpRequest and fetch(). Maybe you missed those? Note that https://github.com/w3c/web-platform-tests/pull/4583/files changes the added tests to perform less network fetches.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Er, yeah, I just misread the diff. :(

Comment 5

[Tom Tung [:tt, :ttung]]

I found this problem when I investigated bug 1346313, so I would like to work on this bug. :p

Comment 6

[Tom Tung [:tt, :ttung]]

Attachment: P1-wip: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: P2-wip: Remove ini file to verify we can pass the test.

Comment 8

[Tom Tung [:tt, :ttung]]

Current status: I just upload two patches to make sure they share the codes for triming HTTP whitespace. 

I'll take a look at the other part of SetRequestHeader() in xhr and Headers in fetch to see if there are any other things that can be shared.

Comment 9

[Tom Tung [:tt, :ttung]]

Hi Anne,

After reading comment[1] in github issue and spec[2], I have a question about stripping trailing/leading HTTP whitespace.
I list my question as following:

var headers = new Headers();
headers.set("foo", "b\r\nar"); // throw
headers.set("foo", "\r\n");    // ok
headers.set("foo", "\rbar\n"); // ok? but I'm not sure about it

Does it mean we can have 0x0A/0x0D in the begining or ending of header's value since we strip the trailing/leading HTTP whitespace?

[1] https://github.com/whatwg/fetch/issues/332#issuecomment-271879535
[2] https://fetch.spec.whatwg.org/#concept-header-value

Comment 10

[Anne (:annevk)]

Yes, that is what it means. Any API that deals with header values will first run https://fetch.spec.whatwg.org/#concept-header-value-normalize and only then check it meets the other constraints before throwing.

Comment 11

[Tom Tung [:tt, :ttung]]

(In reply to Anne (:annevk) from comment #10)
> Yes, that is what it means. Any API that deals with header values will first
> run https://fetch.spec.whatwg.org/#concept-header-value-normalize and only
> then check it meets the other constraints before throwing.

Got it! Thanks for answering my question!

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: P1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR.

Comment 13

[Tom Tung [:tt, :ttung]]

Attachment: P2: Remove ini files to verify we can pass the test.

Comment 14

[Tom Tung [:tt, :ttung]]

Attachment: P3: Update mochitests to aligin with spec since we normalize value before checking and throwing.

Comment 15

[Tom Tung [:tt, :ttung]]

Attachment: P3: Update mochitests to aligin with spec since we normalize value before checking and throwing.

Sorry, previous one is wrong patch...

Comment 16

[Tom Tung [:tt, :ttung]]

Comment on attachment 8857848 [details] [diff] [review]
P1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR.

Hi Andrea,

As comment 1, this bug is mainly to make fetch be able to trim leading/trailing HTTP whitespace and share the code with XHR. 

And, this patch is the implementation part.

Could you help me to review this patch? Thanks!

Comment 17

[Tom Tung [:tt, :ttung]]

Comment on attachment 8857852 [details] [diff] [review]
P2: Remove ini files to verify we can pass the test.

This patch is for removing WPT tests.

Comment 18

[Tom Tung [:tt, :ttung]]

Comment on attachment 8857854 [details] [diff] [review]
P3: Update mochitests to aligin with spec since we normalize value before checking and throwing.

This patch is for modifying mochitests since the behavior is changed.

Comment 19

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #17)
> This patch is for removing WPT tests.

s/removing/enabling/

Comment 20

[Tom Tung [:tt, :ttung]]

Thanks for the reviews!

Comment 21

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1330297 - Part 1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR. r=baku

Comment 22

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1330297 - Part 2: Remove ini files to verify we can pass the test. r=baku

Comment 23

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1330297 - Part 3: Update mochitests to aligin with spec since we normalize value before checking and throwing. r=baku

Comment 24

[Tom Tung [:tt, :ttung]]

Attachment: [Final] Bug 1330297 - Part 3: Update mochitests to aligin with spec since we normalize value before checking and throwing. r=baku

Previous updated patch is wrong...

Comment 25

[Tom Tung [:tt, :ttung]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=996827b6233c6470a9fffd4635388f7c0f9c914c

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c72f2f5feaae
Part 1: Strip leading or trailing HTTP whitespace for Header value to follow the spec and share code bewteen fetch and XHR. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e230df9fbec5
Part 2: Remove ini files to verify we can pass the test. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/51df9eb41148
Part 3: Update mochitests to align with spec since we normalize value before checking and throwing. r=baku

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c72f2f5feaae
https://hg.mozilla.org/mozilla-central/rev/e230df9fbec5
https://hg.mozilla.org/mozilla-central/rev/51df9eb41148