777385 - Support Paris bindings objects that are either nsISupports or non-cycle-collected as weak map keys

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Andrew McCreight [:mccr8]]

With bug 753517 landed, I believe that all wrapper cached things will support wrapper preservation.  Is that something that is intended to be an invariant?  Is there some way for us to statically check that property?  Right now we rely on nsContentUtils::CheckCCWrapperTraversal + fuzzing to detect cases where we try to preserve something that can't be preserved, which is not ideal.

If we can rely on this invariant, then we can change the PreserveWrapper callback in XPCJSRuntime to support all wrapper cached objects as weak map keys, not just nodes.

Comment 1

[Andrew McCreight [:mccr8]]

More than just nodes are wrapper-preservable, so it would be nice to figure out how to test for that property, and then support those classes as weak map keys.

Longer term, we may be able to implement billm's technique for proxies, where classes implement a hook to return a delegate that governs whether they are alive or not. A native would just say that the underlying object is always marked gray to the GC, and give the actual native to the CC. This would be much trickier, because we'd have to deal with non-JS delegates. This would basically be reimplementing wrapper preservation, for the special case of weak map keys.

Comment 2

[Boris Zbarsky [:bzbarsky]]

One obvious way is to add a JSClass flag... but I'm not sure there are free flags so much.

If non-wrapper-preservable things can't be used as keys, does that mean that non-wrappercached WebIDL bindings also can't be used?  That's somewhat unfortunate, if so....

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> If non-wrapper-preservable things can't be used as keys, does that mean that
> non-wrappercached WebIDL bindings also can't be used?  That's somewhat
> unfortunate, if so....

This is basically the same problem as expandos face, as far as I am aware. Is there some reason it is worse for weak map keys?

I piggybacked on wrapper preservation back in bug 680937, because it was easier that way, but if it is important to support all natives, then I can try looking into creating some kind of special purpose wrapper preservation mechanism for weak maps, as I described in comment 1.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Yes, because there are objects that we know statically can never have the expando problem.  These are objects that can never be "gotten" from c++, only "created".  So even with the WebIDL bindings such objects do not use wrapper preservation or indeed a wrapper cache at all.

The question is whether we can support such objects here....

Comment 5

[Andrew McCreight [:mccr8]]

I think I will generalize and focus this a bit, to look at adding support for all new DOM bindings objects.  (Which are not all wrapper-preservable.)  I'll leave bug 804386 for trying to come up with a more complex approach that might support objects with ye olde bindings.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Add support for Paris bindings objects as weak map keys, WIP

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 674795 [details] [diff] [review]
Add support for Paris bindings objects as weak map keys, WIP

Don't you need to test whether UnwrapDOMObjectToISupports returned true?

Also, can we handle non-isupports Paris binding objects somehow?

Comment 8

[Andrew McCreight [:mccr8]]

This may be relying on unspecified behavior, but if UnwrapDOMObjectToISupports returns false, then it doesn't set its argument, so |supports| will remain NULL.  Oops, I guess we'll get a null deref then because of the way I QI.

This patch is supposed to handle non-ISupports objects by doing nothing (it just return true, which means "hey, weak maps, this object is okay!"). Can non-ISupports Paris bindings objects be owned by the native side?  I was assuming that anything that could be owned from the native-side would nsISupports+nsWrapperCached+preservable.

Comment 9

[Andrew McCreight [:mccr8]]

So, with a check of the return value, the call would be like this:
    if (!mozilla::dom::UnwrapDOMObjectToISupports(obj, supports))
        return true;

Comment 10

[Andrew McCreight [:mccr8]]

The semantics of this function are that, when the JS engine attempts to use an object as a key, it checks if it is a wrapped native.  If it is, then it calls into this callback. If the callback returns true, then the JS engine goes ahead with inserting the key. Otherwise, it throws an exception. The callback has to do whatever additional work is required.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I think we can have refcounted and cced but not nsISupports natives....

Comment 12

[Andrew McCreight [:mccr8]]

Attachment: Add support for Paris bindings objects as weak map keys, WIP, v2

It turns out Paris bindings objects are some kind of proxy thing, so we have to detect them differently in the weak map.  With this patch, we successfully use a .style as a weak map key, and it survives GC/CC. Thanks to bz for patiently explaining how the bindings stuff works.

I need to write more test cases, to cover non-wrapper-cached and non-nsISupports Paris bindings objects, to see whether or not they work as expected.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> It turns out Paris bindings objects are some kind of proxy thing

Some of them are.  The ones that have weird named/indexed getter stuff going on...  The others are not.

I think the test you actually want is:

    if (key->getClass()->ext.isWrappedNative ||
        (key->getClass()->flags & JSCLASS_IS_DOMJSCLASS) ||
        (key->isProxy() && GetProxyHandler(key)->family() == GetListBaseHandlerFamily()))

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: part 1: DOMClass's participant should be a script tracer

To support wrapper preservation, we need to know that the CC participant stored in the DOM class is a script tracer.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 2: Add support for Paris bindings objects as weak map keys, WIP, v3

With khuey's advice, I understand things a bit more now. This should handle non-nsISupports refcounted stuff "correctly", though the bit where I extract the nsWrapperCache from the object is probably sketchy and/or wrong. I still haven't tested anything beyond .style, though.

Comment 16

[Andrew McCreight [:mccr8]]

I don't think I can write a good test for preservation of non-nsISupports refcounted Paris bindings objects. The only one that exists currently, as far as I can see, is AudioContext. It looks like that all of the native objects that can hold a reference to them have the AudioContext as their parent, so their reflector keeps the context's reflector alive.  I'm not sure how to keep a native reference to any of these audio objects without the reflector.

Comment 17

[Andrew McCreight [:mccr8]]

I'll still add a test, to ensure that the code path to handle preservation for non-nsISupports refcounted objects doesn't crash or whatnot.

Comment 18

[Andrew McCreight [:mccr8]]

Try run was okay on Linux64 and OSX. I'll have to upload my final patch at some point.
https://tbpl.mozilla.org/?tree=Try&rev=796d7d4afcbd

Comment 19

[Andrew McCreight [:mccr8]]

Attachment: part 2: add PreserveWrapper method for new bindings, use it for weak maps

I added a new mozilla::dom::PreserveWrapper() method to BindingUtils, as this seems generally useful. Then I change PreserveWrapper in XPCJSRuntime to use this in the case of new DOM bindings.

I added some tests, but they aren't very thorough, as there is no good way to test the more obscure cases of bindings.

Comment 20

[Peter Van der Beken [:peterv]]

Comment on attachment 675204 [details] [diff] [review]
part 1: DOMClass's participant should be a script tracer

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

::: dom/bindings/DOMJSClass.h
@@ +71,5 @@
>  
>    // This stores the CC participant for the native, null if this class is for a
>    // worker or for a native inheriting from nsISupports (we can get the CC
>    // participant by QI'ing in that case).
> +  nsScriptObjectTracer* mParticipant;

This makes all non-nsISupports refcounted DOM classes that need CC implement a CC trace hook, even if they don't need one (if they're not nsWrapperCached). I'm not sure we want to do that.

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 680775 [details] [diff] [review]
part 2: add PreserveWrapper method for new bindings, use it for weak maps

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

Ah, I see. This buils on that to make having a non-null participant mean implementing nsWrapperCache, which isn't enforced anywhere I think? Not sure what we want to do here, I'll think about it.

Comment 22

[Andrew McCreight [:mccr8]]

Oh, right, that's a good point.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It seems ok to me to require that all objects which supports paris bindings also are able to keep a pointer to their JS-wrapper. It seems to me that the majority of objects don't fall into the class described in comment 4 anyway. So for most objects this is something required to support expandos anyway. Keeping things simple and not worrying about an extra 4 bytes per object seems like a win to me.

We can always worry about saving that pointer for objects of comment 4 class later.

If this means that they'll have to support nsWrapperCache, or if we use some other mechanism specifically for paris-bound objects, I don't have an opinion on.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> Keeping things simple and not worrying about an extra 4 bytes per object seems like a win
> to me.

It's not just 4 bytes per object.  If you want to have a wrapper cache, you have to be CCed.  And then you end up in the CC graph.  And things like WebGLUniformLocation have tons of them floating around so adding them to the CC graph would blow up CC times.

Comment 25

[Andrew McCreight [:mccr8]]

So, the particular problem is non-nsISupports classes that are already cycle collected, but not wrapper cached.

The cases in PreserveWrapper break down like this:

1. nsISupports, does not QI to nsWrapperCache: this must not be a class where we care about preserving the wrapper, so do nothing.

2. nsISupports, does QI to nsWrapperCache: this gives us enough to call nsContentUtils::PreserveWrapper.

3. non-nsISupports, non-cycle-collected: In this case, it must not be wrapper cached, because all wrapper cached things are cycle collected, so this can't be a case where we need to preserve, so do nothing.

4. non-nsISupports, cycle-collected: Finally, the problematic case. Here I make the incorrect assumption that this class must be wrapper cached, and just cast it to nsWrapperCache, letting me preserve the wrapper.

It is only in case 4 where we would need to wrapper cache something that doesn't actually need to be wrapper cached. I could imagine a class that can own other C++ (that may eventually own JS, and thus need to be cycle collected), but one that itself cannot be owned by C++, or can't be re-gotten by JS, so we wouldn't need to wrapper cache it.

Comment 26

[Andrew McCreight [:mccr8]]

We could probably slap in some kind of "is wrapper cached" flag somewhere, but presumably you want to avoid crudding up everything with random bits from various special interests.

Comment 27

[Andrew McCreight [:mccr8]]

Attachment: don't try to support non-nsISupports cycle collected classes

Here's a simpler version that fails for cycle-collected non-nsISupports. It is a pretty trivial change to the previous version. Most of the changes are to adjust to changes that have happened in the bindings code since I wrote the last version, so hopefully it isn't too weird.

Comment 28

[Andrew McCreight [:mccr8]]

Tests aren't working in the way I would expect for that last patch, so something must be wrong.

Comment 29

[Andrew McCreight [:mccr8]]

Attachment: don't try to support non-nsISupports cycle collected classes

It suddenly started working without me changing anything, so I guess I just wasn't rebuilding hard enough. Anyways, I took the opportunity to split out the new tests into a new file. Seems to be working now.

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

Comment 30

[Boris Zbarsky [:bzbarsky]]

> non-nsISupports classes that are already cycle collected, but not wrapper cached.

I would be OK with not supporting those, personally.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> but presumably you want to avoid crudding up everything with random bits from various
> special interests.

I have no problem adding a flag to DOMClass as needed.

Comment 32

[Andrew McCreight [:mccr8]]

Attachment: don't try to support non-nsISupports cycle collected classes

What I propose to do here is to fix the case of nsISupports, as well as non-nsISupports, non-cycle-collected objects here, as that's what we need to fix the regression in 20, then I'll file a followup bug about adding support for non-nsISupports cycle-collected objects, as it seems like that will require some mucking around with the code generator.

Comment 33

[Andrew McCreight [:mccr8]]

Peter: review ping.

Comment 34

[Peter Van der Beken [:peterv]]

Comment on attachment 696379 [details] [diff] [review]
don't try to support non-nsISupports cycle collected classes

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

::: dom/bindings/BindingUtils.cpp
@@ +495,5 @@
> +TryPreserveWrapper(JSObject* obj)
> +{
> +  nsISupports* native;
> +  if (UnwrapDOMObjectToISupports(obj, native)) {
> +    nsCOMPtr<nsISupports> canonical = do_QueryInterface(native);

I don't think you need this, nsContentUtils::PreserveWrapper QIs to nsCycleCollectionISupports.

@@ +497,5 @@
> +  nsISupports* native;
> +  if (UnwrapDOMObjectToISupports(obj, native)) {
> +    nsCOMPtr<nsISupports> canonical = do_QueryInterface(native);
> +    nsWrapperCache* cache = nullptr;
> +    native->QueryInterface(NS_GET_IID(nsWrapperCache), reinterpret_cast<void**>(&cache));

Please use CallQueryInterface.

@@ +506,5 @@
> +  }
> +
> +  // If this DOMClass is not cycle collected, then it isn't wrappercached,
> +  // so it does not need to be preserved. If it is cycle collected, then
> +  // we can't tell if it is wrappercached or not, so we just fail.

s/fail/return false/?

::: dom/bindings/BindingUtils.h
@@ +764,5 @@
> +// to preserve. In the latter case we don't need to preserve the wrapper, because
> +// the object can only be obtained by JS once, or they cannot be meaningfully
> +// owned from the native side.
> +//
> +// This operation will fail only for non-nsISupports cycle-collected objects,

s/fail/return false/?

Comment 35

[Andrew McCreight [:mccr8]]

Thanks for the review.

> I don't think you need this, nsContentUtils::PreserveWrapper QIs to nsCycleCollectionISupports.

Oops, I wasn't even using canonical...

Addressed all review comments:

https://hg.mozilla.org/integration/mozilla-inbound/rev/945ed4328628

I filed bug 829239 for a followup to support the remaining case.

Comment 36

[Andrew McCreight [:mccr8]]

This is a fix for a regression (bug 825053).

Comment 37

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/945ed4328628

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/945ed4328628

Comment 39

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/945ed4328628

Comment 40

[Boris Zbarsky [:bzbarsky]]

That patch applies to Aurora too, right?

Comment 41

[Andrew McCreight [:mccr8]]

Yes, I'll put it up for approval in a day or two assuming there is no fallout.

Comment 42

[Alex Keybl [:akeybl]]

It's not clear to us what the user impact would be if left unfixed. Please clarify.

Comment 43

[Andrew McCreight [:mccr8]]

This is really just the same thing as bug 825053, so you don't really need to track both. Basically, some addons, and possibly some websites, won't work right in some cases.

Comment 44

[bhavana bajaj [:bajaj]]

Tracking this considering we have tracked bug 825053.

Comment 45

[Andrew McCreight [:mccr8]]

Comment on attachment 696379 [details] [diff] [review]
don't try to support non-nsISupports cycle collected classes

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 821606
User impact if declined: some addons and websites could stop working (AdBlockPlus uses weak maps, I think, though maybe not in a way that would be affected here)
Testing completed (on m-c, etc.): it has been on m-c for about a week
Risk to taking this patch (and alternatives if risky): This patch is pretty straightforward, so it should be okay.
String or UUID changes made by this patch: none

Comment 46

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 696379 [details] [diff] [review]
don't try to support non-nsISupports cycle collected classes

Approving low risk (enough) patch for aurora, marking addon-compat to keep on our radar for regressions.

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/162b9b638e45

Comment 48

[Andrew McCreight [:mccr8]]

Yeah, that issue is just the same problem as issue 1567, which is that we don't support events as weak map keys (bug 803844).  I'll comment further over in the other bugzilla bug.