Closed Bug 589635 Opened 9 years ago Closed 9 years ago

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

Categories

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

1.9.2 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla2.0b8
Tracking Status
blocking1.9.2 --- needed
status1.9.2 --- .14-fixed
blocking1.9.1 --- needed
status1.9.1 --- .17-fixed

People

(Reporter: martijn.martijn, Assigned: mats)

Details

(4 keywords, Whiteboard: [sg:dos null pointer access] "fixed" on trunk by disabling content XUL)

Crash Data

Attachments

(4 files)

Attached file 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
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.
OS: Windows 7 → Windows XP
Whiteboard: [sg:critical]
Couldn't reproduce using Valgrind (on Mac OS X 10.6.x) either :(
Marcia will try to reproduce this in the QA lab following the meeting.
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.
Not able to crash with testcase using  Mozilla/5.0 (Windows NT 5.1; rv:2.0b7pre) Gecko/20100921 Firefox/4.0b7pre.
Martijn: Can you try your STR again please? I am unable to get a trunk crash.
Did this get fixed along the way with another patch?
I'm still crashing in current trunk build.
Couldn't reproduce on linux
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).
This probably isn't critical on trunk, but nominating for 1.9.2.
blocking1.9.2: --- → ?
Does this also affect 1.9.1?
blocking1.9.1: --- → ?
blocking1.9.2: ? → needed
status1.9.1: --- → ?
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
Shiretoko has the same crash: bp-f745d95f-1f53-42f6-b356-5f2032101008
blocking1.9.1: ? → needed
Mats, are you actually working on this?  Since we can reproduce this on branch, we should just fix this.
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?
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.
Turning this into a branch bug since we've disabled content XUL on trunk (at least, for non-whitelisted sites).
Whiteboard: [sg:critical] → [sg:critical] "fixed" on trunk by disabling content XUL
Version: Trunk → 1.9.2 Branch
Mats, got an eta here?
Mats, ping?
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.
OS: Windows XP → All
Hardware: x86 → All
Whiteboard: [sg:critical] "fixed" on trunk by disabling content XUL → [sg:dos null pointer access] "fixed" on trunk by disabling content XUL
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.
Attachment #495726 - Flags: review?(roc)
Attached patch Fix for trunkSplinter Review
Attached patch crashtest.diffSplinter Review
Attachment #495727 - Flags: review?(roc)
Attachment #495727 - Flags: approval2.0?
Attachment #495726 - Flags: approval1.9.2.14?
Attachment #495726 - Flags: approval1.9.1.17?
http://hg.mozilla.org/mozilla-central/rev/06e68e399226
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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
Attachment #495726 - Flags: approval1.9.2.14?
Attachment #495726 - Flags: approval1.9.2.14+
Attachment #495726 - Flags: approval1.9.1.17?
Attachment #495726 - Flags: approval1.9.1.17+
Group: core-security
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?
Keywords: verified1.9.2
Whiteboard: [sg:dos null pointer access] "fixed" on trunk by disabling content XUL → [sg:dos null pointer access] "fixed" on trunk by disabling content XUL [qa-examined-191] [qa-needs-STR]
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.
Keywords: verified1.9.1
Whiteboard: [sg:dos null pointer access] "fixed" on trunk by disabling content XUL [qa-examined-191] [qa-needs-STR] → [sg:dos null pointer access] "fixed" on trunk by disabling content XUL
Crash Signature: [@ nsSubDocumentFrame::AttributeChanged]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.