The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 20

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({addon-compat})

Trunk
mozilla21
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20+ fixed, firefox21 fixed)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

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

Updated

5 years ago
Blocks: 777877
(Assignee)

Updated

5 years ago
Assignee: nobody → continuation
Summary: Should wrapper cache imply wrapper-preservable? → Support a wider variety of wrapper-preservable classes as weak map keys
(Assignee)

Comment 1

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

Updated

5 years ago
Blocks: 803844
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....
(Assignee)

Comment 3

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

Comment 5

5 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.
Summary: Support a wider variety of wrapper-preservable classes as weak map keys → Support new DOM bindings objects as weak map keys
(Assignee)

Comment 6

5 years ago
Created attachment 674795 [details] [diff] [review]
Add support for Paris bindings objects as weak map keys, WIP
(Assignee)

Updated

5 years ago
Summary: Support new DOM bindings objects as weak map keys → Support Paris bindings objects as weak map keys
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?
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

5 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.
I think we can have refcounted and cced but not nsISupports natives....
(Assignee)

Comment 12

5 years ago
Created attachment 674892 [details] [diff] [review]
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.
Attachment #674795 - Attachment is obsolete: true
> 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()))
(Assignee)

Comment 14

5 years ago
Created attachment 675204 [details] [diff] [review]
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.
(Assignee)

Comment 15

5 years ago
Created attachment 675231 [details] [diff] [review]
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.
Attachment #674892 - Attachment is obsolete: true
(Assignee)

Comment 16

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

Comment 17

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

Updated

5 years ago
Depends on: 805988
(Assignee)

Updated

4 years ago
Attachment #675204 - Flags: review?(peterv)
(Assignee)

Comment 18

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

Comment 19

4 years ago
Created attachment 680775 [details] [diff] [review]
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.
Attachment #680775 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Attachment #675231 - Attachment is obsolete: true
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 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.
(Assignee)

Comment 22

4 years ago
Oh, right, that's a good point.
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.
> 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.
(Assignee)

Comment 25

4 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.
(Assignee)

Comment 26

4 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.
(Assignee)

Comment 27

4 years ago
Created attachment 696296 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Attachment #675204 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Attachment #680775 - Flags: review?(peterv)
(Assignee)

Comment 28

4 years ago
Tests aren't working in the way I would expect for that last patch, so something must be wrong.
(Assignee)

Comment 29

4 years ago
Created attachment 696321 [details] [diff] [review]
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
Attachment #696296 - Attachment is obsolete: true
> non-nsISupports classes that are already cycle collected, but not wrapper cached.

I would be OK with not supporting those, personally.
> 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.
(Assignee)

Comment 32

4 years ago
Created attachment 696379 [details] [diff] [review]
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.
Attachment #675204 - Attachment is obsolete: true
Attachment #680775 - Attachment is obsolete: true
Attachment #696321 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #696379 - Flags: review?(peterv)
(Assignee)

Updated

4 years ago
Blocks: 825053
(Assignee)

Comment 33

4 years ago
Peter: review ping.
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/?
Attachment #696379 - Flags: review?(peterv) → review+
(Assignee)

Comment 35

4 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.
Summary: Support Paris bindings objects as weak map keys → Support Paris bindings objects that are either nsISupports or non-cycle-collected as weak map keys
tracking-firefox20: --- → ?
(Assignee)

Comment 36

4 years ago
This is a fix for a regression (bug 825053).
https://hg.mozilla.org/mozilla-central/rev/945ed4328628
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/945ed4328628
https://hg.mozilla.org/mozilla-central/rev/945ed4328628
That patch applies to Aurora too, right?
(Assignee)

Comment 41

4 years ago
Yes, I'll put it up for approval in a day or two assuming there is no fallout.
Depends on: 829798
It's not clear to us what the user impact would be if left unfixed. Please clarify.
(Assignee)

Comment 43

4 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.
Tracking this considering we have tracked bug 825053.
status-firefox20: --- → affected
tracking-firefox20: ? → +
(Assignee)

Comment 45

4 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
Attachment #696379 - Flags: approval-mozilla-aurora?
Keywords: addon-compat
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.
Attachment #696379 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/162b9b638e45
status-firefox20: affected → fixed
status-firefox21: --- → fixed

Updated

4 years ago
(Assignee)

Comment 48

4 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.
You need to log in before you can comment on or make changes to this bug.