272011 - crash on adding to innerHTML in xhtml [@nsXMLContentSink::AddContentAsLeaf]

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Rene Pronk]

I got Mozilla 1.8a5 to crash on adding a string to an innerHTML in an xhtml (see
the upcoming testcase). It has 3 testcases in it, the first one works fine but
the second and third one both crash consistently but each with a different
callstack. I'm not if we're talking about 2 different bugs here or actually the
same bug so i'll open a new bug on the second crash if needed.


testcase 2

NTDLL! 77f75a58()
nsDebugImpl::Assertion(nsDebugImpl * const 0x002e6da0, const char * 0x01a1e9bc,
const char * 0x01a1e9a0, const char * 0x01a1e950, int 182) line 290
nsDebug::Assertion(const char * 0x01a1e9bc, const char * 0x01a1e9a0, const char
* 0x01a1e950, int 182) line 109
nsXMLFragmentContentSink::DidBuildModel(nsXMLFragmentContentSink * const
0x032aa640) line 182 + 56 bytes
nsExpatDriver::DidBuildModel(nsExpatDriver * const 0x032c34a8, unsigned int
2152596471, int 1, nsIParser * 0x032a1f88, nsIContentSink * 0x032aa640) line
1061 + 12 bytes
nsParser::DidBuildModel(unsigned int 2152596471) line 1316 + 46 bytes
nsParser::Terminate(nsParser * const 0x032a1f88) line 1406
nsParser::Tokenize(int 0) line 2717 + 12 bytes
nsParser::ResumeParse(int 0, int 0, int 0) line 1893 + 31 bytes
nsParser::Parse(nsParser * const 0x032a1f88, const nsAString & {...}, void *
0x0012d1a4, const nsACString & {...}, int 0, int 1, nsDTDMode
eDTDMode_full_standards) line 1757 + 17 bytes
nsParser::ParseFragment(nsParser * const 0x032a1f88, const nsAString & {...},
void * 0x00000000, nsVoidArray & {...}, int 1, const nsACString & {...},
nsDTDMode eDTDMode_full_standards) line 1825 + 99 bytes
nsRange::CreateContextualFragment(nsRange * const 0x032a1ecc, const nsAString &
{...}, nsIDOMDocumentFragment * * 0x0012d8f0) line 2499 + 69 bytes
nsGenericHTMLElement::SetInnerHTML(const nsAString & {...}) line 954 + 51 bytes
nsGenericHTMLElementTearoff::SetInnerHTML(nsGenericHTMLElementTearoff * const
0x032a1e80, const nsAString & {...}) line 215 + 27 bytes
XPTC_InvokeByIndex(nsISupports * 0x032a1e80, unsigned int 9, unsigned int 1,
nsXPTCVariant * 0x0012da94) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_SETTER) line 2034 + 43 bytes
XPCWrappedNative::SetAttribute(XPCCallContext & {...}) line 1890 + 14 bytes
XPC_WN_GetterSetter(JSContext * 0x02b94290, JSObject * 0x03243d28, unsigned int
1, long * 0x032a30f0, long * 0x0012dd78) line 1311 + 12 bytes
js_Invoke(JSContext * 0x02b94290, unsigned int 1, unsigned int 2) line 1286 + 23
bytes
js_InternalInvoke(JSContext * 0x02b94290, JSObject * 0x03243d28, long 52706616,
unsigned int 0, unsigned int 1, long * 0x0012e81c, long * 0x0012e81c) line 1383
+ 20 bytes
js_InternalGetOrSet(JSContext * 0x02b94290, JSObject * 0x03243d28, long
43103440, long 52706616, int 8, unsigned int 1, long * 0x0012e81c, long *
0x0012e81c) line 1426 + 31 bytes
js_SetProperty(JSContext * 0x02b94290, JSObject * 0x03243d28, long 43103440,
long * 0x0012e81c) line 2776 + 53 bytes
js_Interpret(JSContext * 0x02b94290, long * 0x0012e9c0) line 3456 + 1594 bytes
js_Invoke(JSContext * 0x02b94290, unsigned int 1, unsigned int 2) line 1306 + 13
bytes
js_InternalInvoke(JSContext * 0x02b94290, JSObject * 0x02c72c80, long 52705888,
unsigned int 0, unsigned int 1, long * 0x0012ebb4, long * 0x0012ebb0) line 1383
+ 20 bytes
JS_CallFunctionValue(JSContext * 0x02b94290, JSObject * 0x02c72c80, long
52705888, unsigned int 1, long * 0x0012ebb4, long * 0x0012ebb0) line 3767 + 31 bytes
nsJSContext::CallEventHandler(JSObject * 0x02c72c80, JSObject * 0x03243a60,
unsigned int 1, long * 0x0012ebb4, long * 0x0012ebb0) line 1344 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x032b1650, nsIDOMEvent
* 0x032b3970) line 175 + 51 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x032b1710,
nsIDOMEvent * 0x032b3970, nsIDOMEventTarget * 0x030c5704, unsigned int 1,
unsigned int 7) line 1512 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x032b3bc0,
nsPresContext * 0x0327c360, nsEvent * 0x0012ef30, nsIDOMEvent * * 0x0012ee98,
nsIDOMEventTarget * 0x030c5704, unsigned int 7, nsEventStatus * 0x0012ef58) line
1606
GlobalWindowImpl::HandleDOMEvent(nsPresContext * 0x0327c360, nsEvent *
0x0012ef30, nsIDOMEvent * * 0x0012ee98, unsigned int 7, nsEventStatus *
0x0012ef58) line 908
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x02f24830, unsigned
int 0) line 890 + 35 bytes
nsDocShell::EndPageLoad(nsIWebProgress * 0x030e067c, nsIChannel * 0x029deab0,
unsigned int 0) line 4302
nsWebShell::EndPageLoad(nsIWebProgress * 0x030e067c, nsIChannel * 0x029deab0,
unsigned int 0) line 750
nsDocShell::OnStateChange(nsDocShell * const 0x030c5204, nsIWebProgress *
0x030e067c, nsIRequest * 0x029deab0, unsigned int 131088, unsigned int 0) line 4229
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x030e067c, nsIRequest *
0x029deab0, int 131088, unsigned int 0) line 1225
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x029deab0, unsigned int 0)
line 832
nsDocLoaderImpl::DocLoaderIsEmpty() line 729
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x030e066c, nsIRequest *
0x029deab0, nsISupports * 0x00000000, unsigned int 0) line 661
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x030e0898, nsIRequest *
0x029deab0, nsISupports * 0x00000000, unsigned int 0) line 701 + 44 bytes
nsFileChannel::OnStopRequest(nsFileChannel * const 0x029deab8, nsIRequest *
0x0301b108, nsISupports * 0x00000000, unsigned int 0) line 547
nsInputStreamPump::OnStateStop() line 505
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x0301b10c,
nsIAsyncInputStream * 0x029ded38) line 341 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x0301b324) line 119
PL_HandleEvent(PLEvent * 0x0301b324) line 692 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00abbf88) line 627 + 9 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x00abbec0) line
398 + 12 bytes
nsWindow::DispatchPendingEvents() line 3721
nsWindow::ProcessMessage(unsigned int 256, unsigned int 116, long 4128769, long
* 0x0012fc4c) line 3990
nsWindow::WindowProc(HWND__ * 0x002001fa, unsigned int 256, unsigned int 116,
long 4128769) line 1355 + 27 bytes
USER32! 77d43a50()
USER32! 77d43b1f()
USER32! 77d43d79()
USER32! 77d43ddf()
nsAppStartup::Run(nsAppStartup * const 0x00b78530) line 216
main1(int 1, char * * 0x002e2638, nsISupports * 0x00abeb08) line 1321 + 32 bytes
main(int 1, char * * 0x002e2638) line 1799 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e8141a()



testcase 3

NTDLL! 77f75a58()
nsDebugImpl::Assertion(nsDebugImpl * const 0x002e6da0, const char * 0x01994668
`string', const char * 0x019946ac `string', const char * 0x01995d18 `string',
int 712) line 290
nsDebug::Assertion(const char * 0x01994668 `string', const char * 0x019946ac
`string', const char * 0x01995d18 `string', int 712) line 109
nsCOMPtr<nsIDOMDocument>::operator->() line 712 + 34 bytes
nsXMLContentSink::AddContentAsLeaf(nsIContent * 0x03255878) line 602 + 41 bytes
nsXMLContentSink::FlushText(int 1, int * 0x00000000) line 781
nsXMLContentSink::HandleStartElement(nsXMLContentSink * const 0x03255bd0, const
unsigned short * 0x01a20114, const unsigned short * * 0x0012cc20, unsigned int
1, int -1, unsigned int 4294967295) line 1004
nsXMLContentSink::ReportErrorFrom(nsXMLContentSink * const 0x03255b78, const
unsigned short * 0x032583f0, const unsigned short * 0x0012cdd8, nsIDOMNode *
0x032556fc) line 1358 + 49 bytes
nsXMLFragmentContentSink::ReportError(nsXMLFragmentContentSink * const
0x03255bd0, const unsigned short * 0x032583f0, const unsigned short *
0x0012cdd8) line 299 + 33 bytes
nsExpatDriver::HandleError(const char * 0x0326b3d0, unsigned int 208, int 0)
line 825
nsExpatDriver::ParseBuffer(const char * 0x0326b3d0, unsigned int 208, int 0)
line 844
nsExpatDriver::ConsumeToken(nsExpatDriver * const 0x0326b4dc, nsScanner & {...},
int & 0) line 957 + 32 bytes
nsParser::Tokenize(int 0) line 2710 + 26 bytes
nsParser::ResumeParse(int 0, int 0, int 0) line 1893 + 31 bytes
nsParser::Parse(nsParser * const 0x032559b0, const nsAString & {...}, void *
0x0012d1a4, const nsACString & {...}, int 0, int 1, nsDTDMode
eDTDMode_full_standards) line 1757 + 17 bytes
nsParser::ParseFragment(nsParser * const 0x032559b0, const nsAString & {...},
void * 0x00000000, nsVoidArray & {...}, int 1, const nsACString & {...},
nsDTDMode eDTDMode_full_standards) line 1825 + 99 bytes
nsRange::CreateContextualFragment(nsRange * const 0x03252bc4, const nsAString &
{...}, nsIDOMDocumentFragment * * 0x0012d8f0) line 2499 + 69 bytes
nsGenericHTMLElement::SetInnerHTML(const nsAString & {...}) line 954 + 51 bytes
nsGenericHTMLElementTearoff::SetInnerHTML(nsGenericHTMLElementTearoff * const
0x03252ab0, const nsAString & {...}) line 215 + 27 bytes
XPTC_InvokeByIndex(nsISupports * 0x03252ab0, unsigned int 9, unsigned int 1,
nsXPTCVariant * 0x0012da94) line 102
XPCWrappedNative::CallMethod(XPCCallContext & {...}, XPCWrappedNative::CallMode
CALL_SETTER) line 2034 + 43 bytes
XPCWrappedNative::SetAttribute(XPCCallContext & {...}) line 1890 + 14 bytes
XPC_WN_GetterSetter(JSContext * 0x030dad08, JSObject * 0x0326ea50, unsigned int
1, long * 0x0324dcd0, long * 0x0012dd78) line 1311 + 12 bytes
js_Invoke(JSContext * 0x030dad08, unsigned int 1, unsigned int 2) line 1286 + 23
bytes
js_InternalInvoke(JSContext * 0x030dad08, JSObject * 0x0326ea50, long 52882032,
unsigned int 0, unsigned int 1, long * 0x0012e81c, long * 0x0012e81c) line 1383
+ 20 bytes
js_InternalGetOrSet(JSContext * 0x030dad08, JSObject * 0x0326ea50, long
43103568, long 52882032, int 8, unsigned int 1, long * 0x0012e81c, long *
0x0012e81c) line 1426 + 31 bytes
js_SetProperty(JSContext * 0x030dad08, JSObject * 0x0326ea50, long 43103568,
long * 0x0012e81c) line 2776 + 53 bytes
js_Interpret(JSContext * 0x030dad08, long * 0x0012e9c0) line 3456 + 1594 bytes
js_Invoke(JSContext * 0x030dad08, unsigned int 1, unsigned int 2) line 1306 + 13
bytes
js_InternalInvoke(JSContext * 0x030dad08, JSObject * 0x02c73080, long 51987128,
unsigned int 0, unsigned int 1, long * 0x0012ebb4, long * 0x0012ebb0) line 1383
+ 20 bytes
JS_CallFunctionValue(JSContext * 0x030dad08, JSObject * 0x02c73080, long
51987128, unsigned int 1, long * 0x0012ebb4, long * 0x0012ebb0) line 3767 + 31 bytes
nsJSContext::CallEventHandler(JSObject * 0x02c73080, JSObject * 0x031942b8,
unsigned int 1, long * 0x0012ebb4, long * 0x0012ebb0) line 1344 + 33 bytes
nsJSEventListener::HandleEvent(nsJSEventListener * const 0x03259240, nsIDOMEvent
* 0x03237d80) line 175 + 51 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x03259300,
nsIDOMEvent * 0x03237d80, nsIDOMEventTarget * 0x030da43c, unsigned int 1,
unsigned int 7) line 1512 + 20 bytes
nsEventListenerManager::HandleEvent(nsEventListenerManager * const 0x03251b78,
nsPresContext * 0x029dad58, nsEvent * 0x0012ef30, nsIDOMEvent * * 0x0012ee98,
nsIDOMEventTarget * 0x030da43c, unsigned int 7, nsEventStatus * 0x0012ef58) line
1606
GlobalWindowImpl::HandleDOMEvent(nsPresContext * 0x029dad58, nsEvent *
0x0012ef30, nsIDOMEvent * * 0x0012ee98, unsigned int 7, nsEventStatus *
0x0012ef58) line 908
DocumentViewerImpl::LoadComplete(DocumentViewerImpl * const 0x03235fa8, unsigned
int 0) line 890 + 35 bytes
nsDocShell::EndPageLoad(nsIWebProgress * 0x030e05fc, nsIChannel * 0x03224f30,
unsigned int 0) line 4302
nsWebShell::EndPageLoad(nsIWebProgress * 0x030e05fc, nsIChannel * 0x03224f30,
unsigned int 0) line 750
nsDocShell::OnStateChange(nsDocShell * const 0x030d9f04, nsIWebProgress *
0x030e05fc, nsIRequest * 0x03224f30, unsigned int 131088, unsigned int 0) line 4229
nsDocLoaderImpl::FireOnStateChange(nsIWebProgress * 0x030e05fc, nsIRequest *
0x03224f30, int 131088, unsigned int 0) line 1225
nsDocLoaderImpl::doStopDocumentLoad(nsIRequest * 0x03224f30, unsigned int 0)
line 832
nsDocLoaderImpl::DocLoaderIsEmpty() line 729
nsDocLoaderImpl::OnStopRequest(nsDocLoaderImpl * const 0x030e05ec, nsIRequest *
0x03224f30, nsISupports * 0x00000000, unsigned int 0) line 661
nsLoadGroup::RemoveRequest(nsLoadGroup * const 0x030e0818, nsIRequest *
0x03224f30, nsISupports * 0x00000000, unsigned int 0) line 701 + 44 bytes
nsFileChannel::OnStopRequest(nsFileChannel * const 0x03224f38, nsIRequest *
0x032256e0, nsISupports * 0x00000000, unsigned int 0) line 547
nsInputStreamPump::OnStateStop() line 505
nsInputStreamPump::OnInputStreamReady(nsInputStreamPump * const 0x032256e4,
nsIAsyncInputStream * 0x03225220) line 341 + 11 bytes
nsInputStreamReadyEvent::EventHandler(PLEvent * 0x032258fc) line 119
PL_HandleEvent(PLEvent * 0x032258fc) line 692 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00abbf88) line 627 + 9 bytes
nsEventQueueImpl::ProcessPendingEvents(nsEventQueueImpl * const 0x00abbec0) line
398 + 12 bytes
nsWindow::DispatchPendingEvents() line 3721
nsWindow::ProcessMessage(unsigned int 256, unsigned int 13, long 1835009, long *
0x0012fc4c) line 3990
nsWindow::WindowProc(HWND__ * 0x001901ae, unsigned int 256, unsigned int 13,
long 1835009) line 1355 + 27 bytes
USER32! 77d43a50()
USER32! 77d43b1f()
USER32! 77d43d79()
USER32! 77d43ddf()
nsAppStartup::Run(nsAppStartup * const 0x00b78530) line 216
main1(int 1, char * * 0x002e2638, nsISupports * 0x00abeb08) line 1321 + 32 bytes
main(int 1, char * * 0x002e2638) line 1799 + 37 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! 77e8141a()

Comment 1

[Rene Pronk]

Attachment: testcase

Comment 2

[Rene Pronk]

Man, what was I thinking... The second testcase only asserts but doesn't crash
at all. Only the third one actually crashes. New testcase coming up. Changing
component to HTML:Parser which is better I think.

It crashes on the domDoc->AppendChild because domDoc and mDocument are null.

nsresult
nsXMLContentSink::AddContentAsLeaf(nsIContent *aContent)
{
  nsresult result = NS_OK;

  if ((eXMLContentSinkState_InProlog == mState) ||
      (eXMLContentSinkState_InEpilog == mState)) {
    nsCOMPtr<nsIDOMDocument> domDoc( do_QueryInterface(mDocument) );
    nsCOMPtr<nsIDOMNode> trash;
    nsCOMPtr<nsIDOMNode> child( do_QueryInterface(aContent) );
    NS_ASSERTION(child, "not a dom node");
    domDoc->AppendChild(child, getter_AddRefs(trash));
  }

Comment 3

[Rene Pronk]

Attachment: testcase (crashes!)

Comment 4

[Rene Pronk]

Attachment: patch

With this patch Mozilla no longer crashes and testcase 2 and 3 now give exactly
the same assertion.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This looks wrong.  Why are we getting into AddContentAsLeaf at all here? We
should be ending up with an XML error earlier on and bailing out of things....
(and probably throwing an appropriate exception from SetInnerHTML).

Comment 6

[Rene Pronk]

Relevant callstack:

nsCOMPtr<nsIDOMDocument>::operator->() line 712 + 34 bytes
nsXMLContentSink::AddContentAsLeaf(nsIContent * 0x0321f038) line 603 + 41 bytes
nsXMLContentSink::FlushText(int 1, int * 0x00000000) line 783
nsXMLContentSink::HandleStartElement(nsXMLContentSink * const 0x03203ef0, const
unsigned short * 0x01510114, const unsigned short * * 0x0012cc20, unsigned int
1, int -1, unsigned int 4294967295) line 1006
nsXMLContentSink::ReportErrorFrom(nsXMLContentSink * const 0x03203e98, const
unsigned short * 0x032027b0, const unsigned short * 0x03202898, nsIDOMNode *
0x03204c4c) line 1360 + 49 bytes
nsXMLFragmentContentSink::ReportError(nsXMLFragmentContentSink * const
0x03203ef0, const unsigned short * 0x032027b0, const unsigned short *
0x03202898) line 299 + 33 bytes
nsExpatDriver::HandleError(const char * 0x0321eac8, unsigned int 1078, int 0)
line 825
nsExpatDriver::ParseBuffer(const char * 0x0321eac8, unsigned int 1078, int 0)
line 844


The problem seems to be the following: Mozilla perfectly detects the error and
wants to send a parse error to the browser saying that it expects a </br> tag.
The error message is created in nsExpatDriver::HandleError. In 
nsXMLContentSink::ReportErrorFrom the xml for the error is created using calls
to HandleStartElement and HandleStartElement calls FlushText which tries to
flush the remaining "hhh" from the testcase which fails miserably.

I'm not really sure what the right solution would be here. I guess it depends on
how you want to handle the error. Writing the xml parse error to the innerHTML
doesn't really seem like the right thing to do to me but I guess that is a bit
personal. Firefox 1.0 silently ignores the problem and shows nothing, IE shows a
Javascript error.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Firefox 1.0 has no support for innerHTML on XML nodes.

The right thing to do is to throw an error return and discard the
partially-parsed DOM.  We really shouldn't be ending up in the error-reporting
in this case...

Comment 8

[steve.swanson]

Attachment: patch, detecting condition closer to parser

My suggestion for detecting the situation in
nsXMLContentSink::ReportErrorFrom() instead of down in the guts of node
creation.  Net effect is the same as the other patch.

The assertion in nsXMLFragmentContentSink::DidBuildModel() remains.  I don't
think this is a problem as it looks like all the content is cleaned up and the
parser is returning an error anyway.

I don't know anything about setInnerHTML()...

Comment 9

[Boris Zbarsky [:bzbarsky]]

steve, SetInnerHTML is just a thin wrapper around CreateContextualFragment
followed by appending the data from the fragment.  See
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsGenericHTMLElement.cpp#954
for that part of the implementation.

So createContextualFragment needs to propagate out this error when it happens,
basically.

Comment 10

[steve.swanson]

I don't think createContextualFragment is hiding any errors.  During the 
parse, nsParser::ResumeParse() gets a theTokenizerResult of 
NS_ERROR_HTMLPARSER_STOPPARSING. It then returns NS_OK. (So far) I don't see 
any other errors being lost in the call chain.

Based on the documentation ;-) it's not clear what errors would come out of 
using innerHTML.  The basic behavior (with the patch) to not change the 
document seems right.  Who should keep the error: the content sink or the 
parser?  In the normal XML load case, the error becomes the new document and 
everyone says NS_OK.

Comment 11

[Boris Zbarsky [:bzbarsky]]

The behavior I expect to see if I call createContextualFragment() on malformed
XML is an error return out of createContextualFragment (which will translate
into a JS exception when innerHTML is being set).

So we need to fix things to:

1)  Not create an error document when we're doing innerHTML parsing
2)  Propagate the content-sink error as needed.

Perhaps the easiest thing to do #2 is to return an error from GetFragment() on
the sink.  Since the sink knows a well-formedness error happened (it gets
notified by the expat driver) it can set a member accordingly to return a null
fragment and return an error when GetFragment() is called.

Comment 12

[steve.swanson]

Attachment: improved patch, and parse error travels to JavaScript

This patch addresses the issues raised above.  Two lesser issues have come up

1. nsXMLFragmentContentSink::GetFragment() now returns NS_ERROR_FAILURE if
there was a parsing error.  Is there a better error code?  (The
NS_ERROR_HTMLPARSER_* errors seemed inappropriate.)

2. Should I just rewrite nsXMLFragmentContentSink::ReportError(), not reusing
the base class code? The behaviors have diverged.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Is there a better error code?

I think the right thing to do here is to add an error code corresponding to the
DOM level 3 PARSE_ERR error code (see
http://w3.org/TR/2004/REC-DOM-Level-3-LS-20040407/load-save.html#LS-LSException)
to dom/public/nsDOMError.h and dom/src/base/domerr.msg

jst, do these need to be in their own error module, or in NS_ERROR_MODULE_DOM?

> Should I just rewrite nsXMLFragmentContentSink::ReportError(

Your call.... If it'd be small enough on its own, it may be clearer to do that, yes.

Comment 14

[steve.swanson]

Attachment: better error code, rewrite nsXMLFragmentContentSink::ParseError()

Now returns NS_ERROR_DOM_SYNTAX_ERR on XML parsing error. This seems like the
best error currently available. Decoupled
nsXMLFragmentContentSink::ParseError() from base class version.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167741 [details] [diff] [review]
better error code, rewrite nsXMLFragmentContentSink::ParseError()

>Index: xml/document/src/nsXMLFragmentContentSink.cpp
>+  PRBool                mParseError;
> 
>   // if FALSE, take content inside endnote tag
>   PRBool                mAllContent;

Make both of those PRPackedBool?

I guess we want to report to console so page authors have better feedback on
what went wrong?

Apart from that, looks great!  I still think we want to use the load/save error
codes here eventually, though.	Maybe just add an XXX comment about that?

Comment 16

[steve.swanson]

Attachment: minor corrections to above

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 167766 [details] [diff] [review]
minor corrections to above

r+sr=bzbarsky, but is the nsContentUtils include really needed?  I don't see it
being used....

Comment 18

[Boris Zbarsky [:bzbarsky]]

Checked in to the trunk for 1.8a6, with the nsContentUtils include replaced by
the one that was actually needed (nsServiceManagerUtils).

Steve, thanks for the patch!

Comment 19

[Peter Van der Beken [:peterv]]

Comment on attachment 167766 [details] [diff] [review]
minor corrections to above

> nsXMLFragmentContentSink::GetFragment(nsIDOMDocumentFragment** aFragment)
> {
>-  if (mRoot) {
>+  *aFragment = nsnull;
>+  if (mParseError) {
>+    //XXX PARSE_ERR from DOM3 Load and Save would be more appropriate
>+    return NS_ERROR_DOM_SYNTAX_ERR;
>+  } else if (mRoot) {

else after return

>     return CallQueryInterface(mRoot, aFragment);
>+  } else {

else after return

>+    return NS_OK;