673468 - Wrapped JS objects used as keys can disappear from WeakMaps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

Follow up to Bug 655297.  Any wrapped Javascript objects (cross compartment wrappers and maybe others) that are subject to being thrown away when they are unused and can be rematerialized later will have their bindings incorrectly thrown away during garbage collection if they are used as keys in WeakMaps.

The problem is that if a wrapper isn't referred to explicitly, and it contains no references to anything else, then the GC doesn't mark it.  If such a wrapper is used as a key in a WeakMap, the WeakMap will think the key is unused and throw away the binding.  If the wrapper is later rematerialized, it won't find the binding, even though the user is expecting that.

The solution used for XPCWrappedNatives is to mark keys that are wrappers.  That approach should work here, except that instead of adding a new interface, we can probably just call unwrap() to find out if it is a wrapper, and whatever we need to know about the underlying object (marked, unmarked, same compartment, etc.).

Comment 1

[Andrew McCreight [:mccr8]]

Bill, how hard do you think this would be?

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

At first glance it seems like this should be pretty easy. Instead of having an isWrappedNative in the the JSClass, we could have something like shouldPreserveInWeakMap. The proxy class would forward this call to its handler, and the handler for the different security wrapper would check the mark bit for the object being wrapped.

Comment 3

[Andrew McCreight [:mccr8]]

I don't think isWrappedNative can be changed, it is being used in XPConnect.

Comment 4

[Andrew McCreight [:mccr8]]

Alexandre, I'm curious if you've actually hit this problem in practice?

Comment 5

[Alexandre Poirot [:ochameau]]

Attachment: Jetpack unit test

We start hitting this bug in Jetpack codebase :/
Here is a jetpack unit test that resume this whole issue for us.
We are using one Sandbox per module. So that we have wrappers between each module.
*But*, thanks to bug 677294 and the `sameGroupAs` sandbox option, all these sandboxes end up sharing the same compartment, so that we do not have wrappers anymore on FF version that has this new feature landed (Nightly at least).

So we are hitting this bug through Namespace module:
  https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/docs/namespace.md
(a handy module that allows to hide some private API data)
And thanks to bug 677294, we are not hitting this issue anymore on Nightly!
But we need to handle this issue in FF10/11 either by not using Namespace module and Weakmaps until Jetpack minimal Firefox version support is FF12 (1.9 in May) or fix this somehow?

This testcase is going to fail on FF9 and FF10.
You can make it fail on FF12 too by commenting the `{ sameGroupAs: {} }` thing (will use different compartments).
Or make it pass by commenting the `forceGC()` line.

There is still something I do not understand. Someone on Mac wasn't able to reproduce this issue?! I'm able to reproduce it on Window and Linux.

Comment 6

[Andrew McCreight [:mccr8]]

Well, this is not going to be fixed for 10, and probably not for 11.

That's odd that is works on Mac.  I'm not sure why that would be.

Comment 7

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

For the record, I tested on Mac on Lion for Stable, Aurora and Nightly and it fails on all of them with `sameGroupAs` removed.

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Andrew is there any workaround, you could think of that we could employ for the time being ? I wonder if there is any API for unwrapping XPCWrappedNatives or a way to identify if object is a XPCWrappedNative ?

Comment 9

[Andrew McCreight [:mccr8]]

Support for XPCWrappedNatives was added in bug 668855 and bug 680937.  Or should have been.  This is about a different type of wrapper.  But I don't really know enough about them to know how to work around it, sorry.  Maybe somebody in the #jsapi IRC channel can help.

Comment 10

[Dave Townsend [:mossop]]

This seems like something liable to bite a lot of extensions. All restartless extensions run in their own compartment so if they try to use anything outside of that as a WeakMap key then presumably they are going to hit this. Can we get someone assigned to work on this?

Comment 11

[Gabor Krizsanits (INACTIVE)]

Andrew, is there anyone currently working on this bug? If not I can give it a try...

Comment 12

[Andrew McCreight [:mccr8]]

Not as far as I know.  That would be great.

Comment 13

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Support for XPCWrappedNatives was added in bug 668855 and bug 680937.  Or
> should have been.  This is about a different type of wrapper.  But I don't
> really know enough about them to know how to work around it, sorry.  Maybe
> somebody in the #jsapi IRC channel can help.

So the way I see it there is not much use of preserving security wrappers here the same way as for it's done for XPCWrappedNatives. First security wrappers can wrap a simple pure js object in which case the same approach (nsWrapperCache) is not really something we can apply. Second, I don't see any problem with unwrapping the object from the security wrappers and then using the real object as the key for the hashmap, since we don't use any internals from the object that would expose any security vulnerability within the map. So I propose a simple unwrap before we put a new key object into the map or before we look up a key. Andrew: am I overlooking something here, or do you think my approach could work?

Comment 14

[Andrew McCreight [:mccr8]]

That should work from the perspective of weak maps, but I don't know if that's okay to do or not.  I don't know anything about security wrappers.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I don't know much about security wrappers either, but we would have to be careful about compartments. We cannot use an object as the key if it's from another compartment. So if unwrapping a security wrapper can yield an object from a different compartment, then there would be a problem.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bill McCloskey (:billm) from comment #15)
> I don't know much about security wrappers either, but we would have to be
> careful about compartments. We cannot use an object as the key if it's from
> another compartment. So if unwrapping a security wrapper can yield an object
> from a different compartment, then there would be a problem.

That's exactly the problem... Most security wrappers are cross compartment wrappers and therefore unwrapping them returns an object in another compartment.

Comment 17

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Is there any reason why we can't use the original approach that was used for XPCWrappedNatives? That is, when we iterate over the entries of the weakmap, we check if the key is a wrapper. If it is, we check if the object it wraps is marked (using IsAboutToBeFinalized, which is safe across compartments). If it is marked, we mark the value *and* the key (i.e., the wrapper).

The idea I proposed in comment 2 for checking whether something should be preserved in this way will still work--we just need to use a separate entry point, rather than replacing isWrappedNative.

Comment 18

[Jim Blandy :jimb]

JS-level WeakMap certainly shouldn't ever have cross-compartment references as keys, but the C++-level WeakMap, as used by Debugger, often has cross-compartment references as keys.

Comment 19

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #16)

> That's exactly the problem... Most security wrappers are cross compartment
> wrappers and therefore unwrapping them returns an object in another
> compartment.

And why is that a problem exactly? As far as I know the weakmap has a get, set, has, delete api, none of which returns a key. So the fact that we are using an object from another compartment as a key internally does not expose an object from another compartment at JS-level. Unless internally the hash map access something on the key object, which would surprise me based on Comment 18 I don't see why it could not work.

Bill: I will try your approach in the meanwhile, it just takes some study for me around the GC to get started with it.

Comment 20

[Gabor Krizsanits (INACTIVE)]

Or even if we expose the keys (nondeterministicGetWeakMapKeys ?) can't we rewrap the keys objects there if needed?

Comment 21

[Mark S. Miller]

> Or even if we expose the keys (nondeterministicGetWeakMapKeys ?) can't we rewrap the keys objects there if needed?

Why expose the keys? Do you have a use case?

I continue to think that Weak References (e.g., http://wiki.ecmascript.org/doku.php?id=strawman:weak_references ) should be kept distinct from WeakMaps. Only Weak References should make it possible to sense GC decisions.

Comment 22

[Andrew McCreight [:mccr8]]

(In reply to Mark S. Miller from comment #21)
> > Or even if we expose the keys (nondeterministicGetWeakMapKeys ?) can't we rewrap the keys objects there if needed?
> 
> Why expose the keys? Do you have a use case?

The keys are exposed only to browser code, not the web, to enable testing that weak map entries are properly cleaned up.

If that function is a problem, we could change it to return the values.

Comment 23

[Mark S. Miller]

Privileged testing is indeed a good use case. I don't think returning values make anything less problematic than returning keys, and returning keys has more utility than returning values, so no need to change plans.

Thanks for clarifying.

Comment 24

[Andrew McCreight [:mccr8]]

(In reply to Mark S. Miller from comment #23)
> I don't think returning values
> make anything less problematic than returning keys, and returning keys has
> more utility than returning values, so no need to change plans.

Oh, sorry, I just meant from the purpose of this immediate bug, where there was some possibility of exposing keys from another compartment.  But that's probably a bad idea either way.

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: unwrapping the keys

So I talked to Bill on irc and he thinks it should be fine to unwrap the keys. Tests worked so far, I pushed it to try as well just in case. Andrew, I added you as the reviewer please feel free to add anyone else you think should review it. And if anyone else ofc has something against this approach please let me know. What I like about this approach that it's very simple. What not, is that I don't know how to assure that in the future no one should ever add and api that exposes a key without wrapping it, and as Bill pointed out I'm breaking an invariant here with this approach.

Comment 26

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=dcbe4be993ae

Comment 27

[Andrew McCreight [:mccr8]]

Comment on attachment 596012 [details] [diff] [review]
unwrapping the keys

Review of attachment 596012 [details] [diff] [review]:
-----------------------------------------------------------------

I think you should turn the whole NonNullObject, UnwrapObject sequence into a function like GetKeyArg.  Return null if it fails, then check and return false if that fails in the get/set/etc operations.  Then you can put a comment on that function talking about the cross-compartment weirdness.  It might be a good idea to put a comment in there at the point of rewrapping.

I think it is okay that any additional API that exposes keys will require fixing, as it is a basic principle of weak maps that keys aren't exposed, and we are only violating that for testing.

Bill or somebody who understands wrappers more than me should look at this, too.  The keys are also examined in the markIteratively, sweep and traceMappings functions in jsweakmap.h.  The cycle collector doesn't care about compartments, so I don't imagine there should be a problem for traceMappings, but Bill should look over the uses of it for the GC and see if that looks okay.

But anyways, looks good to me as far as I can tell.  Thanks for fixing this.

Comment 28

[Gabor Krizsanits (INACTIVE)]

Attachment: added GetKeyArg and comments

(In reply to Andrew McCreight [:mccr8] from comment #27)
> 
> I think you should turn the whole NonNullObject, UnwrapObject sequence into
> a function like GetKeyArg.  Return null if it fails, then check and return
> false if that fails in the get/set/etc operations.  Then you can put a
> comment on that function talking about the cross-compartment weirdness.  It
> might be a good idea to put a comment in there at the point of rewrapping.

So I made the changes you proposed, let me know weather you like it and thanks for the quick review.

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 596959 [details] [diff] [review]
added GetKeyArg and comments

Review of attachment 596959 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for making the changes!

nit: you need to indent 4 spaces everywhere instead of 2.

::: js/src/jsweakmap.cpp
@@ +122,5 @@
>  }
>  
> +// If the key is from another compartment, and we store the wrapper as the key
> +// the wrapper might be GC-ed since it is not strong referenced (Bug 673468). 
> +// To avoid this we always use the unwrapped object as key instead its security

nit: "as key instead its security" should be "as the key instead of its security".

@@ +123,5 @@
>  
> +// If the key is from another compartment, and we store the wrapper as the key
> +// the wrapper might be GC-ed since it is not strong referenced (Bug 673468). 
> +// To avoid this we always use the unwrapped object as key instead its security
> +// wrapper. Which also means if the keys are ever exposed they must be re-wrapped

nit: "Which also means" -> "This also means that" 
maybe

@@ +126,5 @@
> +// To avoid this we always use the unwrapped object as key instead its security
> +// wrapper. Which also means if the keys are ever exposed they must be re-wrapped
> +// (see: JS_NondeterministicGetWeakMapKeys).
> +static JSObject * 
> +GetKeyArg( JSContext *cx, CallArgs &args ) 

nit: no spaces before first arg or after last arg.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 596959 [details] [diff] [review]
added GetKeyArg and comments

Review of attachment 596959 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thanks!

::: js/src/jsweakmap.cpp
@@ +126,5 @@
> +// To avoid this we always use the unwrapped object as key instead its security
> +// wrapper. Which also means if the keys are ever exposed they must be re-wrapped
> +// (see: JS_NondeterministicGetWeakMapKeys).
> +static JSObject * 
> +GetKeyArg( JSContext *cx, CallArgs &args ) 

Given that this is now the only user of NonNullObject, maybe you could combine them.

@@ +296,5 @@
>          return false;
>      ObjectValueMap *map = GetObjectMap(obj);
>      if (map) {
>          for (ObjectValueMap::Range r = map->nondeterministicAll(); !r.empty(); r.popFront()) {
> +            JSObject* key = r.front().key;

Should be "JSObject *key =".

Comment 31

[Gabor Krizsanits (INACTIVE)]

Attachment: cleaned up version

I cleaned up the patch and I merged the GetKeyArg with NonNullObject, and I also moved the comment right before the unwrap call. If you guys find some more nits, just let me know. Else I will put a check-in request on this version. 

https://tbpl.mozilla.org/?tree=Try&rev=bf272c20621e

Comment 32

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5e48ff8f8d5a

Comment 33

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e48ff8f8d5a

Comment 34

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: References are not null

null is primitive, null references don't exist, reviewers should catch this

Comment 35

[Gabor Krizsanits (INACTIVE)]

(In reply to Ms2ger from comment #34)
> Created attachment 598956 [details] [diff] [review]
> References are not null
> 
> null is primitive, null references don't exist, reviewers should catch this

Whoops, that's true. That code is not something I introduced, but I overlooked it as well. It does not make any sense to turn a reference to a pointer and then check against null. Since it's not very harmful one, just makes no sense, I suggest to add a follow up patch that fixes only that line. Ms2ger do you have that patch just attached a null patch by accident or shall I make the follow up patch?

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: References are not null

qref? What's that?

Comment 37

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 598962 [details] [diff] [review]
References are not null

Review of attachment 598962 [details] [diff] [review]:
-----------------------------------------------------------------

It's not my call... edmorley : I think it's easier to apply this patch on top of mine, so you don't have to revert the previous one on inbound and central. And it fixes something that was not introduced by the first patch anyway. But if you prefer me to resend my original patch merged with this one just let me know.

Comment 38

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Ms2ger from comment #34)
> Created attachment 598956 [details] [diff] [review]
> References are not null
> 
> null is primitive, null references don't exist, reviewers should catch this

The patch that Andrew and I reviewed required a null check to handle the !isPrimitive case. It's true that there was an extra null check in the patch that eventually got checked in. But that's hardly the end of the world, I think.

Comment 39

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/8134fedf2a8f

Comment 40

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/72d5c85096fe6e671ea6c2e0a8525ae48f5b75e5
Removing workarounds for bug 673468