461053 - [FIX] Crash [@ nsXULDocument::SynchronizeBroadcastListener] with command

Status: VERIFIED FIXED

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

Description

[Martijn Wargers (dead)]

See testcase, which crashes current trunk build and Firefox 3 within 100ms.

http://crash-stats.mozilla.com/report/index/69096ca8-9fbd-11dd-9496-001a4bd43e5c?p=1
0  	xul.dll  	nsXULDocument::SynchronizeBroadcastListener  	 content/xul/document/src/nsXULDocument.cpp:703
1 	xul.dll 	nsTArray_base::SwapArrayElements 	obj-firefox/xpcom/build/nsTArray.cpp:205
2 	xul.dll 	nsXULDocument::EndUpdate 	content/xul/document/src/nsXULDocument.cpp:3271

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Brandon Sterne (:bsterne)]

Attachment: backtrace from trunk crash

I'm having trouble determining the exploitability of this bug.  Here's a backtrace from the crash on a fresh Linux m-c debug build.

Comment 3

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

Attachment: create a list of to-be-broadcasted attributes before broadcasting

...and make sure the attribute still there in broadcaster before setting
it to listener.

Comment 4

[[e-mail]]

Comment on attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

From whom did you intend review? ;-)

Comment 5

[Neil Deakin]

Comment on attachment 347769 [details] [diff] [review]
create a list of to-be-broadcasted attributes before broadcasting

>-            const nsAttrName* attrName = broadcaster->GetAttrNameAt(count);
>+        nsTArray<nsAttrNameInfo> attributes(count);
>+        for (PRUint32 i = 0; i < count; ++i) {
>+            const nsAttrName* attrName = broadcaster->GetAttrNameAt(i);
>             PRInt32 nameSpaceID = attrName->NamespaceID();
>             nsIAtom* name = attrName->LocalName();
> 
>             // _Don't_ push the |id|, |ref|, or |persist| attribute's value!
>             if (! CanBroadcast(nameSpaceID, name))
>                 continue;
> 
>+            attributes.AppendElement(nsAttrNameInfo(attrName->NamespaceID(),
>+                                                    attrName->LocalName(),
>+                                                    attrName->GetPrefix()));

You should either use the temporary variables in both cases or neither.
>-                              mInitialLayoutComplete);
>+            if (broadcaster->GetAttr(nameSpaceID, name, value)) {
>+              listener->SetAttr(nameSpaceID, name, attributes[count].mPrefix,
>+                                value, mInitialLayoutComplete);
>+            }

What if the attribute has a value on the listener but not on the broadcaster? Is this creating different behaviour in some situation?

Comment 6

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

(In reply to comment #5)
> You should either use the temporary variables in both cases or neither.
Indeed, a left over from a previous (local) version. Fixing.

> What if the attribute has a value on the listener but not on the broadcaster?
> Is this creating different behaviour in some situation?
This is creating different behavior than currently only if someone 
modifies attribute using mutation listeners.
Note, if the element if removed (which if(GetAttr) checks), other code
should already remove the attribute from listener.
So this is IMO fixing the problem when removed attribute is recreated on
listener with empty value.

Comment 7

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

Attachment: v2

Comment 8

[Brandon Sterne (:bsterne)]

Looks like we have a reviewed patch here.  Can we get this landed?

Comment 9

[Martijn Wargers (dead)]

The patch needs sr+, afaik.

Comment 10

[Martijn Wargers (dead)]

The patch still isn't ready for check-in, afaik.
This bug has to be blocking1.9.1+ or the patch needs to get approval1.9.1 or both.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ah, indeed!

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 347850 [details] [diff] [review]
v2

a191=beltzner

Comment 13

[Johnny Stenback (:jst)]

This is ready to go, not marking as a blocker.

Comment 14

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081204 Minefield/3.2a1pre

Comment 15

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

Fixed on trunk and 1.9.1

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on 1.9.1 using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre

Martijn, can we add the testcase to the crashtest harness?

Comment 17

[Daniel Veditz [:dveditz]]

This is always a null deref in debug builds, but could it be uninitialized in optimized builds? Can't tell, let's just make sure it's fixed on 1.9.0

Comment 18

[Daniel Veditz [:dveditz]]

Olli: does this patch work for the 1.9.0 branch or will it require porting?

Comment 19

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

Comment on attachment 347850 [details] [diff] [review]
v2

Applies to 1.9.0 with some small fuzz

Comment 20

[Daniel Veditz [:dveditz]]

Comment on attachment 347850 [details] [diff] [review]
v2

Approved for 1.9.0.8, a=dveditz

Comment 21

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

Checking in content/xul/document/src/nsXULDocument.cpp;
/cvsroot/mozilla/content/xul/document/src/nsXULDocument.cpp,v  <--  nsXULDocument.cpp
new revision: 1.831; previous revision: 1.830
done

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.0.8 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8pre) Gecko/2009031904 GranParadiso/3.0.8pre. Crashes cleanly in 1.9.0.7.