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

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

unspecified
mozilla30
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
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.
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.
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.
Assignee

Comment 3

5 years ago
(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?
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
Assignee

Comment 5

5 years ago
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.
Ah, yes, I forgot about that.  So I guess if it gets in that list, the CC should be okay.
Assignee

Comment 7

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

Comment 8

5 years ago
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.
Attachment #8378081 - Flags: review?(wmccloskey)
Attachment #8378081 - Flags: review?(continuation)
Assignee

Comment 9

5 years ago
This part interacts pretty directly with Part 1.
Attachment #8378082 - Flags: review?(wmccloskey)
Attachment #8378082 - Flags: review?(continuation)
Assignee

Comment 10

5 years ago
The rest of these patches are straightforward. Boris should be able to review
them quickly.
Attachment #8378083 - Flags: review?(bzbarsky)
Comment on attachment 8378083 [details] [diff] [review]
Part 3 - Hook Xrays up to the new expando map. v1

r=me
Attachment #8378083 - Flags: review?(bzbarsky) → review+
Comment on attachment 8378084 [details] [diff] [review]
Part 4 - Remove Xray expando slot from WNs. v1

r=me
Attachment #8378084 - Flags: review?(bzbarsky) → review+
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.
Attachment #8378085 - Flags: review?(bzbarsky)
Attachment #8378085 - Flags: review+
Attachment #8378085 - Flags: feedback?(peterv)
Attachment #8378082 - Flags: review?(continuation) → review+
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.
Attachment #8378081 - Flags: review?(continuation) → review+
Assignee

Comment 17

5 years ago
(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.
Assignee

Comment 18

5 years ago
Attachment #8378081 - Attachment is obsolete: true
Attachment #8378081 - Flags: review?(wmccloskey)
Attachment #8378685 - Flags: review?(terrence)
Assignee

Comment 19

5 years ago
Attachment #8378082 - Attachment is obsolete: true
Attachment #8378082 - Flags: review?(wmccloskey)
Attachment #8378686 - Flags: review+
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 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.
Attachment #8378685 - Flags: review?(terrence) → review+
Assignee

Comment 22

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

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

Updated

5 years ago
Blocks: 975042
Assignee

Updated

5 years ago
No longer blocks: XrayToJS
Assignee

Comment 24

5 years ago
(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
Assignee

Comment 25

5 years ago
Err, I meant to trigger that for all platforms:

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

Comment 26

5 years ago
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
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.
Assignee

Comment 30

5 years ago
Actually, I think this is just an IDB bug.

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

Comment 31

5 years ago
This seems to do the trick.
Attachment #8379533 - Flags: review?(khuey)
(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.
Attachment #8378085 - Flags: feedback?(peterv) → feedback+
You need to log in before you can comment on or make changes to this bug.