Last Comment Bug 664388 - NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS uses wrong pointer
: NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS uses wrong pointer
Status: RESOLVED FIXED
:
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]
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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)));

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] 2011-06-15 03:04:26 PDT
Created attachment 539468 [details] [diff] [review]
v1
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-15 14:24:23 PDT
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] 2011-06-28 05:49:54 PDT
http://hg.mozilla.org/mozilla-central/rev/c36e6d83d89d

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