GetResponseXML returns NS_OK on failure (crash)

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
XML
P2
critical
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: jesup, Assigned: Heikki Toivonen (remove -bugzilla when emailing directly))

Tracking

({crash})

Trunk
mozilla1.2alpha
x86
All
crash
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

16 years ago
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.)
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?

Updated

16 years ago
QA Contact: petersen → rakeshmishra
(Reporter)

Comment 2

16 years ago
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.
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.
(Reporter)

Comment 4

16 years ago
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.
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.
(Reporter)

Comment 6

16 years ago
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.
Keywords: nsbeta1+
Target Milestone: --- → mozilla1.2beta
Target Milestone: mozilla1.2beta → mozilla1.2alpha

Comment 7

16 years ago
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.
Created attachment 95434 [details] [diff] [review]
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.
IE seems to have a non-null document always. In any case, I have not been able
to have it throw in IE.
Severity: normal → critical
Status: NEW → ASSIGNED
Keywords: nsbeta1+ → crash
Priority: -- → P2
Comment on attachment 95434 [details] [diff] [review]
fix: check document exists before using

r/sr=bzbarsky.
Attachment #95434 - Flags: review+
Comment on attachment 95434 [details] [diff] [review]
fix: check document exists before using

sr=jst
Attachment #95434 - Flags: superreview+
Checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.