798275 - crash in nsParser::WillBuildModel

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, defect)

Description

[Scoobidiver (away)]

It slightly spiked from 18.0a1/20121004. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=635fcc11d2b1&tochange=4cb8f88213f5
It's likely a regression from bug 750019.

Signature 	nsParser::WillBuildModel(nsString&) More Reports Search
UUID	7e01cc23-3c0e-42e2-a64d-c0b872121005
Date Processed	2012-10-05 06:16:21
Uptime	4372
Last Crash	5.8 weeks before submission
Install Age	10.2 hours since version was first installed.
Install Time	2012-10-04 20:02:09
Product	Firefox
Version	18.0a1
Build ID	20121004030525
Release Channel	nightly
OS	Windows NT
OS Version	5.1.2600 Service Pack 3
Build Architecture	x86
Build Architecture Info	AuthenticAMD family 15 model 127 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION_WRITE
Crash Address	0x3682e90
App Notes 	
AdapterVendorID: 0x10de, AdapterDeviceID: 0x03d0, AdapterSubsysID: 73091462, AdapterDriverVersion: 6.14.11.9107
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True
Adapter Vendor ID	0x10de
Adapter Device ID	0x03d0
Total Virtual Memory	2147352576
Available Virtual Memory	1766838272
System Memory Use Percentage	69
Available Page File	1360216064
Available Physical Memory	242753536

Frame 	Module 	Signature 	Source
0 		@0x4f21e36 	
1 	xul.dll 	nsParser::WillBuildModel 	parser/htmlparser/src/nsParser.cpp:928
2 	xul.dll 	nsParser::ResumeParse 	parser/htmlparser/src/nsParser.cpp:1509
3 	xul.dll 	nsHTMLDocument::WriteCommon 	content/html/document/src/nsHTMLDocument.cpp:1739
4 	xul.dll 	nsHTMLDocument::Write 	content/html/document/src/nsHTMLDocument.cpp:1752
5 	mozjs.dll 	js::Wrapper::New 	js/src/jswrapper.cpp:56
6 	mozjs.dll 	JSCompartment::wrap 	js/src/jscompartment.cpp:309
7 	nspr4.dll 	_MD_CURRENT_THREAD 	nsprpub/pr/src/md/windows/w95thred.c:312
8 	mozjs.dll 	js::gc::ArenaLists::allocateFromArena 	js/src/jsgc.cpp:1653
9 	mozjs.dll 	js::gc::ArenaLists::refillFreeList 	js/src/jsgc.cpp:1859
10 	xul.dll 	XPC_WN_JSOp_ThisObject 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1275
11 	mozjs.dll 	js::CrossCompartmentWrapper::call 	js/src/jswrapper.cpp:722
12 	mozjs.dll 	proxy_Call 	js/src/jsproxy.cpp:3001
13 	mozjs.dll 	js::types::TypeSet::addType 	js/src/jsinferinlines.h:1381
14 	mozjs.dll 	js::Interpret 	js/src/jsinterp.cpp:2461
15 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:378
16 	mozjs.dll 	js::Invoke 	js/src/jsinterp.h:109
17 	mozjs.dll 	JSFunction::getBoundFunctionArgument 	js/src/jsfun.cpp:1034
18 	mozjs.dll 	js::CallOrConstructBoundFunction 	js/src/jsfun.cpp:1086
19 	mozjs.dll 	js::InvokeKernel 	js/src/jsinterp.cpp:367
20 	mozjs.dll 	js::Invoke 	js/src/jsinterp.cpp:411
21 	mozjs.dll 	JS_CallFunctionValue 	js/src/jsapi.cpp:5863
22 	xul.dll 	nsJSContext::CallEventHandler 	dom/base/nsJSEnvironment.cpp:1921
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsParser%3A%3AWillBuildModel%28nsString%26%29

Comment 1

[Henri Sivonen (:hsivonen)]

Very weird. Either something has managed to call document.write() on about:blank during the about:blank parsing or there is an XML parser parsing a document marked as HTML.

Comment 2

[David Mandelin [:dmandelin]]

(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Very weird. Either something has managed to call document.write() on
> about:blank during the about:blank parsing or there is an XML parser parsing
> a document marked as HTML.

Interesting. How did you know it was during about:blank?

Comment 3

[Henri Sivonen (:hsivonen)]

(In reply to David Mandelin [:dmandelin] from comment #2)
> (In reply to Henri Sivonen (:hsivonen) from comment #1)
> > Very weird. Either something has managed to call document.write() on
> > about:blank during the about:blank parsing or there is an XML parser parsing
> > a document marked as HTML.
> 
> Interesting. How did you know it was during about:blank?

about:blank is the only text/html URL that still uses nsParser.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Report the insertion point as undefined in nsParser

I’m pretty confident that this is the right fix, but it would be proper to write a test case, still.

Comment 5

[Henri Sivonen (:hsivonen)]

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

I tried to come up with a test case for this but failed. However, this is green on try and logic says this should help, so I suggest we land this and see if the crashes go away. nsParser::IsInsertionPointDefined() has been bogus ever since the option to run the old HTML parser went away.

Comment 6

[Marcia Knous [:marcia]]

6 	about:blank
1 	http://www.a10.com/racing-games/road-of-the-dead
1 	https://mightytext.net/app/
1 	http://apps.facebook.com/farmville-two/
1 	http://www.vn-zoom.com/f264/nero-platinum-12-0-02000-bo-cong-cu-multimedia-tat-c
1 	https://apps.facebook.com/onthefarm/reward.php?frHost=100000854884699&frId=7g48t
1 	https://apps.facebook.com/cafeworld/?fb_source=bookmark_apps&ref=bookmarks&count
1 	http://www1.youm7.com/News.asp?NewsID=721301&SecID=48&IssueID=35
1 	http://apps.facebook.com/bubblesafari/?fb_source=canvasbookmark&count=3
1 	http://www1.youm7.com/NewsSection.asp?SecID=245&IssueID=168
1 	http://www.gamesgames.com/myprofile.html
1 	http://www.sfr.fr/recherche/#sfrclicid=P_meta_rechext
1 	https://apps.facebook.com/playslingo/reward.php?frType=FallShare35&frId=3fe49716
1 	http://adf.ly/go/e9344083b7401bde4c952677d0c37a40/aHR0cDovL3RvcGRlemZpbG1lcy5vcm
1 	http://www.real-estate.lviv.ua/ru/arenda-commercialproperty/Lvov/p_4
1 	http://apps.facebook.com/yoville/
1 	https://apps.facebook.com/onthefarm/ugc_deco_material_reward.php

Comment 7

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

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

Could you perhaps explain this a bit to ease reviewing.

Comment 8

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

Ah, IsInsertionPointDefined is something which was added for HTML5 parser.
But based on Bug 503473 the patch would be wrong, since the old parser shouldn't
be terminated.
Did the removal or mWriteState cause this? or Removal of !mPendingScripts.Contains(key)?

Comment 9

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

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

r-, but if you can explain why the patch would be right, feel free to
re-ask review.

Comment 10

[Henri Sivonen (:hsivonen)]

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

This patch is correct because:

mParser can be null, an instance of nsHtml5Parser or an instance of nsParser.

Proceeding to calling Parse() on mParser can only be correct if mParser is an instance of nsHtml5Parser. Obviously, the method must not be called on null. Also, nsParser no longer supports that method. Furthermore, the call is made with a static_cast to nsHtml5Parser, so it's very important not to make that call unless mParser really is an instance of nsHtml5Parser.

If the insertion point is not defined, WriteCommon will not proceed to calling Parse on the current mParser. http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#1651 Instead, it will either return or terminate and right after implicitly call Open() (which creates a new mParser that's guaranteed to be an nsHtml5Parser). Thus, indicating that the insertion point is not defined is a sure way to prevent a call to Parse() on the current mParser.

In the XML case, WriteCommon should have returned earlier at http://mxr.mozilla.org/mozilla-central/source/content/html/document/src/nsHTMLDocument.cpp#1635 . However, semantically it is correct to claim that the insertion point is not defined in the XML case, because there is no way for the insertion point to be defined in the XML case, since the whole concept of the insertion point being defined as an HTML concept.

In the about:blank case, scripts should not have the opportunity to call into WriteCommon before mParser gets nulled away. However, since this crash exists, there must be some way for scripts to make that call and I just felt to figure out what that case is. Still, since that call cannot happen from within a script in the document being parsed (since about:blank by definition has no script tags in its zero-length stream), the insertion point has to be not defined during such a call into WriteCommon, since the insertion point could only be defined while an HTML parser is executing a script.

It is, therefore, always semantically correct to claim that the insertion point is not defined when mParser is an nsParser.

Furthermore, after the capability of handling document.write() was removed from nsParser, claiming that the insertion point is defined cannot be right. The reason why nsParser claims that the insertion point is always defined is that our old HTML parser, while it was still possible to enable it, did not have the concept of an insertion point, so always returning true was equivalent with the legacy behavior.

Comment 11

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

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

thanks for the explanation!

Comment 12

[Henri Sivonen (:hsivonen)]

Thanks. Landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eeaa1babcb5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2eeaa1babcb5

Comment 15

[Henri Sivonen (:hsivonen)]

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 731162

User impact if declined: 

Without this patch, it is possible that nsHTMLDocument makes a bogus static cast and then calls a method that doesn’t match the actual class of the pointer the method is called on.

Testing completed (on m-c, etc.):

Has baked on m-c for quite a while.
 
Risk to taking this patch (and alternatives if risky): 

Very low risk.

String or UUID changes made by this patch: 

None.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

Given the low volume of this crash we won't take the fix for uplift to Beta, but please go ahead with uplift to Aurora (18).

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/56c208e5b325

Comment 18

[Henri Sivonen (:hsivonen)]

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:

This is a crasher with a super-simple fix.

User impact if declined: 

Without this patch, it is possible that nsHTMLDocument makes a bogus static cast and then calls a method that doesn’t match the actual class of the pointer the method is called on.

Fix Landed on Version:

18 (originally landed for 19, got uplifted, uplift to non-ESR 17 got denied).

Risk to taking this patch (and alternatives if risky): 

Very low risk.

String or UUID changes made by this patch: 

None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 19

[Ioana (away)]

There are no crash reports for this crash on Firefox 18 and 19 post-fix:
https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsParser%3A%3AWillBuildModel%28nsString%26%29

Comment 20

[Alex Keybl [:akeybl]]

Comment on attachment 669053 [details] [diff] [review]
Report the insertion point as undefined in nsParser

This isn't severe enough to need uplift to the ESR branch.

Comment 29

[Justin Dolske [:Dolske]]

(Disabling comments since this bug seems to be attracting spam.)