Closed Bug 502973 Opened 11 years ago Closed 11 years ago

[HTML5] Crash [@ nsHtml5TreeBuilder::popOnEof] with 2 iframes with invalid protocol url and closing tab

Categories

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

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: hsivonen)

References

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files, 1 obsolete file)

Attached file testcase
See testcase, to get this crash, you need to have the html5.enable pref set to
true.

To reproduce:
- have at least 2 tabs open
- Open in one of the tabs the testcase, 2 alerts will appear
- Click the first one away
- Now you can close the tab of the testcase (at least in windows), by clicking on the close button of the tab

Result:
crash

http://crash-stats.mozilla.com/report/index/29b326fc-001f-4e3e-b8bd-324bc2090707?p=1
0  	xul.dll  	nsHtml5TreeBuilder::popOnEof  	 parser/html/nsHtml5TreeBuilder.cpp:3283
1 	xul.dll 	nsHtml5TreeBuilder::eof 	parser/html/nsHtml5TreeBuilder.cpp:528
2 	xul.dll 	nsHtml5Tokenizer::eof 	parser/html/nsHtml5Tokenizer.cpp:3129
3 	xul.dll 	nsHtml5Parser::DidBuildModel 	parser/html/nsHtml5Parser.cpp:788
4 	xul.dll 	nsHtml5Parser::Terminate 	parser/html/nsHtml5Parser.cpp:475
5 	xul.dll 	xul.dll@0x35bf2c 	
6 	xul.dll 	nsDocShell::Stop 	docshell/base/nsDocShell.cpp:3958
7 	xul.dll 	nsDocShell::Stop 	docshell/base/nsDocShell.cpp:3986
8 	xul.dll 	nsDocShell::Stop 	docshell/base/nsDocShell.cpp:3986
9 	xul.dll 	nsDocShell::Destroy 	docshell/base/nsDocShell.cpp:4257
10 	xul.dll 	nsXULWindow::Destroy 	xpfe/appshell/src/nsXULWindow.cpp:524
11 	xul.dll 	nsWebShellWindow::Destroy 	xpfe/appshell/src/nsWebShellWindow.cpp:771
12 	xul.dll 	xul.dll@0x3d51e6 	
13 	xul.dll 	nsWindow::DispatchEvent 	widget/src/windows/nsWindow.cpp:2938
14 	xul.dll 	nsWindow::DispatchWindowEvent 	widget/src/windows/nsWindow.cpp:2966
15 	xul.dll 	nsWindow::DispatchStandardEvent 	widget/src/windows/nsWindow.cpp:2959
16 	xul.dll 	xul.dll@0x42acbc 	
17 	xul.dll 	nsWindow::WindowProc 	widget/src/windows/nsWindow.cpp:3555
18 	xul.dll 	xul.dll@0x34e631 	
19 		@0xf 

The steps to reproduce don't work on the Mac, I noticed, because the second dialog stays modal (as it should be), but with quite some effort, I can crash there too.
Ok, with html5.enable set to false, the second alert stays modal, so I guess that should be fixed, to fix the crash.
Attached file testcase2
It also crashes with this stacktrace in this testcase (this time directly).
The content of the iframe is this:
<script>window.addEventListener('DOMAttrModified', function() {window.frameElement.parentNode.removeChild(window.frameElement);}, true);</script>
<video></video>
It seems like the video tag is adding attributes at bad times, which might be related to bug 495446, I guees.

http://crash-stats.mozilla.com/report/index/7d7193a7-9871-4da7-87ce-e0b4c2090707
0  	XUL  	nsHtml5TreeBuilder::popOnEof  	 parser/html/nsHtml5TreeBuilder.cpp:3283
1 	XUL 	nsHtml5Tokenizer::eof 	parser/html/nsHtml5Tokenizer.cpp:3129
2 	XUL 	nsHtml5Parser::DidBuildModel 	parser/html/nsHtml5Parser.cpp:788
3 	XUL 	nsHtml5Parser::Terminate 	parser/html/nsHtml5Parser.cpp:475
4 	XUL 	DocumentViewerImpl::Stop 	layout/base/nsDocumentViewer.cpp:1570
5 	XUL 	nsDocShell::Stop 	docshell/base/nsDocShell.cpp:3958
6 	XUL 	nsDocShell::Destroy 	docshell/base/nsDocShell.cpp:4257
7 	XUL 	nsFrameLoader::Finalize 	content/base/src/nsFrameLoader.cpp:291
8 	XUL 	nsDocument::MaybeInitializeFinalizeFrameLoaders 	content/base/src/nsDocument.cpp:5195
9 	XUL 	nsDocument::EndUpdate 	content/base/src/nsDocument.cpp:3739
10 	XUL 	nsHTMLDocument::EndUpdate 	content/html/document/src/nsHTMLDocument.cpp:3020
11 	XUL 	nsGenericElement::doRemoveChildAt 	content/base/src/mozAutoDocUpdate.h:66
12 	XUL 	nsGenericElement::RemoveChildAt 	content/base/src/nsGenericElement.cpp:3256
13 	XUL 	nsGenericElement::doRemoveChild 	content/base/src/nsGenericElement.cpp:3935
14 	XUL 	nsGenericElement::RemoveChild 	content/base/src/nsGenericElement.cpp:3493
15 	XUL 	nsIDOMNode_RemoveChild 	dom_quickstubs.cpp:4183
16 	libmozjs.dylib 	js_Interpret 	js/src/jsinterp.cpp:5210
17 	libmozjs.dylib 	js_Invoke 	js/src/jsinterp.cpp:1397
18 	XUL 	nsXPCWrappedJSClass::CallMethod 	js/src/xpconnect/src/xpcwrappedjsclass.cpp:1647
19 	XUL 	nsXPCWrappedJS::CallMethod 	js/src/xpconnect/src/xpcwrappedjs.cpp:569
20 	XUL 	PrepareAndDispatch 	xpcom/reflect/xptcall/src/md/unix/xptcstubs_unixish_x86.cpp:93
21 	XUL 	PrepareAndDispatch 	
22 	XUL 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:1034
23 	XUL 	nsEventListenerManager::HandleEvent 	content/events/src/nsEventListenerManager.cpp:1140
Attached file testcase3
Ok, I see this crash also happening in this case. In this case the iframe content is this:
<script>window.addEventListener('DOMAttrModified', function() {window.frameElement.parentNode.removeChild(window.frameElement);}, true);</script>
<html id="">
<span></span>
I think Terminate() probably shouldn't call DidBuildModel synchronously but should instead schedule it to be called from the event loop.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #393753 - Flags: review?(mrbkap)
Comment on attachment 393753 [details] [diff] [review]
When Terminate is called, schedule the termination to a later time

Oops. This patch is bogus. Sorry.
Attachment #393753 - Attachment is obsolete: true
Attachment #393753 - Flags: review?(mrbkap)
Comment on attachment 393753 [details] [diff] [review]
When Terminate is called, schedule the termination to a later time

Meant to obsolete the bug from bug 502869...
Attachment #393753 - Attachment is obsolete: false
Attachment #393753 - Flags: review?(mrbkap)
Comment on attachment 393753 [details] [diff] [review]
When Terminate is called, schedule the termination to a later time

> NS_IMETHODIMP
> nsHtml5Parser::Terminate(void)
> {
>-  // We should only call DidBuildModel once, so don't do anything if this is
>-  // the second time that Terminate has been called.
>-  if (mLifeCycle == TERMINATED) {
>+  if (mLifeCycle >= WILL_TERMINATE) {
>     return NS_OK;
>   }
>-  // XXX - [ until we figure out a way to break parser-sink circularity ]
>-  // Hack - Hold a reference until we are completely done...
>-  nsCOMPtr<nsIParser> kungFuDeathGrip(this);
>-  // CancelParsingEvents must be called to avoid leaking the nsParser object
>-  // @see bug 108049
>-  CancelParsingEvents();
>-#ifdef DEBUG
>-  PRBool ready =
>-#endif
>-  ReadyToCallDidBuildModelImpl(PR_TRUE);
>-  NS_ASSERTION(ready, "Should always be ready to call DidBuildModel here.");
>-  return DidBuildModel();
>+  mLifeCycle = WILL_TERMINATE;
>+  MaybePostContinueEvent();
>+  return NS_OK;
> }

I don't think you can do this. We have code (see nsHTMLDocument::WriteCommon) that depends on nsIParser::Terminate calling DidBuildModel (in that case, this is represented as mParser having been nulled out).
Attachment #393753 - Flags: review?(mrbkap) → review-
I see. However, the current HTML5 parser behavior of processing the EOF token synchronously upon nsIParser::Terminate is unacceptable, since it leads to a crash. I'll try a solution where ::Terminate doesn't emit EOF at all.
Attached patch Potential fixSplinter Review
Can't test on Mac if this fixes it. Attaching while waiting for a Windows tryserver build to avoid losing this diff.
Attachment #393753 - Attachment is obsolete: true
Blocks: 506842
Attachment #394496 - Flags: review?(mrbkap)
Comment on attachment 394496 [details] [diff] [review]
Potential fix

I can't reproduce the crash on Windows with or without the patch, but let's try this patch anyway.
No longer blocks: 506842
Comment on attachment 394496 [details] [diff] [review]
Potential fix

Looks reasonable.
Attachment #394496 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/mozilla-central/rev/9798dcc4a19d

In case the HTML5 parser is in 3.6 preffed off (as opposed to removed), this one would be nice to have on the branch, too. Should be low-risk, since this code doesn't run if html5.enable=false.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: wanted1.9.2?
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Duplicate of this bug: 507703
Duplicate of this bug: 503808
Flags: wanted1.9.2?
Crash Signature: [@ nsHtml5TreeBuilder::popOnEof]
You need to log in before you can comment on or make changes to this bug.