The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

()

Core
XUL
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: smaug)

Tracking

(4 keywords)

Trunk
x86
Windows XP
crash, fixed1.8.0.15, testcase, verified1.8.1.8
Points:
---
Bug Flags:
blocking1.8.1.8 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?], crash signature)

Attachments

(3 attachments)

(Reporter)

Description

10 years ago
Created attachment 270976 [details]
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.
hmm, http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.771&mark=188#187
Though, in this case the crash is not because of that, but because 
BroadcastListener gets deleted, I think.
Patch coming.
Assignee: jag → Olli.Pettay
Patch is not coming yet. I need to think broadcaster a bit more.
There are other possible crashers there, I think.
Created attachment 271019 [details] [diff] [review]
proposed patch

For this type of crash keeping broadcast listeners alive is needed.
Attachment #271019 - Flags: superreview?(jst)
Attachment #271019 - Flags: review?(jst)
This is basically the same as bug 351347, right?  Down to the stack, and the analysis, at least... ;)
Note, in particular, the comments in that bug explaining why calling SetAttr here is bad to start with.
Yes, looks like the same bug. The suggestion in comment 6 in that bug
sounds pretty similar to what I did.

Updated

10 years ago
Attachment #271019 - Flags: superreview?(jst)
Attachment #271019 - Flags: superreview+
Attachment #271019 - Flags: review?(jst)
Attachment #271019 - Flags: review+
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Created attachment 272465 [details] [diff] [review]
for 1.8
Attachment #272465 - Flags: approval1.8.1.6?
(Reporter)

Comment 9

10 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007071905 Minefield/3.0a7pre
Status: RESOLVED → VERIFIED
deleted aListener passed to nsXULDocument::ExecuteOnBroadcastHandlerFor(), crashes 1.8.0.12 and 1.8.1.6
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Whiteboard: [sg:critical?]

Updated

10 years ago
Duplicate of this bug: 351347
Flags: blocking1.8.1.7+
Comment on attachment 272465 [details] [diff] [review]
for 1.8

approved for 1.8.1.7, a=dveditz for release-drivers
Attachment #272465 - Flags: approval1.8.1.7? → approval1.8.1.7+
Keywords: fixed1.8.1.7
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.
Keywords: fixed1.8.1.7 → verified1.8.1.7
Group: security

Comment 14

9 years ago
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)
Attachment #272465 - Flags: approval1.8.0.15+
fix committed to 1.8.0 branch
Keywords: fixed1.8.0.15

Comment 16

8 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/9a7346ed943a
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsXULDocument::ExecuteOnBroadcastHandlerFor]
You need to log in before you can comment on or make changes to this bug.