User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.1 Safari/603.1.30 Steps to reproduce: Try the following snippet of code in the console var x= new WebKitCSSMatrix(); var map = new WeakMap(); map.set(x, "test"); // TypeError: cannot use the given object as a weak map key Actual results: TypeError: cannot use the given object as a weak map key Expected results: Should have successfully added an entry in the weak map

From bug 1391116: (In reply to Peter Van der Beken [:peterv] from comment #3) > It's on the new bindings, it's wrappercached but it's not derived from > nsISupports. As explained in TryPreserveWrapper, we don't have a way to know > if it's wrappercached, because it's not derived from nsISupports. So we > can't preserve the wrapper and the weakmap code relies on preserving the > wrapper. > > We could fix this by having a generated hook that casts the native object to > nsWrapperCache.

It might also be possible to add a new method onto nsCycleCollectionParticipant that preserves the wrapper if possible. Some kind of SFINAE stuff could deal with wrapper cached CC'd vs non wrapper cached CC'd, assuming the latter exists.

There are enough dupes of this that we should try to figure out a solution.

The _addProperty hook actually already does wrapper preservation, and is only defined for wrapper cached things, so as a first hacky step I'll try using that. Using that is a little questionable because the hook takes four arguments but only uses one, so the code will depend on the hook not doing anything else. If need be, we could generate a separate hook to preserve wrappers. I need to remember to write at least a basic test for this, in one of the existing weakmap tests.

Comment on attachment 9009750 [details] [diff] [review] Preserve wrappers for non-nsISupport weak map keys. I haven't run this through try yet, so it might not be 100% right, but Boris, what do you think about this? I noticed that the addProperty hook calls PreserveWrapper and does nothing else, so I just call that. This is obviously a horrible hack because it could change later, but maybe that's better than creating a new hook dedicated to preserving the wrapper? (I'm concerned about that in terms of using more memory for the JS classes, and maybe the little bit of additional complexity.) Maybe I could throw a comment in near where the addProperty hook is generated to talk about this additional use? This patch applies on top of another patch to handlify this method. I can upload that patch if you want to try it yourself for some reason.

Comment on attachment 9009750 [details] [diff] [review] Preserve wrappers for non-nsISupport weak map keys. > weak map entry. If the C++ object can be access again later from JS, "can be accessed" >+ if (!domClass) { How can this happen given the IsDOMObject() assert above? >+ JSAddPropertyOp addProperty = domClass->ToJSClass()->getAddProperty(); It's worth documenting why we still need the "QI to nsWrapperCache" bit above. The reason we do is that if the object is a DOM proxy then we should not be calling its addProperty hook here. Luckily, all DOM proxies are currently nsISupports. But this is pretty questionable... We should at the very least add (diagnostic? release?) asserts that we are not looking at a proxy here. We should also assert that addProperty is non-null if and only if domClass->mParticipant is non-null, right? I'm probably OK with this general approach if we add some comments in the addProperty codegen about this weird caller and how it might need to be fixed if we ever change what addProperty does.

Thanks for the feedback. (In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #11) > How can this happen given the IsDOMObject() assert above? I was just copying the existing code, but it looks like that predates the assert so maybe it was just left over cruft. (That code dates to 2013...) I'll remove that. > It's worth documenting why we still need the "QI to nsWrapperCache" bit > above. The reason we do is that if the object is a DOM proxy then we should > not be calling its addProperty hook here. Luckily, all DOM proxies are > currently nsISupports. But this is pretty questionable... Ah, interesting. I'll add a comment to that effect. > We should at the very least add (diagnostic? release?) asserts that we are > not looking at a proxy here. Sounds like a good idea. I'll make it release because I can't imagine this code is perf sensitive, and weak maps are not the most heavily used feature. > We should also assert that addProperty is non-null if and only if > domClass->mParticipant is non-null, right? Make sense. > I'm probably OK with this general approach if we add some comments in the > addProperty codegen about this weird caller and how it might need to be > fixed if we ever change what addProperty does. Sounds good.

The patch in the next part will need a handle to the object in TryPreserveWrapper.

A C++ object that is exposed to JS can have its reflector used as a key in a weak map. Because a weak map does not keep its keys alive, this means that the reflector can be discarded if it has no other references aside from the C++ object, which will in turn remove its weak map entry. If the C++ object can be accessed again later from JS, it will get a new reflector which will have no weak map entry. This is bad because it means some internal implementation detail has resulted in data loss that is visible to JS. (Side note: this is also an issue for cross compartment wrappers, which is handled by another mechanism.) To fix this, we can preserve the wrapper of any DOM reflector used as a weak map key. This ensures that the reflector and its C++ object have the same lifetime. If a WebIDL object is not wrapper cached, that means that it cannot be accessed via C++, so we don't need to preserve the wrapper. This is currently implemented for nsISupports classes, but not other classes. For non-nsISupports classes, it would throw an error rather than silently fail. My patch adds support for non-nsISupports cycle collected objects. It turns out that the existing addProperty hook just does wrapper preservation, so we just call it for cycle collected classes. This does mean that if addProperty changes in the future to do something else, this code will need to be changed. I verified that this test fails if TryPreserveWrapper is changed to do nothing besides return true in the non-nsISuports case. Depends on D6197

Comment on attachment 9010060 [details] Bug 1351501, part 1 - Handlify TryPreserveWrapper Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Comment on attachment 9010061 [details] Bug 1351501, part 2 - Preserve wrappers for non-nsISupports cycle collected weak map keys Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Pushed by amccreight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ab2856e070b9 part 1 - Handlify TryPreserveWrapper r=bzbarsky https://hg.mozilla.org/integration/autoland/rev/ef927b086254 part 2 - Preserve wrappers for non-nsISupports cycle collected weak map keys r=bzbarsky

https://hg.mozilla.org/mozilla-central/rev/ab2856e070b9 https://hg.mozilla.org/mozilla-central/rev/ef927b086254

Do we know when this will be released into main branch of Firefox? Thank you.

Assuming there are no issues that come up, this will ship in Firefox 64. https://wiki.mozilla.org/Release_Management/Calendar has the planned ship dates for releases going forward; Firefox 64 is planned for November 11, 2018.