386914 - Crash [@ nsXULDocument::ExecuteOnBroadcastHandlerFor] with DOMAttrModified event handler and observes

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes Mozilla within 100ms.
It also crashes branch, so marking security sensitive for now.

http://crash-stats.mozilla.com/report/index/e7598a70-2a8c-11dc-952e-001a4bd43e5c
0  	@0x2436041
1 	nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>(nsQueryInterface)
2 	nsXULDocument::ExecuteOnBroadcastHandlerFor(nsIContent *,nsIDOMElement *,nsIAtom *)
3 	nsXULDocument::AttributeChanged(nsIDocument *,nsIContent *,int,nsIAtom *,int)
4 	nsNodeUtils::AttributeChanged(nsIContent *,int,nsIAtom *,int)
5 	nsGenericElement::SetAttrAndNotify(int,nsIAtom *,nsIAtom *,nsAString_internal const &,nsAttrValue &,int,int,int)
6 	nsGenericElement::SetAttr(int,nsIAtom *,nsIAtom *,nsAString_internal const &,int)
7 	nsGenericElement::SetAttribute(nsAString_internal const &,nsAString_internal const &)
8 	NS_InvokeByIndex_P
9 	AutoJSSuspendRequest::SuspendRequest()

Branch talkback ID: TB33739202G

I think this might be related to bug 351347.

Comment 1

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

hmm, http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.771&mark=188#187

Comment 2

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

Though, in this case the crash is not because of that, but because 
BroadcastListener gets deleted, I think.
Patch coming.

Comment 3

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

Patch is not coming yet. I need to think broadcaster a bit more.
There are other possible crashers there, I think.

Comment 4

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

Attachment: proposed patch

For this type of crash keeping broadcast listeners alive is needed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

This is basically the same as bug 351347, right?  Down to the stack, and the analysis, at least... ;)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Note, in particular, the comments in that bug explaining why calling SetAttr here is bad to start with.

Comment 7

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

Yes, looks like the same bug. The suggestion in comment 6 in that bug
sounds pretty similar to what I did.

Comment 8

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

Attachment: for 1.8

Comment 9

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071905 Minefield/3.0a7pre

Comment 10

[Daniel Veditz [:dveditz]]

deleted aListener passed to nsXULDocument::ExecuteOnBroadcastHandlerFor(), crashes 1.8.0.12 and 1.8.1.6

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 272465 [details] [diff] [review]
for 1.8

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 13

[Carsten Book [:Tomcat]]

verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.7pre) Gecko/2007083003 BonEcho/2.0.0.7pre and the testcase in this bug.
No crash on testcase - adding verified keyword.

Comment 14

[Alexander Sack]

Comment on attachment 272465 [details] [diff] [review]
for 1.8

a=asac for 1.8.0.15

(same patch shipped by distros for some time)

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

fix committed to 1.8.0 branch

Comment 16

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/9a7346ed943a