918752 - [XHR2] Default Accept: header more complex than */*

Status: RESOLVED FIXED

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

Description

[Hallvord R. M. Steen [:hallvors]]

If no author-supplied Accept header is given, the default should be */*
Test case:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/send-accept.htm

Comment 1

[Thomas Wisniewski]

Attachment: 918752-set-accept-request-header-properly-if-not-user-set-for-xhrs.diff

Here is a simple patch that overrides accept accordingly in Send(), if it has not been user-specified.

Comment 2

[Thomas Wisniewski]

Note that bug 433859 is a dupe of this one. It does mention the fetch API has the same requirement, but the fetch API's related test is passing already: http://w3c-test.org/fetch/api/basic/accept-header.html

It might be worth changing that test so it also verifies that Accept can be overridden, as it only checks that */* is set by default (the same change might be worth adding to this XHR test).

Also, the Fetch spec isn't as simple as just requiring */*, but requires specific UA strings for navigation requests and certain request content types (see https://fetch.spec.whatwg.org/#concept-fetch). It seems the test isn't checking those cases... perhaps it's outdated?

Comment 3

[Hallvord R. M. Steen [:hallvors]]

(That XHR test is already checking that Accept can be overridden, no? Master repo: https://github.com/w3c/web-platform-tests/blob/master/XMLHttpRequest/send-accept.htm )

Comment 4

[Thomas Wisniewski]

Ah, indeed it is. Strange that I would have missed that; guess I didn't expect it to test for more than one thing at once.

Comment 5

[Thomas Wisniewski]

Comment on attachment 8760829 [details] [diff] [review]
918752-set-accept-request-header-properly-if-not-user-set-for-xhrs.diff

Review of attachment 8760829 [details] [diff] [review]:
-----------------------------------------------------------------

Changing reviewers, just in case :jst is unavailable. Hopefully this isn't a faux pas...

Comment 6

[Thomas Wisniewski]

Here's a successful try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef87b0be88b2

The noted failures there are happening for me locally with or without this patch, so they should be unrelated.

Comment 7

[Thomas Wisniewski]

Attachment: 918752-set-accept-request-header-properly-if-not-user-set-for-xhrs.diff

Rebased.

Comment 8

[Thomas Wisniewski]

Attachment: 918752-set-accept-request-header-properly-if-not-user-set-for-xhrs.diff

Rebased.

Comment 10

[Thomas Wisniewski]

Attachment: 918752-set-accept-request-header-properly-if-not-user-set-for-xhrs.diff

Rebased the patch. Try seems fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76c1042164ad

There were no functional changes, just a minor update to a test I had missed, so I'm carrying over r+ and requesting check-in.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/00afd3c93ccf
Override Accept request header to */* in XMLHttpRequest::Send() if it is not specified by the user. r=jst

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/00afd3c93ccf

Comment 13

[Kohei Yoshino]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/accept-header-for-xhr-has-been-simplified/

Comment 14

[Jean-Yves Perrier [:teoli]]

Added mentions in the docs:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/send
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/setRequestHeader
and
https://developer.mozilla.org/en-US/Firefox/Releases/51#Others