Last Comment Bug 339494 - [FIX]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 fo...
Status: RESOLVED FIXED
[sg:critical?]
: crash, testcase, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla1.9alpha1
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
Depends on:
Blocks: 325861
  Show dependency treegraph
 
Reported: 2006-05-28 04:48 PDT by Jesse Ruderman
Modified: 2006-12-22 11:00 PST (History)
5 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (579 bytes, application/xhtml+xml)
2006-05-28 04:49 PDT, Jesse Ruderman
no flags Details
stack trace (mac debug) (7.56 KB, text/plain)
2006-05-28 04:53 PDT, Jesse Ruderman
no flags Details
Fixes bug, as expected (10.27 KB, patch)
2006-11-02 22:57 PST, Boris Zbarsky [:bz]
jonas: review+
jonas: superreview+
Details | Diff | Review
Updated to comments (10.06 KB, patch)
2006-11-10 16:04 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Branch version (10.47 KB, patch)
2006-11-10 20:32 PST, Boris Zbarsky [:bz]
jonas: review+
jonas: superreview+
dveditz: approval1.8.0.9+
dveditz: approval1.8.1.1+
Details | Diff | Review

Description Jesse Ruderman 2006-05-28 04:48:14 PDT
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 2006-05-28 04:49:19 PDT
Created attachment 223594 [details]
testcase
Comment 2 Jesse Ruderman 2006-05-28 04:53:09 PDT
Created attachment 223595 [details]
stack trace (mac debug)
Comment 3 Jesse Ruderman 2006-07-27 02:42:25 PDT
Nomintating because this crash gets in the way of my testing.
Comment 4 Boris Zbarsky [:bz] 2006-11-02 22:22:15 PST
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 [:bz] 2006-11-02 22:25:53 PST
Though actually... per DOM spec we should be firing the event _after_ the attribute has changed.  Why aren't we?
Comment 6 Boris Zbarsky [:bz] 2006-11-02 22:54:54 PST
We should be able to test that the attr has actually been unset by the time the mutation event fires....
Comment 7 Boris Zbarsky [:bz] 2006-11-02 22:57:42 PST
Created attachment 244546 [details] [diff] [review]
Fixes bug, as expected
Comment 8 Robert Sayre 2006-11-02 22:59:26 PST
(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 [:bz] 2006-11-02 23:12:31 PST
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) 2006-11-07 16:51:24 PST
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 [:bz] 2006-11-07 16:56:49 PST
> 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) 2006-11-07 18:24:31 PST
> > 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 [:bz] 2006-11-10 16:04:08 PST
Created attachment 245278 [details] [diff] [review]
Updated to comments
Comment 14 Boris Zbarsky [:bz] 2006-11-10 16:26:33 PST
Is this not a problem on branch?
Comment 15 Boris Zbarsky [:bz] 2006-11-10 20:32:54 PST
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...
Comment 16 Boris Zbarsky [:bz] 2006-11-10 20:34:15 PST
Fixed on trunk, and added tests to mochitest.
Comment 17 Daniel Veditz [:dveditz] 2006-11-20 12:35:08 PST
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] 2006-11-20 12:36:50 PST
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 [:bz] 2006-11-20 20:09:55 PST
Fixed for 1.8.1.1, 1.8.0.9.
Comment 20 alice nodelman [:alice] [:anode] 2006-11-30 12:06:30 PST
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

Note You need to log in before you can comment on or make changes to this bug.