When we have a well-formedness error in an XML doc loaded as data, should we
really generate a <parseerror> node? Or should we just clear out the DOM and
return? It seems like the latter would be a better idea; the question is
whether people out in the wild seriously depend on the former behavior...
Thoughts? I think this should be pretty easy to fix if we decide to do it (esp.
once bug 251354 is fixed).
I do actually think people depend on the current behaviour. But I also think
it'd be ok to break them as long as we provided some other mechanism for
detecting the parse error. For synconous loads like xmlhttprequest (and doc.load
once we do that) we should IMHO throw an exception.
Sync document.load() returns a boolean to indicate success or failure; it can
return false (as it does now) on parse error.
For xmlhttprequest, don't we already have a way to report errors? We should use
it if we do.
I agree that we should have a way to indicate whether the document was parsed
correctly, though. Maybe even add a boolean property on some document interface?
I think that if all the methods of loading indicate failure somehow, then it's
enough to just empty the document on failure since a successful load can never
result in an empty document. Though of course an additional flag wouldn't hurt,
but it might get tricky to figure out when to clear that flag...
OK, makes sense. Will look into doing this, then.
Oh, and of course, we should check how IE deals with errors.
I don't think XMLHttpRequest has any method other than the <parseerror> element
to report the actual error. If you are going to do something else, why not
standardize on what MS does:
> I don't think XMLHttpRequest has any method other than the <parseerror> element
> to report the actual error.
It has an onerror handler, looks like. Not completely trivial to use from C++,
but really easy from JS.
> why not standardize on what MS does
It's a thought. Unfortunately, it's not very clearly documented. For example,
the errorCode property is undocumented. Do we need compat in errorCode values
if we implement this? The filepos property doesn't say what it means by
"absolute file position" (is that bytes? Or what?).
(In reply to comment #7)
> It has an onerror handler, looks like. Not completely trivial to use from C++,
> but really easy from JS.
True, but what error information do you get?
> It's a thought. Unfortunately, it's not very clearly documented. For example,
> the errorCode property is undocumented. Do we need compat in errorCode values
> if we implement this?
I wouldn't think so, but you can start with
1) is deprecated and 2) is supposedly CE .Net 4.2, but they share values and I
expect are standardized across MSXML implementations.
I would think that we would not have to be identical to their errorCode, nor in
their reason so long as we agreed that non-zero errorCode was an error.
The filepos property doesn't say what it means by
> "absolute file position" (is that bytes? Or what?).
If you are really interested, I can experiment and try to find out.
> True, but what error information do you get?
Not much, really. Just that there was an error. Just like the return value
> If you are really interested, I can experiment and try to find out.
If people don't rely on it, I'm not that interested... if they do, I sort of am.
I think we should make this change, and make it as early as we can.
So the things we currently load as data are: XMLHttpRequest, DOMParser, anything loaded by the syncload service, XMLDocument.load, and some xforms and xslt stuff.
I think we should treat these as following:
XMLHttpRequest: Null responseXML (already happens if the response is not XML)
and dispatch an error event. For sync, throw.
Async XMLDocument.load: error event
syncload service: throw
We seem to already have some code in our tree that assumes it should look for a parsererror tag. We'll have to fix that, of course.
I do think we should do this asap so sites that depend on this have time to get fixed.
This is really annoying, there's no reason we should keep doing this for yet another release.
Let's see if I manage to do this for M9. I should.
Safari has also some kind of error document, which for example DOMParser
returns. Opera throws LSException "PARSE_ERR".
Neither of those, nor IE, seem to use onerror of XHR to indicate error in parsing.
So I'm not so sure how important this is or what should be done in each case.
With XHR you could just set the document to null, I guess.
I'll have to make changes to https://www.squarefree.com/userscripts/valid-xhtml.user.js (assuming this fixes bug 45566), but I'm fine with that.
Bug 399502 should be fixed first to get events from xmldoc.load()
As far as I see, XSLT generates the error page only if the page is loaded
to be visible (not as data). Document loading should be cleaned up a bit, but
for 1.9 XSLT behaves well enough, IMO.
XMLHttpRequest: Null responseXML
DOMParser: Throw LSException like Opera
Async XMLDocument.load: error event
syncload service: throw
XForms: Handles internally, report error to console as done already now.
XSLT: when used in scripts, do whatever documented in nsIXSLTProcessor.idl
DOMParser could also just return null document?
Created attachment 285230 [details] [diff] [review]
Peter, any comments?
Please include information about the XML parsing error in the exception: line number, column number, reason. https://www.squarefree.com/userscripts/valid-xhtml.user.js needs that information.
Ah, true, that would be useful.
Though, I don't know how to do it, or can't find any reasonable way to do it.
We've shipped this forever, right? Why should this block beta?
Yeah, this should not block beta. I'd even say it shouldn't even block the release at this point, but since there is a patch we might as well drive it in.
I'm minusing. That doesn't mean it won't go in, but we need to prioritize accordingly. Anyone who objects can renom.
I thought the idea was that this is a pretty major change in behavior so we should get it shipped in beta so that web sites can adjust to it as needed. That's if we're doing it for 1.9, of course.
Put another way, I would argue that if this doesn't make beta we shouldn't take it for 1.9.
I don't think this is a big enough change that if it doesn't make first beta we can't include it later. Most sites probably has no error handling at all so they assume that the XML is wellformed, and likely only ever gets that.
(In reply to comment #21)
> Please include information about the XML parsing error in the exception: line
> number, column number, reason.
The exception should contain also the JS line number etc where
for example parseFromString is called. Should the stack (nsIException.location)
contain the information about xml parsing error or perhaps inner exception
Comment on attachment 285230 [details] [diff] [review]
We can't throw for DOMParser parsing errors.
*** Bug 837463 has been marked as a duplicate of this bug. ***
What's the status of this now? I filed 837463 and it got marked as a duplicate of this, but there hasn't been any activity here for nearly a year.