753277 - Refactor transplant code and improve comments

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Some of this stuff is more confusing than it needs to be. I'm about to fix a bug in this stuff (I think), and I felt like clarifying some things first. Patches soon.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Factor out CCW remapping from JS_TransplantObject and add comments. v1

Attaching a patch. Flagging mrbkap for review.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 622351 [details] [diff] [review]
Factor out CCW remapping from JS_TransplantObject and add comments. v1

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

::: js/src/jsapi.cpp
@@ +1516,5 @@
> + * identities by altering wrappers.
> + *
> + * The |origobj| argument should be the object whose identity needs to be
> + * remapped, usually to another compartment. The contents of |origobj| are
> + * destroyed, unless |origobj == target|.

Here and below, you refer to the |origobj == target| case, while it's now been outlawed (in favor of JS_RefreshCrossCompartmentWrappers).

@@ +1544,4 @@
>      JSCompartment *destination = target->compartment();
>      WrapperMap &map = destination->crossCompartmentWrappers;
>      Value origv = ObjectValue(*origobj);
>      JSObject *obj;

Is there anything we can do about this name? I could never think of anything better here, but |obj| is pretty generic, especially in the face of |origobj| and |target|... Perhaps |newObj|?

Comment 3

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #2)

> Is there anything we can do about this name? I could never think of anything
> better here, but |obj| is pretty generic, especially in the face of
> |origobj| and |target|... Perhaps |newObj|?

newIdentity. I'm making that a separate patch with r=mrbkap. Let me know if you disagree. ;-)

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Factor out CCW remapping from JS_TransplantObject and add comments. v2 r=mrbkap

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Rename |obj| to |newIdentity| in JS_TransplantObject. v1 r=mrbkap

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 3 - Make js_TransplantObjectWithWrapper use RemapWrappers. v1

Comment 7

[Bobby Holley (:bholley)]

Attachment: Part 4 - Rename |obj| to |newWrapper| in js_TransplantObjectWithWrapper. v1

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 5 - Rename tobj to wrapperGuts, and avoid going through the SCSW on the other side unnecessarily. v1

It's not wrong, but it's not necessary either, since we'll just unwrap directly to the underlying object during wrapping. And this is confusing enough as-is.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 6 - Assert that all cross-scope wrapper reparenting operations are cross-compartment, and remove the conditional. v1

The reindentation is pure. No snakes.

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 623078 [details] [diff] [review]
Part 5 - Rename tobj to wrapperGuts, and avoid going through the SCSW on the other side unnecessarily. v1

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

Only the rename happened, no?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Ms2ger from comment #10)

> Only the rename happened, no?

No, it's subtle. Note that we previously were pointing to newWrapper, and now we point to targetobj.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 623079 [details] [diff] [review]
Part 6 - Assert that all cross-scope wrapper reparenting operations are cross-compartment, and remove the conditional. v1

A diff -w would have been nice here.

Comment 13

[Bobby Holley (:bholley)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1e91648bba69

Comment 14

[Bobby Holley (:bholley)]

Looks green. Pushed to m-i:

http://hg.mozilla.org/integration/mozilla-inbound/rev/6678097e44e5
http://hg.mozilla.org/integration/mozilla-inbound/rev/86ce1810ab6f
http://hg.mozilla.org/integration/mozilla-inbound/rev/2330abd18337
http://hg.mozilla.org/integration/mozilla-inbound/rev/d3af846d8bc9
http://hg.mozilla.org/integration/mozilla-inbound/rev/2d846fdb9ddc
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8bdf0315130

Comment 15

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a8bdf0315130
https://hg.mozilla.org/mozilla-central/rev/2d846fdb9ddc
https://hg.mozilla.org/mozilla-central/rev/d3af846d8bc9
https://hg.mozilla.org/mozilla-central/rev/2330abd18337
https://hg.mozilla.org/mozilla-central/rev/86ce1810ab6f
https://hg.mozilla.org/mozilla-central/rev/6678097e44e5