Closed Bug 435128 Opened 16 years ago Closed 16 years ago

Crash [@ nsParser::WillTokenize] trying to write something in a destroying iframe

Categories

(Core :: DOM: HTML Parser, defect)

Product:
Core
Component:
DOM: HTML Parser
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: mrbkap)

Details

(Keywords: crash, fixed1.9.0.2, testcase, Whiteboard: [sg:nse] null deref)

Crash Data

Attachments

(3 files, 1 obsolete file)

testcase
1.38 KB, text/html
Details
Proposed fix
3.62 KB, patch
jst
: review+
jst
: superreview+
samuel.sidler+old
: approval1.9.0.2+
Details | Diff | Splinter Review
Proposed fix -w
4.50 KB, patch
Details | Diff | Splinter Review
Attached file testcase (obsolete) — 
See testcase, which can crash in current trunk build, within 100ms or a minute or so. Usually, it should crash within 30 seconds.

The iframe source consists of this:
<html><head></head><body>
<iframe id="a"></iframe>
<script>
function doe() {
var x = window.frames[0].document;

var y=document.getElementById('a');
y.parentNode.removeChild(y);

try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
try {x.write('t');} catch(e) {}
}
setTimeout('doe()',20);
</script>
</body></html>

It seems like a regression, it doesn't seem to crash on branch. But because the crash doesn't happen instantly, it's kind of difficult to get a regression range.

http://crash-stats.mozilla.com/report/index/6caad09c-2793-11dd-be79-001321b13766?p=1
0  	xul.dll  	nsParser::WillTokenize  	 mozilla/parser/htmlparser/src/nsParser.cpp:2380
1 	xul.dll 	nsParser::Tokenize 	mozilla/parser/htmlparser/src/nsParser.cpp:2419
2 	xul.dll 	nsParser::ResumeParse 	mozilla/parser/htmlparser/src/nsParser.cpp:1649
3 	xul.dll 	nsParser::Parse 	mozilla/parser/htmlparser/src/nsParser.cpp:1450
4 	xul.dll 	nsHTMLDocument::WriteCommon 	mozilla/content/html/document/src/nsHTMLDocument.cpp:2475
5 	xul.dll 	nsHTMLDocument::ScriptWriteCommon 	mozilla/content/html/document/src/nsHTMLDocument.cpp:2531
6 	mozcrt19.dll 	arena_dalloc_small 	jemalloc.c:4094
7 	xul.dll 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
8 	js3250.dll 	js_GetClassObject
Martijn, I'm having trouble making your testcase work at all. Is there any chance you could upload a full testcase to bugzilla?
Attached file testcase — 
Sorry, I attached the wrong file.
Attachment #322029 - Attachment is obsolete: true
Attached patch Proposed fix — — Splinter Review 
This is caused by calling document.open() on a document whose docshell has gone away. The crash is caused by creating a parser and then bailing out before calling SetContentSink on it (so when we get into nsHTMLDocument::Write, we have a partially-initialized parser, causing the crash).

This patch is defense-in-depth: it nulls out the parser if we couldn't create a content sink and bails out early if there's no docshell, since we won't be able to open the document successfully anyway. I also noticed that we got the docshell twice for no reason, so I took away one of the docshells.

I'll also attach a diff -w for easier reviewing.
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #327593 - Flags: superreview?(jst)
Attachment #327593 - Flags: review?(jst)
Attachment #327593 - Flags: superreview?(jst)
Attachment #327593 - Flags: superreview+
Attachment #327593 - Flags: review?(jst)
Attachment #327593 - Flags: review+
Pushed as changeset 9c2b88886f7f.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Thanks, that fixed it.
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071504 Minefield/3.1a1pre
Status: RESOLVED → VERIFIED
Comment on attachment 327593 [details] [diff] [review]
Proposed fix

This fixes a crash.
Attachment #327593 - Flags: approval1.9.0.2?
Flags: in-testsuite?
Comment on attachment 327593 [details] [diff] [review]
Proposed fix

Approved for 1.9.0.2. Please land in CVS. a=ss

Also, be sure to land a testcase for this crash after this release has gone out.
Attachment #327593 - Flags: approval1.9.0.2? → approval1.9.0.2+
Fix checked into the 1.9 branch.
Keywords: fixed1.9.0.2
Group: core-security
Whiteboard: [sg:nse] null deref
crash test added
http://hg.mozilla.org/mozilla-central/rev/0d115aa982f1
Flags: in-testsuite? → in-testsuite+
test disabled due to length of time required.
http://hg.mozilla.org/mozilla-central/rev/f46badba0e2b
Crash Signature: [@ nsParser::WillTokenize]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: