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