Andrew McCreight [:mccr8] Assignee
	Description • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 1 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 12 years ago

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....

	Andrew McCreight [:mccr8] Assignee
	Comment 3 • 12 years ago

(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.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 12 years ago

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....

	Andrew McCreight [:mccr8] Assignee
	Comment 5 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 12 years ago

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?

	Andrew McCreight [:mccr8] Assignee
	Comment 8 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 9 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 10 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 12 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 12 years ago

> 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()))

	Andrew McCreight [:mccr8] Assignee
	Comment 14 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 15 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 16 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 17 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 18 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 19 • 12 years ago

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.

	Peter Van der Beken [:peterv]
	Comment 20 • 12 years ago

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.

	Peter Van der Beken [:peterv]
	Comment 21 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 22 • 12 years ago

Oh, right, that's a good point.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 23 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 24 • 12 years ago

> 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.

	Andrew McCreight [:mccr8] Assignee
	Comment 25 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 26 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 27 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 28 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 29 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 30 • 12 years ago

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

I would be OK with not supporting those, personally.

	Boris Zbarsky [:bzbarsky]
	Comment 31 • 12 years ago

> 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.

	Andrew McCreight [:mccr8] Assignee
	Comment 32 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 33 • 12 years ago

Peter: review ping.

	Peter Van der Beken [:peterv]
	Comment 34 • 12 years ago

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/?

	Andrew McCreight [:mccr8] Assignee
	Comment 35 • 12 years ago

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.

	Andrew McCreight [:mccr8] Assignee
	Comment 36 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 37 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 38 • 12 years ago

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

	Ed Morley [:emorley]
	Comment 39 • 12 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 40 • 12 years ago

That patch applies to Aurora too, right?

	Andrew McCreight [:mccr8] Assignee
	Comment 41 • 12 years ago

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

	Alex Keybl [:akeybl]
	Comment 42 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 43 • 12 years ago

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.

	bhavana bajaj [:bajaj]
	Comment 44 • 12 years ago

Tracking this considering we have tracked bug 825053.

	Andrew McCreight [:mccr8] Assignee
	Comment 45 • 12 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 46 • 12 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 12 years ago

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

	Andrew McCreight [:mccr8] Assignee
	Comment 48 • 12 years ago

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.