Closed Bug 288674 Opened 19 years ago Closed 19 years ago

[FIX]Crash with innerHTML in xul document

Categories

(Core :: DOM: HTML Parser, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8beta2

People

(Reporter: martijn.martijn, Assigned: bzbarsky)

Details

(Keywords: crash, regression, testcase)

Attachments

(3 files, 2 obsolete files)

See testcase. 
Basically the testcase is a xul document with this script code:
var toggle_div=document.createElementNS('http://www.w3.org/1999/xhtml','div');
toggle_div.innerHTML='css_props ';

This causes a crash in my trunk build.
Attached file Bactrace
This is a backtrace from my debug build.
The only relevant checkin in that range is Blake's cleanup of <endnote> stuff,
looks like...

Note that the problem is likely to be that there is an XML syntax error here.

Also note that we're never asserting till we get to that null nsCOMPtr, so we
_do_ have an mDocument at some point, and then it gets cleared...
OS: Windows XP → All
Hardware: PC → All
Ok, nevermind that last part of comment 4.  We never set mDocument on
nsXMLFragmentContentSink.  For the reason, see discussion in bug 272011.

At the time I asked whether one could wean nsXMLContentSink from relying on
mDocument, but it doesn't look like that quite happened.  We'll crash for leaves
added in the prologue or epilogue (which is what happens here) and I think we'll
crash for <html:title> nodes.

Blake, do you have time to add the relevant null-checks?  If not, maybe Steve
does....
For reference, the mText when we get into FlushText() is:

  "css_props"

I think we should simply reset mText and mTextLength to null and 0 respectively
when ReportError is called (in both the fragment sink and the XML sink).
Attached patch Patch (obsolete) — Splinter Review
Actually, I got antsy and just did this... Look reasonable?
Assignee: parser → bzbarsky
Status: NEW → ASSIGNED
Attachment #179317 - Flags: superreview?(peterv)
Attachment #179317 - Flags: review?(mrbkap)
Priority: -- → P1
Summary: Crash with innerHTML in xul document → [FIX]Crash with innerHTML in xul document
Target Milestone: --- → mozilla1.8beta2
Comment on attachment 179317 [details] [diff] [review]
Patch

Maybe nsXMLFragmentContentSink::DidBuildContent() should only call FlushText()
and PopContent() if mParseError is false? r=mrbkap either way.
Attachment #179317 - Flags: review?(mrbkap) → review+
Comment on attachment 179317 [details] [diff] [review]
Patch

jst, could you take a look?
Attachment #179317 - Flags: superreview?(peterv) → superreview?(jst)
What about the other uses of mDocument in nsXMLContentSink::DidBuildModel()?  
I agree with comment 8. Unfortunately, I won't have access to a debugger for 
several days so I can't check things in detail until then.
> What about the other uses of mDocument in nsXMLContentSink::DidBuildModel()?  

That's never called for fragment sinks -- the fragment sink overrides that method.
Comment on attachment 179317 [details] [diff] [review]
Patch

sr=jst
Attachment #179317 - Flags: superreview?(jst) → superreview+
Actually, I think we want to PopContent() no matter what.  I'll add a check for
error around FlushText().
Attached patch Patch updated to comments (obsolete) — Splinter Review
Attachment #179317 - Attachment is obsolete: true
Attachment #179650 - Attachment is obsolete: true
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
It looks like this caused bug 289180
The crash is Verified FIXED using build 2005-04-18-05, Windows XP Seamonkey
trunk on the https://bugzilla.mozilla.org/attachment.cgi?id=179303 testcase.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: