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

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
DOM
P1
normal
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Jesse Ruderman, Assigned: bz)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9alpha1
crash, testcase, verified1.8.0.9, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

11 years ago
Created attachment 223594 [details]
testcase
(Reporter)

Updated

11 years ago
Group: security
(Reporter)

Comment 2

11 years ago
Created attachment 223595 [details]
stack trace (mac debug)
(Reporter)

Comment 3

11 years ago
Nomintating because this crash gets in the way of my testing.
Flags: blocking1.9a1?
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....
Though actually... per DOM spec we should be firing the event _after_ the attribute has changed.  Why aren't we?
We should be able to test that the attr has actually been unset by the time the mutation event fires....
Flags: in-testsuite?
Created attachment 244546 [details] [diff] [review]
Fixes bug, as expected
Attachment #244546 - Flags: superreview?(bugmail)
Attachment #244546 - Flags: review?(bugmail)
(Assignee)

Updated

11 years ago
Assignee: general → bzbarsky
OS: Mac OS X 10.4 → All
Priority: -- → P1
Hardware: Macintosh → All
Summary: Out of bounds and crash removing an attribute during a mutation event for its removal [@ nsAttrAndChildArray::RemoveAttrAt] [@ nsAttrName::ReleaseInternalName] → [FIX]Out of bounds and crash removing an attribute during a mutation event for its removal [@ nsAttrAndChildArray::RemoveAttrAt] [@ nsAttrName::ReleaseInternalName]
Target Milestone: --- → mozilla1.9alpha

Comment 8

11 years ago
(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.
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 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
Attachment #244546 - Flags: superreview?(bugmail)
Attachment #244546 - Flags: superreview+
Attachment #244546 - Flags: review?(bugmail)
Attachment #244546 - Flags: review+
> 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 {}.
> > 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)
Created attachment 245278 [details] [diff] [review]
Updated to comments
Attachment #244546 - Attachment is obsolete: true
Is this not a problem on branch?
Created attachment 245306 [details] [diff] [review]
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...
Attachment #245306 - Flags: superreview?(bugmail)
Attachment #245306 - Flags: review?(bugmail)
Attachment #245306 - Flags: approval1.8.1.1?
Attachment #245306 - Flags: approval1.8.0.9?
Fixed on trunk, and added tests to mochitest.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Flags: in-testsuite?
Flags: in-testsuite+
Flags: blocking1.9a1?
Resolution: --- → FIXED
Attachment #245306 - Flags: superreview?(bugmail)
Attachment #245306 - Flags: superreview+
Attachment #245306 - Flags: review?(bugmail)
Attachment #245306 - Flags: review+
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.
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9+
Whiteboard: [sg:critical?]
Comment on attachment 245306 [details] [diff] [review]
Branch version

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #245306 - Flags: approval1.8.1.1?
Attachment #245306 - Flags: approval1.8.1.1+
Attachment #245306 - Flags: approval1.8.0.9?
Attachment #245306 - Flags: approval1.8.0.9+
Fixed for 1.8.1.1, 1.8.0.9.
Keywords: fixed1.8.0.9, fixed1.8.1.1
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
Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
Group: security
Crash Signature: [@ nsAttrAndChildArray::RemoveAttrAt] [@ nsAttrName::ReleaseInternalName]
You need to log in before you can comment on or make changes to this bug.