Closed
Bug 777385
Opened 12 years ago
Closed 12 years ago
Support Paris bindings objects that are either nsISupports or non-cycle-collected as weak map keys
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Keywords: addon-compat)
Attachments
(1 file, 7 obsolete files)
(deleted),
patch
|
peterv
:
review+
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•12 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•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.
![]() |
||
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....
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.
![]() |
||
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....
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.
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Summary: Support new DOM bindings objects as weak map keys → Support Paris bindings objects as weak map keys
![]() |
||
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?
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.
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;
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.
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.
Attachment #674795 -
Attachment is obsolete: true
![]() |
||
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()))
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.
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.
Attachment #674892 -
Attachment is obsolete: true
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.
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.
Assignee | ||
Updated•12 years ago
|
Attachment #675204 -
Flags: review?(peterv)
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
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.
Attachment #680775 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Attachment #675231 -
Attachment is obsolete: true
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.
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.
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•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.
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.
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.
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.
Assignee | ||
Updated•12 years ago
|
Attachment #675204 -
Flags: review?(peterv)
Assignee | ||
Updated•12 years ago
|
Attachment #680775 -
Flags: review?(peterv)
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.
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
Attachment #696296 -
Attachment is obsolete: true
![]() |
||
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.
![]() |
||
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.
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.
Attachment #675204 -
Attachment is obsolete: true
Attachment #680775 -
Attachment is obsolete: true
Attachment #696321 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #696379 -
Flags: review?(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/?
Attachment #696379 -
Flags: review?(peterv) → review+
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.
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
![]() |
||
Updated•12 years ago
|
tracking-firefox20:
--- → ?
Comment 37•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/945ed4328628
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Assignee | ||
Comment 41•12 years ago
|
||
Yes, I'll put it up for approval in a day or two assuming there is no fallout.
Comment 42•12 years ago
|
||
It's not clear to us what the user impact would be if left unfixed. Please clarify.
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.
Comment 44•12 years ago
|
||
Tracking this considering we have tracked bug 825053.
status-firefox20:
--- → affected
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
Attachment #696379 -
Flags: approval-mozilla-aurora?
Updated•12 years ago
|
Keywords: addon-compat
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.
Attachment #696379 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 47•12 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/162b9b638e45
status-firefox21:
--- → fixed
Updated•12 years ago
|
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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•