329982 - Crash [@ nsXULElement::RemoveChildAt] involving DOMNodeRemoved mutation event

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Jesse Ruderman]

To reproduce the crash, load the testcase.

Before the crash, I see some assertion failures:

###!!! ASSERTION: invalid removeIndex: 'removeIndex >= 0 && !(oldParent == container && removeIndex == insPos)', file /Users/admin/trunk/mozilla/content/base/src/nsGenericElement.cpp, line 2974

###!!! ASSERTION: out-of-bounds access in nsAttrAndChildArray: 'aPos < ChildCount()', file /Users/admin/trunk/mozilla/content/base/src/nsAttrAndChildArray.h, line 81


Debug crash stacktrace:

Thread 0 Crashed:
0   nsCOMPtr<nsIContent>::nsCOMPtr[in-charge](nsIContent*) + 72 (nsCOMPtr.h:627)
1   nsXULElement::RemoveChildAt(unsigned, int) + 176 (nsXULElement.cpp:1025)
2   nsGenericElement::doReplaceOrInsertBefore(int, nsIDOMNode*, nsIDOMNode*, nsIContent*, nsIDocument*, nsIDOMNode**) + 4796 (nsGenericElement.cpp:2978)
3   nsGenericElement::InsertBefore(nsIDOMNode*, nsIDOMNode*, nsIDOMNode**) + 84 (nsGenericElement.cpp:2454)
4   nsHTMLHtmlElement::InsertBefore(nsIDOMNode*, nsIDOMNode*, nsIDOMNode**) + 60 (nsHTMLHtmlElement.cpp:57)
5   nsGenericElement::AppendChild(nsIDOMNode*, nsIDOMNode**) + 76 (nsGenericElement.h:520)
6   nsHTMLHtmlElement::AppendChild(nsIDOMNode*, nsIDOMNode**) + 52 (nsHTMLHtmlElement.cpp:57)
7   _XPTC_InvokeByIndex + 216 (xptcstubsdef.inc:256)
(see attachment for the rest)


Security-sensitive because
(1) There is a violation of C-style array bounds, and it's not checked at runtime except by an assertion.  Something is reading from or writing to a bogus memory location.
(2) The testcase reveals some of the ideas behind bug 325861.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack trace (mac debug)

Comment 3

[Jesse Ruderman]

Attachment: slightly simpler testcase

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Neil might have cycles for this.

Comment 5

[Neil Deakin]

Looks like nsXULElement::RemoveChildAt should have a guard against mutation event handlers changing the DOM, like nsGenericElement::doRemoveAt has. In fact, both functions should really just be consolidated.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Fire mutations after dealing with selection. Also call nsGenericElement::RemoveChildAt rather then duplicating the code in nsXULElement.

I also added some fixes to nsGenericElement::doRemoveChildAt to make it behave a little better if a sibling of a going-away child is removed during the mutationevent.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: same with -w

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216204 [details] [diff] [review]
Patch to fix

>Index: content/base/src/nsGenericElement.cpp
> nsGenericElement::RemoveChildAt(PRUint32 aIndex, PRBool aNotify)
> {
>-  nsCOMPtr<nsIContent> oldKid = GetChildAt(aIndex);
>+  nsCOMPtr<nsIContent> oldKid = mAttrsAndChildren.GetSafeChildAt(aIndex);

I think it's worth adding an assert that this gives the same result as GetChildAt (just to catch cases when someone makes GetChildAt do something weird).

>@@ -2398,44 +2398,46 @@ nsGenericElement::doRemoveChildAt(PRUint
>   NS_PRECONDITION(aKid && aKid->GetParent() == aParent &&
>                   aKid == container->GetChildAt(aIndex), "Bogus aKid");

Add an assert that |container->IndexOf(aKid) == aIndex|, since we're relying on that later, to this precondition?

>+  if (aParent && nsGenericElement::HasMutationListeners(aParent,
>+        NS_EVENT_BITS_MUTATION_NODEREMOVED)) {

Do you happen to know why we're checking on aParent, not on aKid here?  We're dispatching to aKid, so I'd think we'd check for listeners on aKid too...

>+  // Someone may have removed the kid or any of its siblings while that event
>+  // was processing.
>+  if (guard.Mutated(0)) {
>+    aIndex = container->IndexOf(aKid);
>+    if (aIndex < 0) {
>+      return NS_OK;

Hmmm...  I don't buy this.  Specifically, say we have the following situation:

1)  Node A has a DOMNodeRemoved listener that moves it somewhere else in
    the tree.
2)  We call B.appendChild(A).

What will happen is that appendChild will remove A from its current parent.  Except that the mutation listener will just move it to a different parent, so we'll hit this aIndex < 0 case and return NS_OK.  Then we'll put A in the child list of B.  So now A is in two different child lists and confused about what its parent is.

So I think we either we need to deal with this case in appendChild or we need to throw here if aIndex < 0 and aKid has a parent node (possibly the document!).

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> >@@ -2398,44 +2398,46 @@ nsGenericElement::doRemoveChildAt(PRUint
> >   NS_PRECONDITION(aKid && aKid->GetParent() == aParent &&
> >                   aKid == container->GetChildAt(aIndex), "Bogus aKid");
> 
> Add an assert that |container->IndexOf(aKid) == aIndex|, since we're relying
> on that later, to this precondition?

Isn't that pretty much the same thing as checking |aKid == container->GetChildAt(aIndex)|? Or are you worried about bugs in IndexOf/GetChildAt?

> >+  if (aParent && nsGenericElement::HasMutationListeners(aParent,
> >+        NS_EVENT_BITS_MUTATION_NODEREMOVED)) {
> 
> Do you happen to know why we're checking on aParent, not on aKid here?  We're
> dispatching to aKid, so I'd think we'd check for listeners on aKid too...

Good point, i'll fix that in bug 90983 to avoid possibility of perf regression (longer parent chain to check).

> 1)  Node A has a DOMNodeRemoved listener that moves it somewhere else in
>     the tree.
> 2)  We call B.appendChild(A).

We already handle this in appendChild. Look at the GetParent || IsInDoc checks here http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&rev=3.458&mark=2962,3053#2962
Otherwise this would have been a problem before this patch too.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 216204 [details] [diff] [review]
Patch to fix

> Or are you worried about bugs in IndexOf/GetChildAt?

Sort of, and also I feel it's better to assert the invariants we're actually depending on instead of other things that really should be equivalent...

Good point on appendChild.  I looked for that code, but didn't find it.  :(  Maybe add a comment here saying that that caller handles this situation correctly?

r+sr=bzbarsky with the assert nit picked.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'll add he assert. I'd rather not add the comment since this function is really only about removing nodes.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch that landed on 1.8.1 branch

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 216204 [details] [diff] [review]
Patch to fix

approved for 1.8.0 branch, a=dveditz for drivers

Comment 14

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.4) Gecko/20060508 Firefox/1.5.0.4

Comment 15

[Jesse Ruderman]

This has been fixed, right?  I'm trying to figure out why this isn't marked as FIXED.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I probably kept it open for branches.

Comment 17

[Jesse Ruderman]

Crashtest checked in.