Closed Bug 464009 Opened 14 years ago Closed 14 years ago

Crash [@ nsGenericHTMLFormElement::UnbindFromTree] with DOMAttrModified removing file input in form

Categories

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

x86
Windows XP
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: martijn.martijn, Assigned: smaug)

Details

(4 keywords, Whiteboard: [sg:low?] probably null-deref, worrying assertion)

Crash Data

Attachments

(3 files)

Attached file 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
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 :( )
Component: DOM: Events → DOM: Core & HTML
QA Contact: events → general
I thought I assigned this to myself already :/
Assignee: nobody → Olli.Pettay
This is enough to fix the crash.
Attachment #347618 - Flags: superreview?(jonas)
Attachment #347618 - Flags: review?(jonas)
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.
Attachment #347619 - Flags: superreview?(jonas)
Attachment #347619 - Flags: review?(jonas)
Flags: blocking1.9.1?
Attachment #347619 - Flags: superreview?(jonas)
Attachment #347619 - Flags: superreview+
Attachment #347619 - Flags: review?(jonas)
Attachment #347619 - Flags: review+
Comment on attachment 347619 [details] [diff] [review]
but this is what I prefer

Yeah, i like this one better too
Comment on attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

ok for branch
Attachment #347618 - Flags: superreview?(jonas)
Attachment #347618 - Flags: superreview+
Attachment #347618 - Flags: review?(jonas)
Attachment #347618 - Flags: review+
Attachment #347618 - Flags: approval1.9.1?
Attachment #347619 - Flags: approval1.9.1?
Comment on attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

a191=beltzner
Attachment #347618 - Flags: approval1.9.1? → approval1.9.1+
Comment on attachment 347619 [details] [diff] [review]
but this is what I prefer

a191=beltzner
Attachment #347619 - Flags: approval1.9.1? → approval1.9.1+
Blocking, but this has approval etc so it's ready to go either way.
Flags: blocking1.9.1? → blocking1.9.1+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [needs-1.9.1-landing]
Attachment #347618 - Flags: approval1.9.0.6?
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Fixed on trunk and 1.9.1
Whiteboard: [needs-1.9.1-landing]
Keywords: fixed1.9.1
Attachment #347618 - Flags: approval1.9.0.6? → approval1.9.0.6+
Comment on attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

Approved for 1.9.0.6	, a=dveditz for release-drivers.
Keywords: fixed1.9.0.6
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).
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.
Whiteboard: [sg:low?] probably null-deref, worrying assertion
Group: core-security
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
Crash Signature: [@ nsGenericHTMLFormElement::UnbindFromTree]
You need to log in before you can comment on or make changes to this bug.