531106 - [HTML5][Patch] Crash in [@ nsHtml5Parser::DropStreamParser] triggered by softpedia framebreaker.

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[nemo]

Accessed this URL from minefield using google image search on the keyword "wormux"

Crash was only with HTML5 enabled, and only when loading inside the google frame, which is removed immediately by softpedia.

xul!nsRefPtr<nsDocShell>::assign_assuming_AddRef(class nsDocShell * newPtr = 0x00000040)+0x4 [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\obj-firefox\dist\include\nsautoptr.h @ 954]
xul!nsRefPtr<nsCSSStyleSheet>::assign_with_AddRef(class nsCSSStyleSheet * rawPtr = 0x0834de44)+0x19 [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\obj-firefox\dist\include\nsautoptr.h @ 941]
xul!nsHtml5Parser::DropStreamParser(void)+0xf [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\parser\html\nshtml5parser.h @ 311]
xul!nsHtml5TreeOpExecutor::DidBuildModel(int aTerminated = 0)+0x87 [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\parser\html\nshtml5treeopexecutor.cpp @ 135]
xul!nsHtml5TreeOpExecutor::Flush(void)+0x1b1 [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\parser\html\nshtml5treeopexecutor.cpp @ 369]
xul!nsHtml5ExecutorFlusher::Run(void)+0xc [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\parser\html\nshtml5streamparser.cpp @ 111]
xul!nsThread::ProcessNextEvent(int mayWait = <Memory access error>, int * result = <Memory access error>)+0x220 [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\xpcom\threads\nsthread.cpp @ 533]
xul!nsBaseAppShell::Run(void)+0x4a [e:\builds\moz2_slave\mozilla-central-win32-nightly\build\widget\src\xpwidgets\nsbaseappshell.cpp @ 169]

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Move the call to DropStreamParser(), add yet another mParser null check

nsIParser::Terminate() seems to be an inexhaustible supply on trouble. :-(

I moved the call to DropStreamParser immediately after one mParser null check.

Then I discovered that if I don't add yet another mParser null check later, onload gets unblocked too many times, so I added yet another null check. However, I didn't add it immediately below the DidBuildModelImpl() call to make sure the observer removals are never omitted. (I'm assuming observer removals are idempotent so it's safe to remove the same observer twice.)

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

The only reason I haven't r+'d this yet is that it's not obvious to me why this comment is true:

    // DidBuildModelImpl may cause mParser to be nulled out

Looking at http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#1635, I'm guessing that mParser might get nulled out as a result of the call to mScriptLoader->ParsingComplete(aTerminated);, since pending script requests are processed at the end of that method.

I don't doubt that this check is necessary, but if you have a simple explanation of how the nulling-out actually happens, I'd like to hear it.

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to comment #2)
> I don't doubt that this check is necessary, but if you have a simple
> explanation of how the nulling-out actually happens, I'd like to hear it.

I don't have an explanation. I observed the nulling out in debugger. Then I tried to step through code to see where the nulling out happens, but I gave up before I found out. I figured that since nulling out mParser happens if something calls nsIParser::Terminate(), I guessed that something that mScriptLoader->ParsingComplete(aTerminated) calls can call nsIParser::Terminate().

Comment 4

[Henri Sivonen (:hsivonen)]

I debugged this some more. Now I have an explanation.

When there's a pending defer script, it gets run so that re-entering as follows is possible:
#0	0x1304d8d7 in nsContentSink::DidBuildModelImpl at nsContentSink.cpp:1643
#1	0x1352b5eb in nsHtml5TreeOpExecutor::DidBuildModel at nsHtml5TreeOpExecutor.cpp:119
#2	0x1349b4f6 in nsHtml5Parser::Terminate at nsHtml5Parser.cpp:419
#3	0x1308c165 in nsDocument::StopDocumentLoad at nsDocument.cpp:2265
#4	0x132578cc in nsHTMLDocument::StopDocumentLoad at nsHTMLDocument.cpp:1003
#5	0x12d95d27 in DocumentViewerImpl::Stop at nsDocumentViewer.cpp:1643
#6	0x14872215 in nsDocShell::Stop at nsDocShell.cpp:3952
#7	0x1487235d in nsDocShell::Stop at nsDocShell.cpp:3980
#8	0x148867ea in nsDocShell::InternalLoad at nsDocShell.cpp:8017
#9	0x148814da in nsDocShell::LoadURI at nsDocShell.cpp:1358
#10	0x133bc5e3 in nsLocation::SetURI at nsLocation.cpp:316
#11	0x133bdb55 in nsLocation::SetHrefWithBase at nsLocation.cpp:590
#12	0x133bdd5b in nsLocation::SetHrefWithContext at nsLocation.cpp:537
#13	0x133be14a in nsLocation::SetHref at nsLocation.cpp:506
#14	0x003c4dd8 in NS_InvokeByIndex_P at xptcinvoke_unixish_x86.cpp:179
#15	0x120f48d3 in XPCWrappedNative::CallMethod at xpcwrappednative.cpp:2727
#16	0x121080c6 in XPCWrappedNative::SetAttribute at xpcprivate.h:2546
#17	0x12101548 in XPC_WN_GetterSetter at xpcwrappednativejsops.cpp:1792
#18	0x0010e028 in js_Invoke at jsinterp.cpp:1377
#19	0x0010e62f in js_InternalInvoke at jsinterp.cpp:1440
#20	0x0007b0c2 in JS_CallFunctionValue at jsapi.cpp:5099
#21	0x1211b66c in XPCWrapper::GetOrSetNativeProperty at XPCWrapper.cpp:722
#22	0x121160ca in XPC_XOW_GetOrSetProperty at XPCCrossOriginWrapper.cpp:674
#23	0x12116387 in XPC_XOW_SetProperty at XPCCrossOriginWrapper.cpp:739
#24	0x0012becd in JSScopeProperty::set at jsscope.h:864
#25	0x0011fc07 in js_NativeSet at jsobj.cpp:4390
#26	0x001220ec in js_SetPropertyHelper at jsobj.cpp:4797
#27	0x000f7add in js_Interpret at jsops.cpp:1897
#28	0x0010d29a in js_Execute at jsinterp.cpp:1619
#29	0x0007c6b6 in JS_EvaluateUCScriptForPrincipals at jsapi.cpp:5057
#30	0x133714c9 in nsJSContext::EvaluateString at nsJSEnvironment.cpp:1713
#31	0x131115ba in nsScriptLoader::EvaluateScript at nsScriptLoader.cpp:713
#32	0x131118b4 in nsScriptLoader::ProcessRequest at nsScriptLoader.cpp:626
#33	0x13111a96 in nsScriptLoader::ProcessPendingRequests at nsScriptLoader.cpp:786
#34	0x13111c35 in nsScriptLoader::ParsingComplete at nsScriptLoader.cpp:1098
#35	0x1304d95d in nsContentSink::DidBuildModelImpl at nsContentSink.cpp:1648
#36	0x1352b5eb in nsHtml5TreeOpExecutor::DidBuildModel at nsHtml5TreeOpExecutor.cpp:119
#37	0x135284e6 in nsHtml5TreeOperation::Perform at nsHtml5TreeOperation.cpp:572
#38	0x1352b379 in nsHtml5TreeOpExecutor::Flush at nsHtml5TreeOpExecutor.cpp:307
#39	0x13532c7e in nsHtml5ExecutorFlusher::Run at nsHtml5StreamParser.cpp:130
#40	0x003a61ec in nsThread::ProcessNextEvent at nsThread.cpp:527
#41	0x0032953b in NS_ProcessPendingEvents_P at nsThreadUtils.cpp:200
#42	0x12bd5917 in nsBaseAppShell::NativeEventCallback at nsBaseAppShell.cpp:121
#43	0x12b865ed in nsAppShell::ProcessGeckoEvents at nsAppShell.mm:498
#44	0x9494a8cb in __CFRunLoopDoSources0
#45	0x9494838f in __CFRunLoopRun
#46	0x94947864 in CFRunLoopRunSpecific
#47	0x94947691 in CFRunLoopRunInMode
#48	0x9105ff0c in RunCurrentEventLoopInMode
#49	0x9105fcc3 in ReceiveNextEventCommon
#50	0x9105fb48 in BlockUntilNextEventMatchingListInMode
#51	0x988daac5 in _DPSNextEvent
#52	0x988da306 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:]
#53	0x9889c49f in -[NSApplication run]
#54	0x12b84d1b in nsAppShell::Run at nsAppShell.mm:851
#55	0x14e6d3ca in nsAppStartup::Run at nsAppStartup.cpp:182
#56	0x000111e8 in XRE_main at nsAppRunner.cpp:3502
#57	0x00002629 in main at nsBrowserApp.cpp:158
#58	0x000022aa in start

The call that's on the top of this stack (the one from Terminate) will set mParser to null. Then call that #35 in this stack eventually hits the second null test. (Or would crash without the null test.)

(The script seen on stack from #31 is a defer script.)

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I'd guess also closing the currently loading window (i.e. remove a loading iframe from parent document) would lead to similar problem.

Comment 6

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 414699 [details] [diff] [review]
Move the call to DropStreamParser(), add yet another mParser null check

Thanks for investigating.  Much clearer now why this patch is necessary and sufficient.

Comment 7

[Henri Sivonen (:hsivonen)]

Thanks.
http://hg.mozilla.org/mozilla-central/rev/c39745ba3c9e