Last Comment Bug 464009 - Crash [@ nsGenericHTMLFormElement::UnbindFromTree] with DOMAttrModified removing file input in form
: Crash [@ nsGenericHTMLFormElement::UnbindFromTree] with DOMAttrModified remov...
Status: VERIFIED FIXED
: [sg:low?] probably null-deref, worryi...
: crash, testcase, verified1.9.0.6, verified1.9.1
Product: Core
Classification: Components
Component: DOM: Core & HTML
: Trunk
: x86 Windows XP
: -- critical (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: general
:
:
:
  Show dependency treegraph
 
Reported: 2008-11-10 05:02 PST by Martijn Wargers [:mw22] (QA - IRC nick: mw22)
Modified: 2009-03-11 10:11 PDT (History)
8 users (show)
jst: blocking1.9.1+
See Also:
Crash Signature:
[@ nsGenericHTMLFormElement::UnbindFromTree]


Attachments
testcase (385 bytes, text/html)
2008-11-10 05:02 PST, Martijn Wargers [:mw22] (QA - IRC nick: mw22)
no flags Details
NS_ASSERTION to NS_ENSURE_STATE (1023 bytes, patch)
2008-11-11 13:42 PST, Olli Pettay [:smaug]
jonas: review+
jonas: superreview+
mbeltzner: approval1.9.1+
dveditz: approval1.9.0.6+
Details | Diff | Splinter Review
but this is what I prefer (8.78 KB, patch)
2008-11-11 13:44 PST, Olli Pettay [:smaug]
jonas: review+
jonas: superreview+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review

Summon comment box

Description Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-11-10 05:02:21 PST
Created attachment 347272 [details]
testcase

See testcase, which crashes current trunk build within 100ms. It also crashes Firefox 3, so marking security sensitive for now.

http://crash-stats.mozilla.com/report/index/eb42a7b8-af24-11dd-8d45-001a4bd43ed6?p=1
0  	xul.dll  	xul.dll@0x2e8342  	
1 	xul.dll 	nsGenericHTMLFormElement::UnbindFromTree 	content/html/content/src/nsGenericHTMLElement.cpp:2513
2 	xul.dll 	nsHTMLInputElement::UnbindFromTree 	content/html/content/src/nsHTMLInputElement.cpp:1917
3 	xul.dll 	nsGenericElement::doRemoveChildAt 	content/base/src/nsGenericElement.cpp:3366
4 	xul.dll 	nsGenericElement::RemoveChildAt 	content/base/src/nsGenericElement.cpp:3293
5 	xul.dll 	nsGenericElement::doRemoveChild 	content/base/src/nsGenericElement.cpp:3955
6 	xul.dll 	nsGenericElement::RemoveChild 	content/base/src/nsGenericElement.cpp:3521
7 	xul.dll 	nsIDOMNode_RemoveChild 	obj-firefox/js/src/xpconnect/src/dom_quickstubs.cpp:2757
8 	js3250.dll 	js_Interpret 	js/src/jsinterp.cpp:4998
Comment 1 Olli Pettay [:smaug] 2008-11-10 05:50:22 PST
I get also the following assertions
###!!! ASSERTION: Child not in controls: 'index != controls.NoIndex', file /Users/smaug/mozilla/hg/mozilla/content/html/content/src/nsHTMLFormElement.cpp, line 1478
###!!! ASSERTION: Invalid start index: 'count == 0 || start < Length()', file ../../../../dist/include/xpcom/nsTArray.h, line 597
###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../../dist/include/xpcom/nsCOMPtr.h, line 521

(But have to wait a bit before I can really debug this. 64bit Linux builds don't really work atm :( )
Comment 2 Olli Pettay [:smaug] 2008-11-11 11:42:20 PST
I thought I assigned this to myself already :/
Comment 3 Olli Pettay [:smaug] 2008-11-11 13:42:09 PST
Created attachment 347618 [details] [review]
NS_ASSERTION to NS_ENSURE_STATE

This is enough to fix the crash.
Comment 4 Olli Pettay [:smaug] 2008-11-11 13:44:22 PST
Created attachment 347619 [details] [review]
but this is what I prefer

Makes mutation events to fire after AfterSetAttr, fixing the crash. This way 
things stay more consistent while changing attributes.
The first patch might be enough for 1.9.0 but I'd like to take these
both for 1.9.1.
Comment 5 Jonas Sicking (:sicking) 2008-11-18 10:20:30 PST
Comment on attachment 347619 [details] [review]
but this is what I prefer

Yeah, i like this one better too
Comment 6 Jonas Sicking (:sicking) 2008-11-18 10:20:59 PST
Comment on attachment 347618 [details] [review]
NS_ASSERTION to NS_ENSURE_STATE

ok for branch
Comment 7 Mike Beltzner [:beltzner] 2008-11-26 22:00:11 PST
Comment on attachment 347618 [details] [review]
NS_ASSERTION to NS_ENSURE_STATE

a191=beltzner
Comment 8 Mike Beltzner [:beltzner] 2008-11-26 22:00:31 PST
Comment on attachment 347619 [details] [review]
but this is what I prefer

a191=beltzner
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2008-12-02 15:14:12 PST
Blocking, but this has approval etc so it's ready to go either way.
Comment 10 Martijn Wargers [:mw22] (QA - IRC nick: mw22) 2008-12-04 09:45:00 PST
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre
Comment 11 Olli Pettay [:smaug] 2008-12-06 09:26:15 PST
Fixed on trunk and 1.9.1
Comment 12 Daniel Veditz 2008-12-08 12:06:56 PST
Comment on attachment 347618 [details] [review]
NS_ASSERTION to NS_ENSURE_STATE

Approved for 1.9.0.6	, a=dveditz for release-drivers.
Comment 13 Al Billings [:abillings] 2009-01-13 18:05:14 PST
Verified fix for 1.9.0.6 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.6pre) Gecko/2009011304 GranParadiso/3.0.6pre. It no longe crashes (but still crashes 3.0.5).
Comment 14 Daniel Veditz 2009-02-03 10:53:05 PST
This was crashing with a consistent null deref, but the nsTArray assertion was a little worrying so I can't say it was safe, either.
Comment 15 Tony Chung [:tchung] 2009-03-11 10:11:44 PDT
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3

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