664388 - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS uses wrong pointer

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Peter Van der Beken [:peterv]]

NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS uses tmp, which is a pointer to the concrete class that we're traversing. However, this passes tmp to the participant's Trace hook, which tries to cast the pointer to the concrete class again (through the CC nsISupports pointer). If the object has multiple inheritance from nsISupports and the interface we use to cast to the CC nsISupports isn't the first interface that the object inherits from we'll end up doing a bad cast.

Here's an example:

class A : public nsIFoo,
          public nsIBar
{}

We use nsIBar to cast to the CC nsISupports.

In Traverse:

p is void*, gotten by doing static_cast<void*>(static_cast<nsISupports*>(static_cast<nsIBar*>(anObject))).

We set tmp to static_cast<A*>(static_cast<nsIBar*>(static_cast<nsISupports*>(p)));

We pass (void*)tmp to Trace via NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS.

In Trace:

p is void*, gotten by doing (void*)tmp, which is equal to static_cast<void*>(static_cast<nsISupports*>(static_cast<nsIFoo*>(anObject))).

We try to get tmp by doing static_cast<A*>(static_cast<nsIBar*>(static_cast<nsISupports*>(p))), which is a bad cast.

Passing p to NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in Traverse will make p in Trace be static_cast<void*>(static_cast<nsISupports*>(static_cast<nsIBar*>(anObject))), making the cast in Trace correct.

I think this currently isn't a problem because we always used the first interface to cast to/from nsISupports, but it's fragile and we're going to use this with objects where that isn't true in the future.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 2

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

Comment on attachment 539468 [details] [diff] [review]
v1

Review of attachment 539468 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense to me.

Comment 3

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/c36e6d83d89d