Bug 673468 - Wrapped JS objects used as keys can disappear from WeakMaps. r=mccr8,billm
authorGabor Krizsanits <gkrizsanits@mozilla.com>
Fri, 17 Feb 2012 13:22:26 +0100
changeset 87095 5e48ff8f8d5a59e360ba5e1533ab1c12949aac67
parent 87094 bfc937247f3c7b70ce8dfcbee41d8726b10e1980
child 87096 67b6c09fb30fbc5c234ae5222922d5d3aeca7165
push id22080
push userbmo@edmorley.co.uk
push dateSat, 18 Feb 2012 00:21:15 +0000
treeherdermozilla-central@e001b5eda618 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8, billm
bugs673468
milestone13.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 673468 - Wrapped JS objects used as keys can disappear from WeakMaps. r=mccr8,billm
js/src/jsweakmap.cpp
--- a/js/src/jsweakmap.cpp
+++ b/js/src/jsweakmap.cpp
@@ -107,23 +107,33 @@ typedef WeakMap<HeapPtr<JSObject>, HeapV
 static ObjectValueMap *
 GetObjectMap(JSObject *obj)
 {
     JS_ASSERT(obj->isWeakMap());
     return (ObjectValueMap *)obj->getPrivate();
 }
 
 static JSObject *
-NonNullObject(JSContext *cx, Value *vp)
+GetKeyArg(JSContext *cx, CallArgs &args) 
 {
+    Value *vp = &args[0];
     if (vp->isPrimitive()) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
         return NULL;
     }
-    return &vp->toObject();
+    JSObject *key = &vp->toObject();
+    if (!key)
+        return NULL;
+
+    // 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 the key instead of its
+    // security wrapper. This also means that if the keys are ever exposed they must
+    // be re-wrapped (see: JS_NondeterministicGetWeakMapKeys).
+    return JS_UnwrapObject(key);
 }
 
 static JSBool
 WeakMap_has(JSContext *cx, uintN argc, Value *vp)
 {
     CallArgs args = CallArgsFromVp(argc, vp);
 
     bool ok;
@@ -131,19 +141,20 @@ WeakMap_has(JSContext *cx, uintN argc, V
     if (!obj)
         return ok;
 
     if (args.length() < 1) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
                              "WeakMap.has", "0", "s");
         return false;
     }
-    JSObject *key = NonNullObject(cx, &args[0]);
+    JSObject *key = GetKeyArg(cx, args);
     if (!key)
         return false;
+
     ObjectValueMap *map = GetObjectMap(obj);
     if (map) {
         ObjectValueMap::Ptr ptr = map->lookup(key);
         if (ptr) {
             args.rval() = BooleanValue(true);
             return true;
         }
     }
@@ -162,19 +173,20 @@ WeakMap_get(JSContext *cx, uintN argc, V
     if (!obj)
         return ok;
 
     if (args.length() < 1) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
                              "WeakMap.get", "0", "s");
         return false;
     }
-    JSObject *key = NonNullObject(cx, &args[0]);
+    JSObject *key = GetKeyArg(cx, args);
     if (!key)
         return false;
+
     ObjectValueMap *map = GetObjectMap(obj);
     if (map) {
         ObjectValueMap::Ptr ptr = map->lookup(key);
         if (ptr) {
             args.rval() = ptr->value;
             return true;
         }
     }
@@ -193,19 +205,20 @@ WeakMap_delete(JSContext *cx, uintN argc
     if (!obj)
         return ok;
 
     if (args.length() < 1) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
                              "WeakMap.delete", "0", "s");
         return false;
     }
-    JSObject *key = NonNullObject(cx, &args[0]);
+    JSObject *key = GetKeyArg(cx, args);
     if (!key)
         return false;
+    
     ObjectValueMap *map = GetObjectMap(obj);
     if (map) {
         ObjectValueMap::Ptr ptr = map->lookup(key);
         if (ptr) {
             map->remove(ptr);
             args.rval() = BooleanValue(true);
             return true;
         }
@@ -225,19 +238,20 @@ WeakMap_set(JSContext *cx, uintN argc, V
     if (!obj)
         return ok;
 
     if (args.length() < 1) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
                              "WeakMap.set", "0", "s");
         return false;
     }
-    JSObject *key = NonNullObject(cx, &args[0]);
+    JSObject *key = GetKeyArg(cx, args);
     if (!key)
         return false;
+    
     Value value = (args.length() > 1) ? args[1] : UndefinedValue();
 
     ObjectValueMap *map = GetObjectMap(obj);
     if (!map) {
         map = cx->new_<ObjectValueMap>(cx, obj);
         if (!map->init()) {
             cx->delete_(map);
             goto out_of_memory;
@@ -272,17 +286,22 @@ JS_NondeterministicGetWeakMapKeys(JSCont
         return true;
     }
     JSObject *arr = NewDenseEmptyArray(cx);
     if (!arr)
         return false;
     ObjectValueMap *map = GetObjectMap(obj);
     if (map) {
         for (ObjectValueMap::Range r = map->nondeterministicAll(); !r.empty(); r.popFront()) {
-            if (!js_NewbornArrayPush(cx, arr, ObjectValue(*r.front().key)))
+            JSObject *key = r.front().key;
+            // Re-wrapping the key (see comment of GetKeyArg)
+            if (!JS_WrapObject(cx, &key))
+                return false;
+
+            if (!js_NewbornArrayPush(cx, arr, ObjectValue(*key)))
                 return false;
         }
     }
     *ret = arr;
     return true;
 }
 
 static void