158977 - GetResponseXML returns NS_OK on failure (crash)

Status: RESOLVED FIXED

(Core :: XML, defect, P2)

Description

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

XMLHttpRequest::GetResponseXML() returns NS_OK in all cases (even if there's no
document).  None of the callers assume that it will return NS_OK with a null
document.  It should return a failure if there's no document, or all callers
should be made to check rv AND pDocument.  (My opinion: it should return failure.)

Comment 1

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

We should do what IE does, since we are simulating them. Also, in my opinion we
should return an error (effectively, throw an exception) only in really
exceptional situations, like out of memory etc.

What are all the callers that you are referring to?

Comment 2

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

TestXMLExtras.cpp, nsSchemaLoader.cpp, nsWDSLLoader.cpp do not check and will
crash if no document is returned.

nsSOAPTransport.cpp does check for a document.

Does IE return NS_OK with a null document?  If so, the callers are wrong and
should be fixed.

Comment 3

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

All of the mentioned cases are in code that is either not built by default or is
test code, so it is not that urgent. But we need to check what IE does and then
solve this accordingly. I still think NS_OK is good even without document.

Comment 4

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

NS_OK with no document _could_ be ok - but if so it really should be documented,
and we need a good reason.

It also would mean there's no way to tell if an error really occurred, or if the
document was empty.  That could be a problem, since the application/etc might
want to handle the two cases differently.  And, as we can see, it leads to
people coding in land mines without realizing it.

Comment 5

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

The caller needs to check several things: response headers, responseText, and
responseXML. For example, if responseXML was empty, it is likely that the mime
type reported was not XML so we didn't even parse it. If responseText was empty,
then responseXML will obviously also be empty.

Comment 6

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

So far as I can tell, none of the callers (including the SOAP transport) are
doing those things you list.  I didn't notice any documentation indicating that
was needed (though perhaps it's there; I didn't write the code that uses this, I
just modified it to be synchronous).

Where is the documentation of these requirements?  The only docs I could find
(and it took using lxt to search for "responseXML", since IDL adds the 'Get')
was this:

/**
 95    * The response to the request is parsed as if it were a
 96    * text/xml stream. This attributes represents the response as 
 97    * a DOM Document object. NULL if the request is unsuccessful or
 98    * has not yet been sent.
 99    */

I hadn't realized this was an IDL (readonly) attribute, or I might have guessed
that rv might not be used.

Comment 7

[Ray Whitmer]

I have thought for a while that SOAP needs even better checking.  A browser user
who is debugging a SOAP call needs to be able to see what came back even if it
was not parsable XML or a valid SOAP call.  On the other hand, the script should
perhaps have less access to responses which are not SOAP or the SOAP service
could be used to ping non-SOAP service4s behind a firewall, in a limited
fashion, and see what came back.

It is clear that SOAP meswsages may also be one-way, so that must be balanced as
well, send SOAP receive something else, and vice versa.  This really gets into
security.

For the moment, SOAP is even ignoring the return content type header, because it
was causing a freeze due to bugs in other code.

Comment 8

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

Attachment: fix: check document exists before using

TextXMLExtras does not crash because it checks if the document exists before
using it. I added checks for document in schema & wsdl.

Comment 9

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

IE seems to have a non-null document always. In any case, I have not been able
to have it throw in IE.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 95434 [details] [diff] [review]
fix: check document exists before using

r/sr=bzbarsky.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 95434 [details] [diff] [review]
fix: check document exists before using

sr=jst

Comment 12

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

Checked in.