973780 - Store Xray expandos in a WeakMap in the target compartment, rather than hanging them off the target object

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Currently we hang Xray expando objects off a slot on the target reflector. This is convenient, because it means we don't have to do any special GC or lifetime management.

To quote Peter when we designed this setup: "Yeah...we could just put those JS objects in a hash map, and then we could trace it..." <laughter>

Unfortunately, our trick isn't going to work for JSXrays, because trying to shotgun a slot for each kind of target object would be an uphill battle at best.

In theory, WeakMaps have underlying C++ machinery that we can re-use here, and hopefully give us something saner than the traditional PLDHash madness.

We could implement this just for JSXrays (using the traits), but once we have the machinery, we might as well use it for everything, and unify some tricky code.

Comment 1

[Andrew McCreight [:mccr8]]

In addition to the GC, you'll want to tell the CC about the stuff in these weak maps.  The patch in bug 693527 shows how to do it.  Basically, you trigger some callback for every (map, key, value) triple in the weak map.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

We'll have to be careful here. The weakmap code is designed for the same-compartment case and we've had bugs in the past when we tried to use it across compartments.
https://bugzilla.mozilla.org/show_bug.cgi?id=798678#c2

I'm sure there's some way to make this work though.

Comment 3

[Bobby Holley (:bholley)]

(In reply to Bill McCloskey (:billm) from comment #2)
> We'll have to be careful here. The weakmap code is designed for the
> same-compartment case and we've had bugs in the past when we tried to use it
> across compartments.
> https://bugzilla.mozilla.org/show_bug.cgi?id=798678#c2
> 
> I'm sure there's some way to make this work though.

I don't understand. Where does the cross-compartment part come in? This will be an object that hangs off the XPCWrappedNativeScope, and maps objects in that scope to (same-compartment) expando holder objects, which themselves have properties that are (potentially) cross-compartment wrappers. This should be straightforward, no?

Comment 4

[Jason Orendorff [:jorendorff]]

bholley's use case is same-compartment: all keys and values would be in the wrapped object's compartment.

The keys in the WeakMap will be objects in that compartment, for which some other compartment has had an x-ray wrapper. The values are Objects which store expando properties attached by other compartments via x-ray wrappers. Plain old Objects, no proto, never exposed to script, just for storage.

The reason this WeakMap can't be in the same compartment as the wrapper is that if two compartments both have x-ray wrappers pointing to an object, they need to see the same expando properties. :-P

Comment 5

[Bobby Holley (:bholley)]

I have patches that work locally. Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=9ddbd63b858e

I want to talk to Andrew about the cycle collector stuff tomorrow. I don't see why any changes are needed, because it seems like it should already be handled the same way it's handled for any other WeakMap - it gets added to the per-compartment gcWeakMapList during the trace() call, and then the CC hooks into that via js::TraceWeakMaps.

Comment 6

[Andrew McCreight [:mccr8]]

Ah, yes, I forgot about that.  So I guess if it gets in that list, the CC should be okay.

Comment 7

[Bobby Holley (:bholley)]

Green, with one ASAN failure that's now fixed. Uploading patches and flagging for review.

Comment 8

[Bobby Holley (:bholley)]

Attachment: Part 1 - Expose a wrapper for the internal WeakMap class outside of the engine. v1

I'm hoping for a careful review from both Bill and Andrew here. In particular,
I'm concerned about interactions with IGC and the cycle collector. Is there
any chance that we might delete one of these things while it's stashed in a
vector, and then have it restored (as garbage) later? I don't really
understand the whole save/restore setup with weakmaps.

Comment 9

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement an expando WeakMap on XPCWrappedNativeScope. v1

This part interacts pretty directly with Part 1.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 3 - Hook Xrays up to the new expando map. v1

The rest of these patches are straightforward. Boris should be able to review
them quickly.

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 4 - Remove Xray expando slot from WNs. v1

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 5 - Remove Xray expando slot from new-binding objects. v1

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8378083 [details] [diff] [review]
Part 3 - Hook Xrays up to the new expando map. v1

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8378084 [details] [diff] [review]
Part 4 - Remove Xray expando slot from WNs. v1

r=me

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8378085 [details] [diff] [review]
Part 5 - Remove Xray expando slot from new-binding objects. v1

So why are DOM_INTERFACE_SLOTS_BASE and DOM_INTERFACE_PROTO_SLOTS_BASE 1 and not 0?

For the latter, it's because it's actually DOM_PROTO_INSTANCE_CLASS_SLOT + 1 (which is what you should use in this header).

But for the interface object, I believe we don't actually use reserved slot 0 right now.  Peter, can you confirm?

r=me with those fixed.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8378081 [details] [diff] [review]
Part 1 - Expose a wrapper for the internal WeakMap class outside of the engine. v1

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

> Is there any chance that we might delete one of these things while it's stashed in a vector, and then have it restored (as garbage) later? I don't really understand the whole save/restore setup with weakmaps.

Sorry, what is being stashed in a vector?  Which save/restore setup?  Are you talking about some kind of generational GC thing?

::: js/public/WeakMap.h
@@ +15,5 @@
> +// The supported template specializations are enumerated in jsweakmap.cpp. If
> +// you want to use this class for a different key/value combination, add it to
> +// the list and the compiler will generate the relevant machinery.
> +template <typename K, typename V>
> +class WeakMap {

I think this should be called WeakMapPtr, as you are really just creating a pointer for a weak map.  Likewise with the file name.

@@ +22,5 @@
> +    WeakMap() : ptr(nullptr) {};
> +    bool init(JSContext *cx);
> +    bool initialized() { return ptr != nullptr; };
> +    void destroy();
> +    void destroyIfInitialized() { if (initialized()) destroy(); }

|destroyIfInitialized| seems unnecessary (why does this exist for destroy() and not trace(), for instance?).  I guess this does match the ergonomics of the usual setting a smart pointer to null, so I guess either way is okay.

::: js/src/jsweakmap.cpp
@@ +465,5 @@
>      return weakMapProto;
>  }
>  
> +//
> +// Machinery for the externally-linkable JS::WeakMap, which wraps js::WeakMap

Could this go in a separate file?

@@ +498,5 @@
> +    typedef Type* PtrType;
> +    static PtrType cast(void *ptr) { return static_cast<PtrType>(ptr); }
> +};
> +
> +}

Maybe add a comment saying what namespace you are closing?  (The blank one.)  Otherwise this is just kind of floating in space.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #16)
> > Is there any chance that we might delete one of these things while it's stashed in a vector, and then have it restored (as garbage) later? I don't really understand the whole save/restore setup with weakmaps.
> 
> Sorry, what is being stashed in a vector?  Which save/restore setup?  Are
> you talking about some kind of generational GC thing?

I'm worried about this stuff: http://mxr.mozilla.org/mozilla-central/source/js/src/jsgc.cpp#3289

Hopefully terrence knows what it's about.

Addressed the rest of Andrew's comments, and flagging terrence for review on the updated patch.

Comment 18

[Bobby Holley (:bholley)]

Attachment: Part 1 - Expose a wrapper for the internal WeakMap class outside of the engine. v2 r=mccr8

Comment 19

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement an expando WeakMap on XPCWrappedNativeScope. v2 r=mccr8

Comment 20

[Andrew McCreight [:mccr8]]

I don't know about the GGC stuff, but as far as the CC is concerned, it iterates over the list of weak maps and scans them all, then copies the data out into its own copy of stuff, and then doesn't interact with the weak maps again, so it will be okay if the weak maps go away during the CC (which can happen with ICC).

Comment 21

[Terrence Cole [:terrence]]

Comment on attachment 8378685 [details] [diff] [review]
Part 1 - Expose a wrapper for the internal WeakMap class outside of the engine. v2 r=mccr8

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

r=me

::: js/public/WeakMapPtr.h
@@ +15,5 @@
> +// The supported template specializations are enumerated in WeakMapPtr.cpp. If
> +// you want to use this class for a different key/value combination, add it to
> +// the list and the compiler will generate the relevant machinery.
> +template <typename K, typename V>
> +class WeakMapPtr {

{ on newline.

@@ +16,5 @@
> +// you want to use this class for a different key/value combination, add it to
> +// the list and the compiler will generate the relevant machinery.
> +template <typename K, typename V>
> +class WeakMapPtr {
> +    void *ptr;

Please move the implementation details to the bottom in a new private section.

@@ +18,5 @@
> +template <typename K, typename V>
> +class WeakMapPtr {
> +    void *ptr;
> +  public:
> +    WeakMapPtr() : ptr(nullptr) {};

Would it be possible to delete the copy constructor or at least replace it with a move constructor that more accurately models what's going on here?

::: js/src/vm/WeakMapPtr.cpp
@@ +15,5 @@
> +
> +namespace {
> +
> +template<typename T>
> +struct DataType {

{ on newline.

@@ +17,5 @@
> +
> +template<typename T>
> +struct DataType {
> +    typedef void* Encapsulated;
> +    static T NullValue() { return nullptr; }

Could we just leave the body empty, or at least comment these definitions, so that void* is more clearly not a valid weakmap key?

@@ +21,5 @@
> +    static T NullValue() { return nullptr; }
> +};
> +
> +template<>
> +struct DataType<JSObject*> {

{ on newline.

@@ +27,5 @@
> +    static JSObject *NullValue() { return nullptr; }
> +};
> +
> +template<>
> +struct DataType<JS::Value> {

{ on newline.

@@ +33,5 @@
> +    static JS::Value NullValue() { return JS::UndefinedValue(); }
> +};
> +
> +template <typename K, typename V>
> +struct Utils {

{ on newline.

Comment 22

[Bobby Holley (:bholley)]

Thanks for the reviews everyone. One final all-platform try push:

https://tbpl.mozilla.org/?tree=Try&rev=2072d1587d21

Comment 23

[Bobby Holley (:bholley)]

https://tbpl.mozilla.org/?tree=Try&rev=5eb706ffa57a

Comment 24

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #23)
> https://tbpl.mozilla.org/?tree=Try&rev=5eb706ffa57a

More windows bustage. Canceled the push and trying again:

https://tbpl.mozilla.org/?tree=Try&rev=ef5cbbc6e4ed

Comment 25

[Bobby Holley (:bholley)]

Err, I meant to trigger that for all platforms:

https://tbpl.mozilla.org/?tree=Try&rev=a1655e9ac390

Comment 26

[Bobby Holley (:bholley)]

Tests look green, just some linux-debug build bustage. I think I've got that sorted. Let's check:

https://tbpl.mozilla.org/?tree=Try&rev=a68432ec400a

Comment 27

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8db4d719f5f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/523e9ec89a9f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/29fbf3dac1ad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/861f8a64627f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0f5f58d5d410

Comment 28

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a917689cc392 because those Windows mochitest-3 assertions on try that you figured weren't related to your push? They were entirely related to your push.

Comment 29

[Bobby Holley (:bholley)]

Hm, that is rather perplexing. Let's bisect:

https://tbpl.mozilla.org/?tree=Try&rev=6b2f6c1877c2
https://tbpl.mozilla.org/?tree=Try&rev=4975f0e0d8d2

Comment 30

[Bobby Holley (:bholley)]

Actually, I think this is just an IDB bug.

https://tbpl.mozilla.org/?tree=Try&rev=eabad5029482

Comment 31

[Bobby Holley (:bholley)]

Attachment: Assertion Fix - Don't double-unregister during IDB teardown. v1

This seems to do the trick.

Comment 32

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/46c9478b570e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fb680cb6a07e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/22dd1b0a3752
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59fcb8a3225f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/913cb7066263
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/468d3ce7f848

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46c9478b570e
https://hg.mozilla.org/mozilla-central/rev/fb680cb6a07e
https://hg.mozilla.org/mozilla-central/rev/22dd1b0a3752
https://hg.mozilla.org/mozilla-central/rev/59fcb8a3225f
https://hg.mozilla.org/mozilla-central/rev/913cb7066263
https://hg.mozilla.org/mozilla-central/rev/468d3ce7f848

Comment 34

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #15)
> But for the interface object, I believe we don't actually use reserved slot
> 0 right now.  Peter, can you confirm?

That seems right.