1096263 - XMLHttpRequest.send({}) should not throw

Status: VERIFIED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:
Because we regressed a behavior web apparently relies on 
https://github.com/webcompat/web-bugs/issues/437

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

I think the safest possible option is to treat send({}) as if no data was sent.

Comment 2

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

Well, should any of this work?

var myObj = {foo:1,  bar:2}
myObj.prototype.toString = function(){ var str='';for(var prop in this)if(this.hasOwnProperty(prop)){ar.push(prop+'='+this[prop])}; return ar.join('&');}
xhr.send(myObj)

var myFakeJSON = {toString:function(){ return JSON.stringify(this, function(key, val){ return key ==='toString'?null:val; }) }}
xhr.send(myFakeJSON)

var myDoc = {valueOf:function(){return document}}
xhr.send(myDoc)


If not, why not?

(Note: all code above untested right now, no idea what implementations do)

Comment 3

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

And because it's not always pleasant to read code in Bugzilla, and the code I wrote may be buggy, here's the intended questions in English: 

1) send() an object that stringifies to a typical application/x-www-form-urlencoded payload - should work or not?
2) send() an object that stringifies to a JSON representation - should work or not?
3) send() an object whose valueOf returns a DOM document - should work or not?

Obviously 1 and 2 are really the same.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

It is not clear what we should do for FF34, since also FF33 throws, it just throws NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED, not TypeError

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Blink passes "[object Object]" as a data to the server.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Blink passes "[object Object]" as a data to the server.

This is what the spec requires.

I think we should just do what the spec says.  That is, if the QI to nsIXPConnectWrappedJS succeeds, get the JSObject it wraps, JS::ToString it, and send the resulting string.

> since also FF33 throws, it just throws NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED

Where does FF33 throw, exactly?  I'm not seeing it on the fiddle at http://jsfiddle.net/xc2k5v16/

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> > Blink passes "[object Object]" as a data to the server.
> 
> This is what the spec requires.
> 
> I think we should just do what the spec says.  That is, if the QI to
> nsIXPConnectWrappedJS succeeds, get the JSObject it wraps, JS::ToString it,
> and send the resulting string.
doing that manually is rather annoying (and a bit error prone too. Which compartment to enter?).





> Where does FF33 throw, exactly?  I'm not seeing it on the fiddle at
> http://jsfiddle.net/xc2k5v16/

try { var xhr = new XMLHttpRequest(); xhr.open("POST", this.location + "/test", true); xhr.onload = function() { alert(xhr.responseText); };  xhr.send({}); } catch(ex) {alert(ex);} alerts

[Exception... "JavaScript component does not have a method named: "available"'JavaScript component does not have a method named: "available"' when calling method: [nsIInputStream::available]"  nsresult: "0x80570030 (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Comment 9

[Boris Zbarsky [:bzbarsky]]

> doing that manually is rather annoying (and a bit error prone too. Which compartment to
> enter?).

The same one the bindings would have, no?  That is, the compartment of the object involved.

Or we could just mark the method as wanting a JSContext and then we'll have one already in the right compartment.

> try { var xhr = new XMLHttpRequest(); xhr.open("POST",

Ah, so the difference is POST vs GET.

travis-ci (the page this is reported against) is doing a GET, not a POST.  So the data argument it passes is actually completely ignored once it gets past the API entry point...

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

33 is unaffected but ESR31, B2G2.0 and B2G1.4 are affected? Did we uplift a fix that caused the regression?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Yes, it was uplifted. 
We used to throw an exception only when passing {} to send() method of XHR which was opened with
"POST" as param, and now we throw also when "GET" is passed.

Patch is coming. I'm just writing test for it.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v1

(For some reason I can't upload to try atm.)

tests coming as a separate patch.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8520158 [details] [diff] [review]
v1

r=me

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: tests

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8520212 [details] [diff] [review]
tests

r=me

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Finally a try push  https://tbpl.mozilla.org/?tree=Try&rev=396a2b7c6e47

Comment 17

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

new w-p-t tests coming here:
https://github.com/w3c/web-platform-tests/pull/1379
I forgot to test for null, undefined and toString() throwing so I stole those ideas from Olli's test here ;)

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac5182ee410a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac0d616b8870

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8520158 [details] [diff] [review]
v1

Approval Request Comment
[Feature/regressing bug #]: 1087633
[User impact if declined]: Travis not working
[Describe test coverage new/current, TBPL]: landed to m-i with tests
[Risks and why]: In theory someone might expect exception, but this patch makes as follow the spec and other browsers do the same
[String/UUID change made/needed]: NA


(Because of bug 1047483 we need a tiny bit different patch for other branches.)

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2dd320673c0

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Attachment: branches

Approval Request Comment
[Feature/regressing bug #]: Bug 1087633
[User impact if declined]: Travis CI doesn't work
[Describe test coverage new/current, TBPL]: landed to m-i with tests
[Risks and why]: Shouldn't be too risky. Makes us work like other browsers.
[String/UUID change made/needed]: NA

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ac5182ee410a
https://hg.mozilla.org/mozilla-central/rev/ac0d616b8870
https://hg.mozilla.org/mozilla-central/rev/f2dd320673c0

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8520619 [details] [diff] [review]
branches

Olli verified the fix on the latest Nightly. (Thanks Olli!) Taking this in beta9.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I can also verify this is fixed in Mozilla/5.0 (X11; Linux x86_64; rv:36.0) Gecko/20100101 Firefox/36.0 ID:20141112030202 CSet: 688f821edcd4

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 8520619 [details] [diff] [review]
branches

will be merged to b2g34 from beta so clearing that.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d2c4f2ef5e9f
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0d37eab5462
https://hg.mozilla.org/releases/mozilla-aurora/rev/75790a27bffb

https://hg.mozilla.org/releases/mozilla-beta/rev/9e57cec588a9
https://hg.mozilla.org/releases/mozilla-beta/rev/0197e9eb324f

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/5852ae12e124
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/01cf8e63c8b6

https://hg.mozilla.org/releases/mozilla-esr31/rev/8aa9e2243cbe
https://hg.mozilla.org/releases/mozilla-esr31/rev/1d354a658206

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/7c9a709e2a8e
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/99d29d28f5bb

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

A tweak needed for esr31 coming.
esr31 has a bit different calling convention for ConvertJSValueToString.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: esr31, 30, 32

Comment 29

[Botond Ballo [:botond]]

(In reply to Olli Pettay [:smaug] from comment #28)
> Created attachment 8521814 [details] [diff] [review]
> esr31, 30, 32

The b2g 1.4 branch also needs this change. It's currently unbuildable.

Comment 30

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #29)
> (In reply to Olli Pettay [:smaug] from comment #28)
> > Created attachment 8521814 [details] [diff] [review]
> > esr31, 30, 32
> 
> The b2g 1.4 branch also needs this change. It's currently unbuildable.

Also this patch doesn't apply cleanly to the 1.4 branch.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Hence the "30" in that attachment name ;)

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e21bf45e6c44
https://hg.mozilla.org/releases/mozilla-esr31/rev/b3a3336f4e01
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/4342543c72ba

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/9e57cec588a9
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/0197e9eb324f

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/e21bf45e6c44
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/01cf8e63c8b6

Comment 33

[Botond Ballo [:botond]]

Thanks :)

(In reply to Botond Ballo [:botond] from comment #30)
> Also this patch doesn't apply cleanly to the 1.4 branch.

This was my mistake, I thought the patch was a follow-up to the other one.

Comment 34

[Coler]

Verify passed, this issue can't be repro on Woodduck 2.0;Flame2.0&2.1&2.2
Attached: Verify_Woodduck_XML.mp4
Reproducing rate: 0/5
Note:We vierify it with content of  https://github.com/webcompat/web-bugs/issues/437.
Woodduck build:
Gaia-Rev        3a98f1287fa7b604891220ba5d86982ae8f9971e
Gecko-Rev       03d3ab62d5b07b915434f2d1d68495ad5915ecd2
Build-ID        20141120103003
Version         32.0

Flame2.0 build:
Gaia-Rev        1ede2666f1e6c1b3fd3b282011caf0cbc59544b0
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/faa64077b0c2
Build-ID        20141119000207
Version         32.0

Flame 2.1 build:
Gaia-Rev        1b231b87aad384842dfc79614b2a9ca68a4b4ff3
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/95fbd7635152
Build-ID        20141119001205
Version         34.0

FLame2.2 build:
Gaia-Rev        e64428c5b2dce5db90b75a5055077a04f4bd4819
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/aa72ddfe9f93
Build-ID        20141119160202
Version         36.0a1

Comment 35

[Coler]

Attachment: Verify_Woodduck_XML.MP4

Comment 36

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed fixed in Fx31.3.0esr, Fx34 and Fx35 also.