Last Comment Bug 589635 - Crash [@ nsSubDocumentFrame::AttributeChanged] with throwing error and removing type attribute on iframe
: Crash [@ nsSubDocumentFrame::AttributeChanged] with throwing error and removi...
Status: RESOLVED FIXED
[sg:dos null pointer access] "fixed" ...
: crash, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 1.9.2 Branch
: All All
: -- critical (vote)
: mozilla2.0b8
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-08-22 17:15 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2011-06-13 10:01 PDT (History)
9 users (show)
mats: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
needed
.14-fixed
needed
.17-fixed


Attachments
testcase (1.40 KB, text/html)
2010-08-22 17:15 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
Fix for 1.9.2 and 1.9.1 (733 bytes, patch)
2010-12-06 17:59 PST, Mats Palmgren (:mats)
roc: review+
dveditz: approval1.9.2.14+
dveditz: approval1.9.1.17+
Details | Diff | Review
Fix for trunk (757 bytes, patch)
2010-12-06 18:00 PST, Mats Palmgren (:mats)
roc: review+
roc: approval2.0+
Details | Diff | Review
crashtest.diff (2.52 KB, patch)
2010-12-06 18:27 PST, Mats Palmgren (:mats)
no flags Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-08-22 17:15:58 PDT
Created attachment 468196 [details]
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 2010-08-31 12:39:10 PDT
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 2010-08-31 15:25:02 PDT
Couldn't reproduce using Valgrind (on Mac OS X 10.6.x) either :(
Comment 3 Marcia Knous [:marcia - use ni] 2010-09-21 13:24:25 PDT
Marcia will try to reproduce this in the QA lab following the meeting.
Comment 4 Marcia Knous [:marcia - use ni] 2010-09-21 15:36:42 PDT
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 - use ni] 2010-09-21 15:39:05 PDT
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 - use ni] 2010-09-28 13:36:55 PDT
Martijn: Can you try your STR again please? I am unable to get a trunk crash.
Comment 7 Daniel Veditz [:dveditz] 2010-09-28 13:37:02 PDT
Did this get fixed along the way with another patch?
Comment 8 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-09-28 13:41:02 PDT
I'm still crashing in current trunk build.
Comment 9 Olli Pettay [:smaug] 2010-09-29 06:26:17 PDT
Couldn't reproduce on linux
Comment 10 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-09-29 10:08:10 PDT
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) 2010-10-05 13:42:38 PDT
This probably isn't critical on trunk, but nominating for 1.9.2.
Comment 12 christian 2010-10-06 10:14:57 PDT
Does this also affect 1.9.1?
Comment 13 Daniel Veditz [:dveditz] 2010-10-08 11:10:45 PDT
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] 2010-10-08 12:17:13 PDT
Shiretoko has the same crash: bp-f745d95f-1f53-42f6-b356-5f2032101008
Comment 15 Damon Sicore (:damons) 2010-10-12 13:49:25 PDT
Mats, are you actually working on this?  Since we can reproduce this on branch, we should just fix this.
Comment 16 Mats Palmgren (:mats) 2010-10-12 14:00:18 PDT
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 (:mats) 2010-10-19 12:06:13 PDT
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] 2010-11-09 14:10:04 PST
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, jst@mozilla.com) 2010-11-23 13:45:53 PST
Mats, got an eta here?
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2010-11-30 13:37:48 PST
Mats, ping?
Comment 21 Mats Palmgren (:mats) 2010-12-06 17:50:33 PST
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 (:mats) 2010-12-06 17:59:22 PST
Created attachment 495726 [details] [diff] [review]
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 (:mats) 2010-12-06 18:00:15 PST
Created attachment 495727 [details] [diff] [review]
Fix for trunk
Comment 24 Mats Palmgren (:mats) 2010-12-06 18:27:09 PST
Created attachment 495736 [details] [diff] [review]
crashtest.diff
Comment 25 Mats Palmgren (:mats) 2010-12-07 18:27:57 PST
http://hg.mozilla.org/mozilla-central/rev/06e68e399226
Comment 26 Daniel Veditz [:dveditz] 2010-12-08 10:33:50 PST
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 28 Al Billings [:abillings] 2011-01-04 17:10:33 PST
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] 2011-01-04 17:36:12 PST
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.

Note You need to log in before you can comment on or make changes to this bug.