Last Comment Bug 289714 - Loaded-as-data XML documents should not generate <parsererror>
: Loaded-as-data XML documents should not generate <parsererror>
Status: NEW
:
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: x86 Linux
: P3 normal with 4 votes (vote)
: mozilla1.9beta1
Assigned To: Olli Pettay [:smaug]
:
Mentors:
: 837463 (view as bug list)
Depends on: 45566 251354 399502
Blocks: 313854
  Show dependency treegraph
 
Reported: 2005-04-09 11:17 PDT by Boris Zbarsky [:bz]
Modified: 2013-09-20 03:30 PDT (History)
20 users (show)
reed: wanted1.9+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
DOMParser throws (37.45 KB, patch)
2007-10-17 07:38 PDT, Olli Pettay [:smaug]
Ms2ger: review-
Details | Diff | Review

Description Boris Zbarsky [:bz] 2005-04-09 11:17:25 PDT
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).
Comment 1 Jonas Sicking (:sicking) 2005-04-09 12:30:07 PDT
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.
Comment 2 Boris Zbarsky [:bz] 2005-04-09 12:37:44 PDT
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?
Comment 3 Jonas Sicking (:sicking) 2005-04-09 12:54:32 PDT
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...
Comment 4 Boris Zbarsky [:bz] 2005-04-09 13:06:14 PDT
OK, makes sense.  Will look into doing this, then.
Comment 5 Jonas Sicking (:sicking) 2005-04-09 13:51:34 PDT
Oh, and of course, we should check how IE deals with errors.
Comment 6 Bob Clary [:bc:] 2005-04-09 13:54:06 PDT
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:
<http://msdn.microsoft.com/library/default.asp?url=/library/en-us/xmlsdk/html/xmproparseerror.asp>
Comment 7 Boris Zbarsky [:bz] 2005-04-09 14:00:55 PDT
> 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?).  
Comment 8 Bob Clary [:bc:] 2005-04-09 15:04:39 PDT
(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)
<http://msdn.microsoft.com/library/default.asp?url=/library/en-us/xmlsdk/html/PushModelParser_ErrorCodes.asp>
2)
<http://msdn.microsoft.com/library/default.asp?url=/library/en-us/wcexmldm/html/cerefIXMLParseErrorErrorMessages.asp>

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.

Comment 9 Boris Zbarsky [:bz] 2005-04-10 08:38:59 PDT
> True, but what error information do you get? 

Not much, really.  Just that there was an error.  Just like the return value
from document.load.

> 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.
Comment 10 Boris Zbarsky [:bz] 2007-02-04 14:40:55 PST
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.
  DOMParser:  Throw.
  Async XMLDocument.load: error event
  syncload service: throw
  XForms: ???
  XSLT:  ???

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.
Comment 11 Jonas Sicking (:sicking) 2007-03-02 17:13:05 PST
This is really annoying, there's no reason we should keep doing this for yet another release.
Comment 12 Peter Van der Beken [:peterv] 2007-06-20 11:15:26 PDT
Pushing.
Comment 13 Olli Pettay [:smaug] 2007-09-27 06:13:14 PDT
Let's see if I manage to do this for M9. I should.
Comment 14 Olli Pettay [:smaug] 2007-10-02 09:03:12 PDT
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.
Comment 15 Olli Pettay [:smaug] 2007-10-08 06:04:18 PDT
So I'm not so sure how important this is or what should be done in each case.
Comment 16 Boris Zbarsky [:bz] 2007-10-08 07:26:41 PDT
With XHR you could just set the document to null, I guess.
Comment 17 Jesse Ruderman 2007-10-10 19:51:30 PDT
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.
Comment 18 Olli Pettay [:smaug] 2007-10-15 09:50:10 PDT
Bug 399502 should be fixed first to get events from xmldoc.load()
Comment 19 Olli Pettay [:smaug] 2007-10-17 07:32:47 PDT
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.
So:
  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
        (throw nsIXSLTException)

DOMParser could also just return null document?
Comment 20 Olli Pettay [:smaug] 2007-10-17 07:38:10 PDT
Created attachment 285230 [details] [diff] [review]
DOMParser throws

Peter, any comments?
Comment 21 Jesse Ruderman 2007-10-17 10:24:11 PDT
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.
Comment 22 Olli Pettay [:smaug] 2007-10-17 10:46:43 PDT
Ah, true, that would be useful.
Comment 23 Olli Pettay [:smaug] 2007-10-17 12:11:54 PDT
Though, I don't know how to do it, or can't find any reasonable way to do it.
Comment 24 Damon Sicore (:damons) 2007-10-17 13:53:32 PDT
We've shipped this forever, right?  Why should this block beta?
Comment 25 Jonas Sicking (:sicking) 2007-10-17 13:57:30 PDT
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.
Comment 26 Damon Sicore (:damons) 2007-10-17 14:09:53 PDT
I'm minusing.  That doesn't mean it won't go in, but we need to prioritize accordingly.  Anyone who objects can renom.
Comment 27 Boris Zbarsky [:bz] 2007-10-17 14:58:26 PDT
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.
Comment 28 Jonas Sicking (:sicking) 2007-10-17 15:08:39 PDT
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.
Comment 29 Olli Pettay [:smaug] 2007-10-17 23:54:14 PDT
(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
(nsIException.inner)? Suggestions?
Comment 30 :Ms2ger 2012-02-12 06:43:15 PST
Comment on attachment 285230 [details] [diff] [review]
DOMParser throws

We can't throw for DOMParser parsing errors.
Comment 31 Robert Longson 2013-02-03 02:39:00 PST
*** Bug 837463 has been marked as a duplicate of this bug. ***
Comment 32 Brian Bi 2013-02-03 10:09:35 PST
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.

Note You need to log in before you can comment on or make changes to this bug.