[FIX]Crash with innerHTML in xul document

VERIFIED FIXED in mozilla1.8beta2

Status

()

Core
HTML: Parser
P1
critical
VERIFIED FIXED
13 years ago
13 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: bz)

Tracking

({crash, regression, testcase})

Trunk
mozilla1.8beta2
crash, regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 179303 [details]
Testcase (crashes on load)
(Reporter)

Comment 2

13 years ago
It looks like a regression. It doesn't crash in 2005021806 Mozilla build. It
crashes in 2005021906 Mozilla build.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-02-18+05%3A00%3A00&maxdate=2005-02-19+07%3A00%3A00&cvsroot=%2Fcvsroot
Keywords: crash, regression
(Reporter)

Comment 3

13 years ago
Created attachment 179308 [details]
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).
Created attachment 179317 [details] [diff] [review]
Patch

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)

Comment 10

13 years ago
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().
Created attachment 179650 [details] [diff] [review]
Patch updated to comments
Created attachment 179651 [details] [diff] [review]
Real patch updated to comments
Attachment #179317 - Attachment is obsolete: true
Attachment #179650 - Attachment is obsolete: true
Fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 17

13 years ago
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.