Crash [@ nsXULElement::QueryInterface] with observes attribute doing some other stuff and reloading

RESOLVED WORKSFORME

Status

()

Core
XUL
P2
critical
RESOLVED WORKSFORME
11 years ago
3 years ago

People

(Reporter: Martijn Wargers (zombie), Assigned: mats)

Tracking

({crash, testcase})

Trunk
x86
All
crash, testcase
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
blocking1.8.1.13 -
wanted1.8.1.x +
blocking1.8.0.next +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [branch 1.8.1 only?] [sg:critical?] using freed content object[dbaron-1.9:Rs], crash signature)

Attachments

(11 attachments)

(Reporter)

Description

11 years ago
Created attachment 280342 [details]
testcase

See testcase, which usually crashes after a few reloads (reloads automatically).
It also crashes branch, so marking security sensitive for now.

http://crash-stats.mozilla.com/report/index/6ff903e3-5fa8-11dc-b7b3-001a4bd43e5c
0  	@0x27989d3  	
1 	nsXULElement::QueryInterface(nsID const&, void**) 	mozilla/content/xul/content/src/nsXULElement.cpp:376
2 	nsQueryInterface::operator()(nsID const&, void**) 	nsCOMPtr.cpp:47
3 	nsCOMPtr_base::assign_from_qi(nsQueryInterface, nsID const&) 	nsCOMPtr.cpp:96
4 	nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>(nsQueryInterface) 	nsCOMPtr.h:645
5 	nsXULDocument::AttributeChanged(nsIDocument*, nsIContent*, int, nsIAtom*, int, unsigned int) 	mozilla/content/xul/document/src/nsXULDocument.cpp:964
6 	nsNodeUtils::AttributeChanged(nsIContent*, int, nsIAtom*, int, unsigned int) 	mozilla/content/base/src/nsNodeUtils.cpp:110
7 	nsGenericElement::SetAttrAndNotify(int, nsIAtom*, nsIAtom*, nsAString_internal const&, nsAttrValue&, int, int, int) 	mozilla/content/base/src/nsGenericElement.cpp:3642
8 	nsGenericElement::SetAttr(int, nsIAtom*, nsIAtom*, nsAString_internal const&, int) 	mozilla/content/base/src/nsGenericElement.cpp:3573
9 	nsGenericElement::SetAttribute(nsAString_internal const&, nsAString_internal const&) 	mozilla/content/base/src/nsGenericElement.cpp:1500
10 	NS_InvokeByIndex_P 	mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
11 	AutoJSSuspendRequest::SuspendRequest() 	mozilla/js/src/xpconnect/src/xpcprivate.h:3317
12 	msvcr80.dll@0x10ae
OS: Windows XP → All
(Assignee)

Updated

11 years ago
Assignee: jag → mats.palmgren
(Assignee)

Comment 2

11 years ago
The problem is that when the ID of the broadcast element is changed its
broadcast listener list needs to be updated - otherwise, if a listener
is removed from the document it will fail to remove itself from the
broadcaster's list (since it can't find it anymore).
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.783&root=/cvsroot&mark=1713-1718#1673

Later, when the listener is destroyed, we will have a dangling pointer:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/content/xul/document/src/nsXULDocument.cpp&rev=1.783&root=/cvsroot&mark=184#176
(Assignee)

Comment 3

11 years ago
Created attachment 280553 [details] [diff] [review]
Patch rev. 1
Attachment #280553 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 4

11 years ago
Comment on attachment 280553 [details] [diff] [review]
Patch rev. 1

(patch also works for branches)
Attachment #280553 - Attachment description: Patch rev. 1 (trunk) → Patch rev. 1
Attachment #280553 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Updated

11 years ago
Attachment #280553 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

11 years ago
Flags: blocking1.8.1.7?
Flags: blocking1.8.0.14?
Whiteboard: [sg:critical?] using freed content object
Comment on attachment 280553 [details] [diff] [review]
Patch rev. 1

I think Neil is a better reviewer here than I am.
Attachment #280553 - Flags: superreview?(bzbarsky) → superreview?(neil)

Comment 6

11 years ago
Comment on attachment 280553 [details] [diff] [review]
Patch rev. 1

Well, this will fix observes="b" but it won't fix <observes element="b"> ...

On trunk we should maybe switch to nsWeakPtrs?

Comment 7

11 years ago
Comment on attachment 280553 [details] [diff] [review]
Patch rev. 1

As we can't generally find the element that caused the broadcaster hookup, I guess all you can do is to remove all listeners.
Attachment #280553 - Flags: superreview?(neil) → superreview-
(Assignee)

Comment 8

11 years ago
Created attachment 281165 [details]
Testcase #2
(Assignee)

Comment 9

11 years ago
Created attachment 281166 [details]
Testcase #3
(Assignee)

Comment 10

11 years ago
Created attachment 281167 [details]
Testcase #4
(Assignee)

Comment 11

11 years ago
Created attachment 281168 [details]
Testcase #5
(Assignee)

Comment 12

11 years ago
(In reply to comment #6)
> (From update of attachment 280553 [details] [diff] [review])
> Well, this will fix observes="b" but it won't fix <observes element="b"> ...

Hmm, right, so looking at the code there are a number of different bugs
1. the broadcaster changing its identity (which is what I fixed)
2. an <observes> changing the 'element'/'attribute' attributes
3. adding or removing <observes> elements (when more than one)
4. changing the 'observes'/'command' attribute (we handle removing it,
   but not changing it)

> On trunk we should maybe switch to nsWeakPtrs?

Maybe, but if we get this right we shouldn't need it.
I'd rather see a nsWeakPtrs bandaid on branch (if they're available).
I can do a separate patch for it if needed.
(Assignee)

Comment 13

11 years ago
Created attachment 281173 [details] [diff] [review]
Patch rev. 2

For listener attributes (observes,command,element,attribute):
  * remove matching listeners in nsXULElement::BeforeSetAttr
  * setup new listeners in nsXULElement::AfterSetAttr
  * for attr removal, remove listeners early in nsXULElement::UnsetAttr
    and possibly set up a new listener at the end (removing a 'observes'
    with non-empty 'commend' only)

For broadcaster attributes (id,ref):
  * update listener list in nsXULDocument::AttributeChanged (roughly as
    in patch rev. 1)

Add 'element' and 'attribute' to the non-broadcastable attrs since they
could affect broadcast listener lists.

Remove 29 unnecessary #includes in nsXULElement.cpp :-)
Attachment #281173 - Flags: superreview?(neil)
Attachment #281173 - Flags: review?(neil)
(Assignee)

Comment 14

11 years ago
Created attachment 281174 [details] [diff] [review]
Patch rev. 2 (diff -w)
(Assignee)

Comment 15

11 years ago
BTW, we could assert that |mBroadcasterMap| is empty in
nsXULDocument::~nsXULDocument(), yes?
So,,,  why are the testcases not in the patch?

Comment 17

11 years ago
Comment on attachment 281173 [details] [diff] [review]
Patch rev. 2

1. I can't r+sr here
2. I don't see why you need to update broadcasters if the element map fails to update, since they will only find the same element again
3. I don't understand the forParent stuff, please explain why checking for <observes> does not suffice?
Attachment #281173 - Flags: review?(neil)
Flags: blocking1.9?
Given the size of the patch (== added risk) and the other 1.8.1.8 blocking bugs assigned to mats this will probably have to wait for the next release.
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.9?
Flags: blocking1.8.1.8?
Flags: blocking1.9? → blocking1.9+
Whiteboard: [sg:critical?] using freed content object → [sg:critical?] using freed content object[dbaron-1.9:Rs]
Flags: blocking1.8.0.14? → blocking1.8.0.15?
P4 seems too low for an sg:critical bug -- needs to not slip out of the release.

Since progress seems to have stalled I don't know if 1.8.1.12 is a realistic brach release for this.
Flags: blocking1.8.1.13?
Priority: P4 → P2
(Reporter)

Updated

11 years ago
Blocks: 409480
Flags: blocking1.8.1.12?

Comment 20

11 years ago
maybe fx3 b3?
Priority: P2 → P1
(Reporter)

Comment 21

11 years ago
The testcases don't crash anymore with current trunk build, so I guess this is fixed by bug 414747.

Mats, is your patch still needed, or can this bug be closed?

Updated

11 years ago
Flags: in-testsuite?
Flags: blocking1.8.1.13? → blocking1.8.1.13+

Updated

11 years ago
Flags: blocking1.8.0.15? → blocking1.8.0.15+
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.8.1.14?
Flags: blocking1.8.1.13-
Flags: blocking1.8.1.13+

Updated

11 years ago
Flags: tracking1.9+
Mats: please answer Neil's questions in comment 17 (particularly 2 and 3)
Flags: blocking1.8.1.15?
(Reporter)

Comment 23

10 years ago
Created attachment 319009 [details]
testcase6

Ok, this one still crashes in current trunk build and I believe this is basically this bug.
(Reporter)

Comment 24

10 years ago
Oh, that one doesn't crash online. I guess it depends on what one has stored in the localstore.rdf file. I guess bug 431128 is basically related to this bug (that one has a testcase that still crashes in current trunk build).
Blocks: 431128
(Reporter)

Comment 25

10 years ago
Created attachment 324814 [details]
testcase7
(Reporter)

Comment 26

10 years ago
All testcases seem to be worksforme, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b1pre) Gecko/20080905031348 Minefield/3.1b1pre
(Reporter)

Updated

10 years ago
Flags: blocking1.9.1?
Mats, any chance you could respond to Neil's questions in comment 17?
Flags: blocking1.9.1? → wanted1.9.1+
None of the testcases crash for me either using:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081119 Minefield/3.1b2pre

Shall we resolve this WORKSFORME?
(Reporter)

Comment 29

10 years ago
I might be able to come up with a testcase that still crashes in current trunk build. 
See also question in comment 21.
Mats, there are several questions addressed to you which await answers.  This bug is in the "Top Security Bugs" queue so please treat this as a priority.
Mats/Neil, what's needed to move forward here?
Guys, any progress here?  This is a Top Security Bug and needs to be a priority.

Comment 33

10 years ago
Smaug, given that you reviewed it, can you explain the patch to me please?
I don't know any other answer for 3) than that it adds support for
<xul:observes> which don't want to observe their parent element.

And 2) isn't really valid anymore, since trunk doesn't have method called AddElementToMap()
(Assignee)

Comment 35

10 years ago
Comment on attachment 281173 [details] [diff] [review]
Patch rev. 2

This patch has bit-rotted and I think the problem was
fixed in other bugs.  None of the testcases crashes
trunk, 1.9.1 or 1.9.0 branches (and no assertions in
DEBUG builds).  It still crashes my 1.8.1 branch debug
build on Linux.

I will have a look at the new code to see if I can spot
some edge case that my patch did address.
And land the attached testcases as crashtests.
Attachment #281173 - Flags: superreview?(neil)
(Assignee)

Comment 36

10 years ago
Martijn, do you have any tests that still crash?
Whiteboard: [sg:critical?] using freed content object[dbaron-1.9:Rs] → [branch 1.8.1 only?] [sg:critical?] using freed content object[dbaron-1.9:Rs]

Comment 37

10 years ago
(In reply to comment #34)
> I don't know any other answer for 3) than that it adds support for
> <xul:observes> which don't want to observe their parent element.
Ah, I see now, <observes observes="observers"> could cause problems.

> And 2) isn't really valid anymore, since trunk doesn't have method called
> AddElementToMap()
Well, it's got AddElementToRefMap which is almost the same, but the point is that bug 344258 changed the code in much the same way that this patch wants.

sr=me JFTR
(Reporter)

Comment 38

10 years ago
(In reply to comment #36)
> Martijn, do you have any tests that still crash?

No, you're right, I can't seem to make a test anymore that crashes. Feel free to mark this worksforme, when needed.
Marking WORKSFORME based on above comments.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → WORKSFORME
Flags: wanted1.9.1+
Flags: wanted1.9.0.x+
Crash Signature: [@ nsXULElement::QueryInterface]

Updated

3 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.