339494 - [FIX]Out of bounds and crash removing an attribute during a mutation event for its removal [@ nsAttrAndChildArray::RemoveAttrAt] [@ nsAttrName::ReleaseInternalName]

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Jesse Ruderman]

Loading the testcase causes a crash in (trunk) nightlies and debug builds.

The testcase is similar to the one for bug 338391, but attributes take the place of nodes.

Comment 1

[Jesse Ruderman]

Attachment: testcase

Comment 2

[Jesse Ruderman]

Attachment: stack trace (mac debug)

Comment 3

[Jesse Ruderman]

Nomintating because this crash gets in the way of my testing.

Comment 4

[Boris Zbarsky [:bzbarsky]]

We could just unconditionally reget the attr index in the case when we fire a mutation event.  I don't think it's worth adding this to the mutation guard....

Comment 5

[Boris Zbarsky [:bzbarsky]]

Though actually... per DOM spec we should be firing the event _after_ the attribute has changed.  Why aren't we?

Comment 6

[Boris Zbarsky [:bzbarsky]]

We should be able to test that the attr has actually been unset by the time the mutation event fires....

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixes bug, as expected

Comment 8

[Robert Sayre]

(In reply to comment #6)
> We should be able to test that the attr has actually been unset by the time the
> mutation event fires....

Yes. The fixer of this bug should be able to write that test. 

The examples in testing/mochitest/README.txt show exactly how to write such a test.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'll be happy to try to set the test up when I have the time to sort out how to run mochitest...  That's not going to be for at least a week, though.

Having to do mozilla work in 20-30 minute chunks _really_ sucks. 

Comment 10

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

Comment on attachment 244546 [details] [diff] [review]
Fixes bug, as expected

>Index: content/base/src/nsAttrAndChildArray.cpp

>+nsAttrAndChildArray::RemoveAttrAt(PRUint32 aPos, nsAttrValue& aValue)
> {
>   NS_ASSERTION(aPos < AttrCount(), "out-of-bounds");
>+  NS_ASSERTION(aValue.IsEmptyString(), "Bogus aValue");

This seems like an unnecessary requirement, if there's a non-empty value there it'll simply be destroyed which I think would be expected behavior.

...
>   aPos -= mapped;
>+  ATTRS(mImpl)[aPos].mValue.SwapValueWith(aValue);
>   ATTRS(mImpl)[aPos].~InternalAttr();

I.e. this dtor will simply kill any old value.

>Index: content/xul/content/src/nsXULElement.cpp

>-    rv = mAttrsAndChildren.RemoveAttrAt(index);
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    {
>+        nsAttrValue ignored;
>+        rv = mAttrsAndChildren.RemoveAttrAt(index, ignored);
>+        NS_ENSURE_SUCCESS(rv, rv);
>+    }

Nit: Why bother with the {}? I don't see a reason we're in a hurry to kill the value.

with that, r/sr=sicking

Comment 11

[Boris Zbarsky [:bzbarsky]]

> This seems like an unnecessary requirement, 

Yeah... At some point I removed the dtor there, then realized that it's needed to destroy the nsAttrName anyway.

> I don't see a reason we're in a hurry to kill the value.

Just trying to not have it leak all over the function, since it's really not needed there...  I guess I can remove the {}.

Comment 12

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

> > I don't see a reason we're in a hurry to kill the value.
> 
> Just trying to not have it leak all over the function, since it's really not
> needed there...  I guess I can remove the {}.

I'm fine either way, but IMHO it's more readable without the {} (especially if we were to do it more often)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

Comment 14

[Boris Zbarsky [:bzbarsky]]

Is this not a problem on branch?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Branch version

This is in fact a problem on branch.  This is basically the same patch, merged to the different indentation, different condition for firing mutation events, and different event dispatch methods that we have on branch...

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk, and added tests to mochitest.

Comment 17

[Daniel Veditz [:dveditz]]

Please nominate patched security bugs as release blockers, we look at approval requests for blockers long before we get to the approval requests for non-blockers that are usually less important.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 245306 [details] [diff] [review]
Branch version

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed for 1.8.1.1, 1.8.0.9.

Comment 20

[alice nodelman [:alice] [:anode]]

using testcase verified that crash existed in 1.8.0.8 release build, confirmed that testcase did not crash current nightly builds for either 1.8.0.9 or 1.8.1.1.

verified 1.8.0.9
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.9pre) Gecko/20061130 Firefox/1.5.0.9pre

verified 1.8.1.1
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/2006113003 BonEcho/2.0.0.1pre