Last Comment Bug 664388 - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS uses wrong pointer
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla7
Assigned To: Peter Van der Beken [:peterv]
: Nathan Froyd [:froydnj]
Depends on:
  Show dependency treegraph
Reported: 2011-06-15 03:00 PDT by Peter Van der Beken [:peterv]
Modified: 2011-06-28 05:49 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

v1 (1.28 KB, patch)
2011-06-15 03:04 PDT, Peter Van der Beken [:peterv]
bent.mozilla: review+
Details | Diff | Splinter Review

Description User image Peter Van der Beken [:peterv] 2011-06-15 03:00:32 PDT
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)));


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 User image Peter Van der Beken [:peterv] 2011-06-15 03:04:26 PDT
Created attachment 539468 [details] [diff] [review]
Comment 2 User image Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-15 14:24:23 PDT
Comment on attachment 539468 [details] [diff] [review]

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

Makes sense to me.
Comment 3 User image Peter Van der Beken [:peterv] 2011-06-28 05:49:54 PDT

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