589635 - Crash [@ nsSubDocumentFrame::AttributeChanged] with throwing error and removing type attribute on iframe

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes current trunk build and Firefox3.6.8 after a while (normally within 1 minute or so).

The content of the iframe is this:
<?xml-stylesheet href="chrome://browser/skin/" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul" xmlns:html="http://www.w3.org/1999/xhtml">
<iframe type="a"></iframe>

<script><![CDATA[
setTimeout(function() {
parent.document.getElementById('content2').contentDocument.documentElement.appendChild(document.documentElement);
parent.document.getElementById('content2').contentWindow.location.reload();
}, 100);

setInterval(function() {
for (var i=0;i < 6; i++) {throw('t');}
},30);

var all = document.getElementsByTagName('*');
var iframe = all[1];

function doe(node) {
node.removeAttributeNode(node.attributes[0]);
}

setTimeout(function() {
doe(iframe);
},150);

]]></script>
</window>

http://crash-stats.mozilla.com/report/index/bp-39e25f91-184e-4504-a829-4ae472100822
0  	xul.dll  	nsSubDocumentFrame::AttributeChanged  	 layout/generic/nsFrameFrame.cpp:788
1 	xul.dll 	nsStyleContext::DoGetStyleDisplay 	layout/style/nsStyleStructList.h:95
2 	mozcrt19.dll 	arena_dalloc_small 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:4153
3 	mozcrt19.dll 	arena_dalloc 	obj-firefox/memory/jemalloc/crtsrc/jemalloc.c:4284
4 	xul.dll 	nsDOMAttribute::Release 	content/base/src/nsDOMAttribute.cpp:151
5 	xul.dll 	nsTHashtable<nsBaseHashtableET<nsAttrHashKey,nsRefPtr<nsDOMAttribute> > >::s_ClearEntry 	obj-firefox/dist/include/nsTHashtable.h:397
6 	xul.dll 	nsNodeUtils::AttributeChanged 	content/base/src/nsNodeUtils.cpp:128
7 	xul.dll 	nsXULElement::UnsetAttr 	content/xul/content/src/nsXULElement.cpp:1455
8 	xul.dll 	nsDOMAttributeMap::RemoveNamedItem 	content/base/src/nsDOMAttributeMap.cpp:390
9 	xul.dll 	nsGenericElement::RemoveAttributeNode 	content/base/src/nsGenericElement.cpp:2478
10 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
11 	xul.dll 	js::InvokeCommon<int > 	js/src/jsinterp.cpp:561
12 		@0x11d215b2 	
13 	xul.dll 	nsXPConnect::GetWrapperForObject 	js/src/xpconnect/src/nsXPConnect.cpp:2481
14 	xul.dll 	js_GetPropertyHelper 	js/src/jsobj.cpp:4830
15 	xul.dll 	XPC_WN_JSOp_ThisObject 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1533

Comment 1

[Jesse Ruderman]

Stack trace looks corrupt, so I'm calling this [sg:critical].

I'm having trouble reproducing on Mac OS X 10.5.x with Firefox trunk.

Comment 2

[Jesse Ruderman]

Couldn't reproduce using Valgrind (on Mac OS X 10.6.x) either :(

Comment 3

[Marcia Knous [:marcia]]

Marcia will try to reproduce this in the QA lab following the meeting.

Comment 4

[Marcia Knous [:marcia]]

I crashed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 (.NET CLR 3.5.30729) - http://crash-stats.mozilla.com/report/index/bp-8aa81493-005c-4667-8961-bb3992100921

My first attempt at loading the testcase on the trunk, it did not crash. Will try again.

Comment 5

[Marcia Knous [:marcia]]

Not able to crash with testcase using  Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100921 Firefox/4.0b7pre.

Comment 6

[Marcia Knous [:marcia]]

Martijn: Can you try your STR again please? I am unable to get a trunk crash.

Comment 7

[Daniel Veditz [:dveditz]]

Did this get fixed along the way with another patch?

Comment 8

[Martijn Wargers (dead)]

I'm still crashing in current trunk build.

Comment 9

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

Couldn't reproduce on linux

Comment 10

[Martijn Wargers (dead)]

It doesn't crash on trunk, because xul is disabled by default. You'll have to enable xul for this site (but that's kinda difficult right now, because xul is disabled without the option of turning it back on in about:config or the ui).

Comment 11

[Damon Sicore (:damons)]

This probably isn't critical on trunk, but nominating for 1.9.2.

Comment 12

[LegNeato]

Does this also affect 1.9.1?

Comment 13

[Daniel Veditz [:dveditz]]

On 3.6.11pre I crash at the same location referencing the same 0x0 address as the stacks in comment 0 and comment 4, but my stack didn't look messed up like the other two: bp-04b15f88-f811-414f-b660-0afc72101008

Comment 14

[Daniel Veditz [:dveditz]]

Shiretoko has the same crash: bp-f745d95f-1f53-42f6-b356-5f2032101008

Comment 15

[Damon Sicore (:damons)]

Mats, are you actually working on this?  Since we can reproduce this on branch, we should just fix this.

Comment 16

[Mats Palmgren (inactive)]

I'm busy on bug 571995.  Optimistically, I could get to this in a couple
of days.  When is the next branch release(s) due?

Comment 17

[Mats Palmgren (inactive)]

There were a couple of regressions from bug 571995 (bug 604843, bug 605340)
which now has patches for review, so I'll look at this bug after that.

Comment 18

[Daniel Veditz [:dveditz]]

Turning this into a branch bug since we've disabled content XUL on trunk (at least, for non-whitelisted sites).

Comment 19

[Johnny Stenback (:jst)]

Mats, got an eta here?

Comment 20

[Johnny Stenback (:jst)]

Mats, ping?

Comment 21

[Mats Palmgren (inactive)]

Sorry for the delay.

I've tested this in debug builds on WinXP, OSX and Linux64 and AFAICT
it's a safe null pointer access.  Jesse reported in comment 1 that
he saw evidence of a damaged stack so I guess there could have been
another bug involved earlier that is now gone.  I'm lowering this to
sg:dos until we have new evidence indicating otherwise.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Fix for 1.9.2 and 1.9.1

BTW, there's a comment that says the code is more or less the same as
in nsFrameLoader::EnsureDocShell.  I've checked that method and it
already has a corresponding null check.
http://mxr.mozilla.org/mozilla1.9.2/source/layout/generic/nsFrameFrame.cpp#703

I've stress tested this patch on WinXP, OSX and Linux64 debug 1.9.2 builds
and a Linux64 1.9.1 debug build.

Making a testcase that actually halts seems tricky -- I'll try make one
in a separate patch.

Comment 23

[Mats Palmgren (inactive)]

Attachment: Fix for trunk

Comment 24

[Mats Palmgren (inactive)]

Attachment: crashtest.diff

Comment 25

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/06e68e399226

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 495726 [details] [diff] [review]
Fix for 1.9.2 and 1.9.1

Approved for 1.9.2.14 and 1.9.1.17, a=dveditz for release-drivers

Comment 27

[Mats Palmgren (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/51edb84f5830
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/168304e2c396

Comment 28

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.2 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.14pre) Gecko/20110104 Namoroka/3.6.14pre ( .NET CLR 3.5.30729) and the testcase. With 1.9.2.13, the testcase will crash after a minute or three. It no longer does so with the 1.9.2.14pre build.

On 1.9.1, I cannot get a crash with the testcase in 1.9.1.16. Have we seen the crash there?

Comment 29

[Al Billings [:abillings - ex-MoCo]]

I was wrong. I was able to get the crash, it just took longer. I've run it in the nightly 1.9.1 build (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.17pre) Gecko/20110104 Shiretoko/3.5.17pre ( .NET CLR 3.5.30729)) for a while now and it hasn't crashed so I'm calling this verified for 1.9.1.