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

Status: VERIFIED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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.

Comment 1

[Martijn Wargers (dead)]

Ok, with html5.enable set to false, the second alert stays modal, so I guess that should be fixed, to fix the crash.

Comment 2

[Martijn Wargers (dead)]

Attachment: 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

Comment 3

[Martijn Wargers (dead)]

Attachment: 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>

Comment 4

[Henri Sivonen (:hsivonen)]

I think Terminate() probably shouldn't call DidBuildModel synchronously but should instead schedule it to be called from the event loop.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: When Terminate is called, schedule the termination to a later time

Comment 6

[Henri Sivonen (:hsivonen)]

Comment on attachment 393753 [details] [diff] [review]
When Terminate is called, schedule the termination to a later time

Oops. This patch is bogus. Sorry.

Comment 7

[Henri Sivonen (:hsivonen)]

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...

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

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).

Comment 9

[Henri Sivonen (:hsivonen)]

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.

Comment 10

[Henri Sivonen (:hsivonen)]

Attachment: Potential fix

Can't test on Mac if this fixes it. Attaching while waiting for a Windows tryserver build to avoid losing this diff.

Comment 11

[Henri Sivonen (:hsivonen)]

Tryserver build dir:
https://build.mozilla.org/tryserver-builds/[e-mail]-suppresseof/

Comment 12

[Henri Sivonen (:hsivonen)]

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.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 394496 [details] [diff] [review]
Potential fix

Looks reasonable.

Comment 14

[Henri Sivonen (:hsivonen)]

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.