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)
Core
DOM: HTML Parser
Tracking
()
VERIFIED
FIXED
mozilla1.8beta2
People
(Reporter: martijn.martijn, Assigned: bzbarsky)
Details
(Keywords: crash, regression, testcase)
Attachments
(3 files, 2 obsolete files)
336 bytes,
application/vnd.mozilla.xul+xml
|
Details | |
15.83 KB,
text/plain
|
Details | |
2.84 KB,
patch
|
Details | Diff | Splinter Review |
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•19 years ago
|
||
Reporter | ||
Comment 2•19 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•19 years ago
|
||
This is a backtrace from my debug build.
Assignee | ||
Comment 4•19 years ago
|
||
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
Assignee | ||
Comment 5•19 years ago
|
||
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....
Assignee | ||
Comment 6•19 years ago
|
||
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).
Assignee | ||
Comment 7•19 years ago
|
||
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)
Assignee | ||
Updated•19 years ago
|
Priority: -- → P1
Summary: Crash with innerHTML in xul document → [FIX]Crash with innerHTML in xul document
Target Milestone: --- → mozilla1.8beta2
Comment 8•19 years ago
|
||
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+
Assignee | ||
Comment 9•19 years ago
|
||
Comment on attachment 179317 [details] [diff] [review] Patch jst, could you take a look?
Attachment #179317 -
Flags: superreview?(peterv) → superreview?(jst)
Comment 10•19 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.
Assignee | ||
Comment 11•19 years ago
|
||
> What about the other uses of mDocument in nsXMLContentSink::DidBuildModel()?
That's never called for fragment sinks -- the fragment sink overrides that method.
Comment 12•19 years ago
|
||
Comment on attachment 179317 [details] [diff] [review] Patch sr=jst
Attachment #179317 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 13•19 years ago
|
||
Actually, I think we want to PopContent() no matter what. I'll add a check for error around FlushText().
Assignee | ||
Comment 14•19 years ago
|
||
Assignee | ||
Comment 15•19 years ago
|
||
Attachment #179317 -
Attachment is obsolete: true
Attachment #179650 -
Attachment is obsolete: true
Assignee | ||
Comment 16•19 years ago
|
||
Fixed.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment 17•19 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.
Description
•