128541 - XMLHttpRequest.send(null) fails

Status: VERIFIED FIXED

(Core :: XML, defect, P3)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Michael Gannon reported this in n.p.m.xml. Need a testcase, which should be easy
to using this info here. I think Michael's patch will not fix it, but I have a
hunch where the problem lies...

Michael wrote:

I am trying to use the XMLHttpRequest object to create a WebDAV client
and have come across what I believe to be a bug related to the above
bug. When the send() method called from a JS file with null as the
parameter I get the following error


[Exception... "Component returned failure code: 0x80460001
(NS_ERROR_CANNOT_CONVERT_DATA) [nsIXMLHttpRequest.send]"  nsresult:
"0x80460001 (NS_ERROR_CANNOT_CONVERT_DATA)"

The following excerpt of code is where the exception is being thrown:
    var req = new XMLHttpRequest();
    req.open("PROPFIND", href, false);
    req.setRequestHeader("Depth", "0");
    req.send(null);


I believe that this is because the method -
nsXMLHttpRequest::Send(nsIVariant *aBody) - expects a nsIVariant
parameter, but it assumes that it is either a DOM object or a string
and not of type 'NULL'.

I've created a patch below that should fix this (I think...):
*** nsXMLHttpRequest.cpp.bak    Thu Feb  7 00:19:04 2002
--- nsXMLHttpRequest.cpp        Tue Feb 26 18:56:57 2002
***************
*** 1020,1025 ****
--- 1020,1028 ----
            }
          }
        }
+       } else if (dataType == nsIDataType::VTYPE_VOID || dataType ==
nsIDataType::VTYPE_EMPTY) {
+               // do nothing...
+               // param was null...
      } else {
        // try variant string
        rv = aBody->GetAsWString(getter_Copies(serial));


Basically, this just adds a check to see if the datatype of the
nsIVariant parameter is of type null/void and does nothing if it is...
Else it assumes that it is a string.

Comment 1

[Michael Gannon]

Attachment: Synchronous OPTIONS call

This testcase, tries to send the OPTIONS command to a HTTP server and currently
fails with an exception.

Comment 2

[Michael Gannon]

I've been using 0.9.9 for a few weeks now, with the above patch applied and it 
has been working fine. 

What might be better though, is if it explicity checked for a 'string' 
argument, instead of defaulting to that. And if 'null' is passed in, it will 
implicitly ignore it.

Comment 3

[Jim Ley]

Does the patch, also fix

xmlhttp.send() - which is what MS recommend for not passing arguments?

This currently generates a 
Error: uncaught exception: [Exception... "Not enough arguments 
[nsIXMLHttpRequest.send]"  nsresult: "0x80570001 
(NS_ERROR_XPC_NOT_ENOUGH_ARGS)"  

If not can we make it so please?  It's needded to match IE's behaviour and it's 
unnatural in ECMAScript objects to be forced into passing null - passing an empty 
string is also definately wrong IMO for HEAD etc. requests, there's no information 
passed with such a request, which is conceptually distinct from an empty string.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Johnny, would it be possible to make XMLHttpRequest.send() accept 0 or 1 arguments?

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Patch as attachment, only comment changed

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

ARGH! This is a major regression. When I added support for nsIVariant I did not
realize it would mean calls from JS would ALWAYS come with aBody != 0 (i.e. of
type empty or something). So as the situation is now, all operations that pass
null that are not GET are likely to fail. I am pretty sure any serious user of
XMLHttpRequest is going to hit this, for example DIG group etc.

I also think Michael's suggestion to explicitly list string types to convert to
is better than what there is now. Will attach another patch.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Better fix

I changed the outermost if-clause to a switch statement. VOID and EMPTY will
reduce to same as no parameter (this bug originally). While testing, I realized
IE will throw errors for arrays (invalid parameter), and will happily accept
and serialize numbers. This will make us match IE more closely.

Reviews, John & Johnny?

Comment 8

[Chris Petersen]

Changing QA Contact

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 79048 [details] [diff] [review]
Better fix

sr=jst

Comment 10

[harishd]

Comment on attachment 79048 [details] [diff] [review]
Better fix

r=harishd

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in to trunk, sending for drivers & ADT approval.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 79048 [details] [diff] [review]
Better fix

a=[e-mail] for branch checkin

Comment 13

[Paul Wyskoczka]

Rakesh, can you verify the fix on the trunk and comment in the bug when done.

Comment 14

[Rakesh Mishra]

Verified on the build 2002-04-16-06-trunk on Win 2000 machine.

Comment 15

[Rakesh Mishra]

Attachment: Modified test case for Bug # 128541

Verified on the build 2002-04-16-06-trunk(Win 2000) with the modified test
case, Testcase_bug128541_new.html

Comment 16

[Paul Wyskoczka]

adt1.0.0+ on behalf of the adt.  Please check into the branch today and add
fixed1.0.0 to the keyword field.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Checked in on branch as well.

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Rakesh had verified this on trunk earlier, marking as such.

Comment 19

[Rakesh Mishra]

Verified on the branch build 2002-04-19-08-1.0.0 on Win 2000 machine.
Checked with the above attachment "Modified test case for Bug # 128541 ".
Adding "verified 1.0.0" to keywords.