Last Comment Bug 754311 - Copy properties before nulling out the private of about-to-be-transplanted reflectors
: Copy properties before nulling out the private of about-to-be-transplanted re...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Bobby Holley (PTO through June 13)
:
Mentors:
Depends on:
Blocks: 752309
  Show dependency treegraph
 
Reported: 2012-05-11 08:52 PDT by Bobby Holley (PTO through June 13)
Modified: 2012-05-15 06:36 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Null out the private of soon-to-be-transplanted reflectors _after_ copying their properties onto the holder. v1 (4.44 KB, patch)
2012-05-11 09:15 PDT, Bobby Holley (PTO through June 13)
mrbkap: review+
Details | Diff | Review

Description Bobby Holley (PTO through June 13) 2012-05-11 08:52:50 PDT
This is one of the bugs causing bug 752309. Patch coming right up.
Comment 1 Bobby Holley (PTO through June 13) 2012-05-11 09:15:14 PDT
Created attachment 623180 [details] [diff] [review]
Null out the private of soon-to-be-transplanted reflectors _after_ copying their properties onto the holder. v1

Attaching a patch. Flagging mrbkap for review.
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-05-14 11:42:07 PDT
Comment on attachment 623180 [details] [diff] [review]
Null out the private of soon-to-be-transplanted reflectors _after_ copying their properties onto the holder. v1

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

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1660,5 @@
> +            // replaced anyway by the ensuing brain trainsplant, so it doesn't
> +            // really matter. But it can stick around if we take the
> +            // js_TransplantObjectWithWrapper path, or if we've got a bug somewhere.
> +            // If that happens, we want to crash cleanly with a null dereference
> +            // rather than mucking around with the wrong XPCWN.

This is actually important for another reason as well: at this point in time, there are now two JSObjects with the same XPCWrappedNative and they'll both try to delete they're underlying wrapped native when they get finalized. Even though we're going to brain transplant this object, all that actually means is that we're going to swap() it with another object, so we need to forcibly null out the private here.
Comment 3 Bobby Holley (PTO through June 13) 2012-05-14 14:31:52 PDT
Pushed to m-i with an updated comment per comment 2:

http://hg.mozilla.org/integration/mozilla-inbound/rev/b5bef2ea3fd9
Comment 4 Ed Morley [:emorley] 2012-05-15 06:36:04 PDT
https://hg.mozilla.org/mozilla-central/rev/b5bef2ea3fd9

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