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

VERIFIED FIXED

Status

()

--
critical
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: smaug)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, testcase, verified1.9.0.6, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
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
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
Created attachment 347618 [details] [diff] [review]
NS_ASSERTION to NS_ENSURE_STATE

This is enough to fix the crash.
Attachment #347618 - Flags: superreview?(jonas)
Attachment #347618 - Flags: review?(jonas)
Created attachment 347619 [details] [diff] [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.
Attachment #347619 - Flags: superreview?(jonas)
Attachment #347619 - Flags: review?(jonas)
(Reporter)

Updated

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs-1.9.1-landing]
Attachment #347618 - Flags: approval1.9.0.6?
(Reporter)

Comment 10

10 years ago
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).
Keywords: fixed1.9.0.6 → verified1.9.0.6
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
Keywords: fixed1.9.1 → verified1.9.1
Crash Signature: [@ nsGenericHTMLFormElement::UnbindFromTree]
You need to log in before you can comment on or make changes to this bug.