514447 - [HTML5] Crash [@ nsHtml5HtmlAttributes::contains] with document.write script changing location

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build with the html5 parser enabled.

http://crash-stats.mozilla.com/report/index/d2d04030-a262-4971-89f6-915932090903?p=1
0  	xul.dll  	nsHtml5HtmlAttributes::contains  	 parser/html/nsHtml5HtmlAttributes.cpp:205
1 	xul.dll 	nsHtml5Tokenizer::attributeNameComplete 	parser/html/nsHtml5Tokenizer.cpp:374
2 	xul.dll 	nsHtml5Tokenizer::stateLoop 	parser/html/nsHtml5Tokenizer.cpp:699
3 	xul.dll 	nsHtml5Tokenizer::tokenizeBuffer 	parser/html/nsHtml5Tokenizer.cpp:459
4 	xul.dll 	nsHtml5Parser::Parse 	parser/html/nsHtml5Parser.cpp:343
5 	xul.dll 	nsHTMLDocument::WriteCommon 	content/html/document/src/nsHTMLDocument.cpp:2175
6 	xul.dll 	nsHTMLDocument::Write 	content/html/document/src/nsHTMLDocument.cpp:2188
7 	xul.dll 	nsIDOMHTMLDocument_Write 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:9223
8 	js3250.dll 	js_Interpret 	js/src/jsops.cpp:2166
9 	js3250.dll 	js_Execute 	js/src/jsinterp.cpp:1601
10 	js3250.dll 	JS_EvaluateUCScriptForPrincipals 	js/src/jsapi.cpp:5089
11 	xul.dll 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1682
12 	xul.dll 	nsScriptLoader::EvaluateScript 	content/base/src/nsScriptLoader.cpp:686
13 	xul.dll 	nsScriptLoader::ProcessRequest 	content/base/src/nsScriptLoader.cpp:600
14 	xul.dll 	nsScriptLoader::ProcessScriptElement 	content/base/src/nsScriptLoader.cpp:554

Comment 1

[georgi - hopefully not receiving bugspam]

on trunk this looks like long/infite loop or recursion.

get many ++DOMWINDOW

probably the crash is due to stack exhaustion.

Comment 2

[georgi - hopefully not receiving bugspam]

no crash on branch too.

Comment 3

[Henri Sivonen (:hsivonen)]

Looks like this will get fixed when bug 503473 lands.

Comment 4

[Henri Sivonen (:hsivonen)]

I believe this was fixed together with bug 503473.

Comment 6

[chris hofmann]

The crash has started to show up on firefox 3.6b4

would we need all the changes in bug 503473 to fixes this on the 1.9.2 branch?

so far the crash is low volume so it might not be worth the risk of taking a larg number of, or a risk set of fixes on 1.9.2,  but it there is an easy fix we should stop the regression that firefox 3.6 users would see.

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #6)
> would we need all the changes in bug 503473 to fixes this on the 1.9.2 branch?

The HTML5 has been abandoned on the 1.9.2 branch since the time 1.9.2 branched. The parser was landed after 1.9.1 was done. It wasn't really planned that it got onto a release branch so soon.

There are a number of now known crashers that apply to the HTML5 parser as it exists on the branch. These bugs have been fixed in the HTML5 parser as it exists after the off-the-main-thread move in my local tree. Some of the fixes aren't even on the trunk, yet. The off-the-main-thread move shuffled code around significantly, so none of the patches apply to the code prior to the move without manual backporting.

> so far the crash is low volume so it might not be worth the risk of taking a
> larg number of, or a risk set of fixes on 1.9.2,  but it there is an easy fix
> we should stop the regression that firefox 3.6 users would see.

I think it doesn't make sense to fix one HTML5 parser crash on the 1.9.2 branch leaving all the others in. And it doesn't make sense to backport fixes for all known issues without taking the off-the-main-thread move, too. And taking it all to the branch is way too late now.

The html5.enable pref was never meant for user who run release builds. It was meant for testers who are willing to test code that wasn't even deemed worthy of turning on by default on the trunk. From that point of view, it doesn't make sense to have the pref in the Firefox 3.6 builds at all. 

I think the real fix available at this time would be hard-coding the pref to always-off on the branch. A slightly more risky fix would be removing the HTML5 parser from the 1.9.2 branch.

Comment 8

[timeless]

yeah, i'd support just removing the code that reads the pref and using the false path. removing the code is pointless, but treating the pref as false seems like a good decision for 192