Closed Bug 1096263 Opened 5 years ago Closed 5 years ago

XMLHttpRequest.send({}) should not throw

Categories

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

36 Branch
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla36
Tracking Status
firefox33 --- unaffected
firefox34 + verified
firefox35 + verified
firefox36 + verified
firefox-esr31 34+ verified
b2g-v1.4 --- fixed
b2g-v2.0 --- verified
b2g-v2.0M --- verified
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(5 files)

[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
Depends on: 1096267
I think the safest possible option is to treat send({}) as if no data was sent.
No longer depends on: 1096267
Depends on: 1096267
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)
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.
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
Blink passes "[object Object]" as a data to the server.
> 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/
(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]
Duplicate of this bug: 1096262
> 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...
33 is unaffected but ESR31, B2G2.0 and B2G1.4 are affected? Did we uplift a fix that caused the regression?
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.
Flags: needinfo?(bugs)
Attached patch v1Splinter Review
(For some reason I can't upload to try atm.)

tests coming as a separate patch.
Attachment #8520158 - Flags: review?(bzbarsky)
Comment on attachment 8520158 [details] [diff] [review]
v1

r=me
Attachment #8520158 - Flags: review?(bzbarsky) → review+
Attached patch testsSplinter Review
Attachment #8520212 - Flags: review?(bzbarsky)
Comment on attachment 8520212 [details] [diff] [review]
tests

r=me
Attachment #8520212 - Flags: review?(bzbarsky) → review+
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 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.)
Attachment #8520158 - Flags: approval-mozilla-aurora?
Attached patch branchesSplinter Review
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
Attachment #8520619 - Flags: approval-mozilla-esr31?
Attachment #8520619 - Flags: approval-mozilla-beta?
Attachment #8520619 - Flags: approval-mozilla-b2g34?
Attachment #8520619 - Flags: approval-mozilla-b2g32?
Attachment #8520619 - Flags: approval-mozilla-b2g30?
Attachment #8520158 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8520619 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Comment on attachment 8520619 [details] [diff] [review]
branches

Olli verified the fix on the latest Nightly. (Thanks Olli!) Taking this in beta9.
Attachment #8520619 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
Status: RESOLVED → VERIFIED
Comment on attachment 8520619 [details] [diff] [review]
branches

will be merged to b2g34 from beta so clearing that.
Attachment #8520619 - Flags: approval-mozilla-b2g34?
Attachment #8520619 - Flags: approval-mozilla-b2g32?
Attachment #8520619 - Flags: approval-mozilla-b2g32+
Attachment #8520619 - Flags: approval-mozilla-b2g30?
Attachment #8520619 - Flags: approval-mozilla-b2g30+
A tweak needed for esr31 coming.
esr31 has a bit different calling convention for ConvertJSValueToString.
Attached patch esr31, 30, 32Splinter Review
(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.
(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.
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.
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
Attached video Verify_Woodduck_XML.MP4
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.