1351501 - Can't use non-nsISupports cycle collected WebIDL objects as WeakMap keys

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), defect, P2)

Description

[ravi.jayaramappa]

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

Comment 2

[Andrew McCreight [:mccr8]]

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.

Comment 3

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Andrew McCreight [:mccr8]]

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

Comment 8

[Andrew McCreight [:mccr8]]

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 9

[Andrew McCreight [:mccr8]]

Attachment: Preserve wrappers for non-nsISupport weak map keys.

Comment 10

[Andrew McCreight [:mccr8]]

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 11

[Boris Zbarsky [:bzbarsky]]

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.

Comment 12

[Andrew McCreight [:mccr8]]

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.

Comment 14

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351501, part 1 - Handlify TryPreserveWrapper

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

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: Bug 1351501, part 2 - Preserve wrappers for non-nsISupports cycle collected weak map keys

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 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9010060 [details]
Bug 1351501, part 1 - Handlify TryPreserveWrapper

Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 20

[u626008]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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.