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

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

Attachment: 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][[e-mail]]]

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][[e-mail]]]

I thought I assigned this to myself already :/

Comment 3

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

Attachment: NS_ASSERTION to NS_ENSURE_STATE

This is enough to fix the crash.

Comment 4

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

Attachment: 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) No longer reading bugmail consistently]

Comment on attachment 347619 [details] [diff] [review]
but this is what I prefer

Yeah, i like this one better too

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

ok for branch

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

a191=beltzner

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347619 [details] [diff] [review]
but this is what I prefer

a191=beltzner

Comment 9

[Johnny Stenback (:jst)]

Blocking, but this has approval etc so it's ready to go either way.

Comment 10

[Martijn Wargers (dead)]

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][[e-mail]]]

Fixed on trunk and 1.9.1

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

Approved for 1.9.0.6	, a=dveditz for release-drivers.

Comment 13

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

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 [:dveditz]]

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]]

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