Explicitly set contentType in Firefox's Request

[00dani]

The default value is "application/x-www-form-urlencoded", but we usually want JSON. In some versions of Firefox, passing {headers: {"Content-Type": "application/json"}} to the Request constructor works. However in other versions, such as the latest Developer Edition 51.0a2, the actual header sent erroneously becomes Content-Type: application/x-www-form-urlencoded, application/json. This breaks XCloud and might break other services as well.

This patch simply ensures that if we ask for application/json, we get application/json.

[nightpool]

Sorry, but, just so I'm clear, the actually issue here is that setting the content type through the headers of Firefox XHR requests breaks in FF 51? Isn't that a regression that should be reported to Mozilla then? Or am I misunderstanding?

[00dani]

It's not XHR that's changed, it's the internal request API for addons, which XKit uses because it's not subject to origin restrictions. A Request has a contentType property separate from its headers, and I believe it is intended that the contentType itself must be set to produce the correct request header.

It might in fact be a regression however, since I can't see in Bugzilla or the commit history why the change was made.

[nightpool]

We should probably confirm that this is at least a intended change before
adding this workaround.

On Fri, Sep 30, 2016 at 3:11 AM Danielle McLean notifications@github.com
wrote:

It's not XHR that's changed, it's the internal request API for addons
https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/request,
which XKit uses because it's not subject to origin restrictions. A Request
has a contentType property separate from its headers, and I believe it is
intended that the contentType itself must be set to produce the correct
request header.

It might in fact be a regression however, since I can't see in Bugzilla or the
commit history
https://hg.mozilla.org/mozilla-central/log?rev=content-type why the
change was made.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#1165 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAORV63xm8oemGk01SvyXOFWUq208AtDks5qvLYEgaJpZM4KKvT2
.

[nightpool]

especially since creating a new extension version and getting everybody
upgraded to it is non-trivial. (although it should work better now, because
we have auto-updating set up, hopefully)

On Fri, Sep 30, 2016 at 3:11 AM Danielle McLean notifications@github.com
wrote:

It's not XHR that's changed, it's the internal request API for addons
https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/request,
which XKit uses because it's not subject to origin restrictions. A Request
has a contentType property separate from its headers, and I believe it is
intended that the contentType itself must be set to produce the correct
request header.

It might in fact be a regression however, since I can't see in Bugzilla or the
commit history
https://hg.mozilla.org/mozilla-central/log?rev=content-type why the
change was made.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#1165 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAORV63xm8oemGk01SvyXOFWUq208AtDks5qvLYEgaJpZM4KKvT2
.

[hobinjk]

This behavior conforms with the XMLHttpRequest standard's specification of setRequestHeader, specifically the duplicate usage case. Based on how request.js functions this is also the correct fix to perform.

[nightpool]

@hobinjk Not sure I follow. Here's the relevant language:

In addition there are certain other headers the user agent will take control of if they are not set by the author as indicated at the end of the send() method section

and

If Content-Type is non-null and author request headers contains no header named Content-Type, append Content-Type/Content-Type to author request headers.

emphasis mine. where does this say they should prepend a default content type if we set one explicitly in our headers?

(there's another paragraph here that looks relevant, but a close reading indicates that its actually about charsets and encodings, which we're not really messing with)

[hobinjk]

@nightpool The problem is that the Request library does setRequestHeader(request.contentType) regardless of whether Content-Type is specified in headers. It has done this since at least 2015 although the fact that Content-Type must be specified using contentType is not documented anywhere.

[nightpool]

@hobinjk I'm down to approve this change for now, but i'm still confused about how this is not a regression, from a user-facing point of view

[hobinjk]

@nightpool Yeah, it's definitely unfortunate that this is necessary. It might be worthwhile to file a bug on bugzilla to try to point out the unexpected behavior,.

@00dani Thanks for the PR! I'll try to get an extension update going tomorrow

@homu r+

[homu]

📌 Commit 0056dcd has been approved by hobinjk

[homu]

⌛ Testing commit 0056dcd with merge 71a19e5...

[homu]

☀️ Test successful - status