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

Status: RESOLVED WORKSFORME

(Core :: XUL, defect, P2)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Mats Palmgren (inactive)]

Attachment: trace

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Comment 4

[Mats Palmgren (inactive)]

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

(patch also works for branches)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

I think Neil is a better reviewer here than I am.

Comment 6

[[e-mail]]

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

[[e-mail]]

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.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 9

[Mats Palmgren (inactive)]

Attachment: Testcase #3

Comment 10

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #5

Comment 12

[Mats Palmgren (inactive)]

(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.

Comment 13

[Mats Palmgren (inactive)]

Attachment: 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 :-)

Comment 14

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2 (diff -w)

Comment 15

[Mats Palmgren (inactive)]

BTW, we could assert that |mBroadcasterMap| is empty in
nsXULDocument::~nsXULDocument(), yes?

Comment 16

[Boris Zbarsky [:bzbarsky]]

So,,,  why are the testcases not in the patch?

Comment 17

[[e-mail]]

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?

Comment 18

[Daniel Veditz [:dveditz]]

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.

Comment 19

[Daniel Veditz [:dveditz]]

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.

Comment 20

[chris hofmann]

maybe fx3 b3?

Comment 21

[Martijn Wargers (dead)]

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?

Comment 22

[Daniel Veditz [:dveditz]]

Mats: please answer Neil's questions in comment 17 (particularly 2 and 3)

Comment 23

[Martijn Wargers (dead)]

Attachment: testcase6

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

Comment 24

[Martijn Wargers (dead)]

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).

Comment 25

[Martijn Wargers (dead)]

Attachment: testcase7

Comment 26

[Martijn Wargers (dead)]

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

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Mats, any chance you could respond to Neil's questions in comment 17?

Comment 28

[Brandon Sterne (:bsterne)]

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?

Comment 29

[Martijn Wargers (dead)]

I might be able to come up with a testcase that still crashes in current trunk build. 
See also question in comment 21.

Comment 30

[Brandon Sterne (:bsterne)]

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.

Comment 31

[Johnny Stenback (:jst)]

Mats/Neil, what's needed to move forward here?

Comment 32

[Brandon Sterne (:bsterne)]

Guys, any progress here?  This is a Top Security Bug and needs to be a priority.

Comment 33

[[e-mail]]

Smaug, given that you reviewed it, can you explain the patch to me please?

Comment 34

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

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()

Comment 35

[Mats Palmgren (inactive)]

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.

Comment 36

[Mats Palmgren (inactive)]

Martijn, do you have any tests that still crash?

Comment 37

[[e-mail]]

(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

Comment 38

[Martijn Wargers (dead)]

(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.

Comment 39

[Johnny Stenback (:jst)]

Marking WORKSFORME based on above comments.