Bug 798678 - Refactor wrapper preservation for weakmaps (r=mccr8)
authorBill McCloskey <wmccloskey@mozilla.com>
Mon, 08 Oct 2012 18:22:47 -0700
changeset 110758 4ca5990b24d5654294d83ae25a428e58282cef5d
parent 110757 2a96aeb11ad44ab6ac55c32a2bf177ad89dabb32
child 110759 1e5bf398fc887b518462e789526ee4159fdaf632
push id93
push usernmatsakis@mozilla.com
push dateWed, 31 Oct 2012 21:26:57 +0000
reviewersmccr8
bugs798678
milestone19.0a1
Bug 798678 - Refactor wrapper preservation for weakmaps (r=mccr8)
js/src/jit-test/tests/basic/bug673468.js
js/src/jsapi.h
js/src/jsclass.h
js/src/jsproxy.cpp
js/src/jsproxy.h
js/src/jsweakmap.cpp
js/src/jsweakmap.h
js/xpconnect/tests/chrome/Makefile.in
js/xpconnect/tests/chrome/test_weakmap_keys_preserved.xul
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug673468.js
@@ -0,0 +1,8 @@
+var g = newGlobal();
+var k = g.eval('var u = new Object(); u');
+var m = new WeakMap();
+m.set(k, {});
+k = null;
+gc();
+k = g.eval('u');
+assertEq(m.has(k), true);
--- a/js/src/jsapi.h
+++ b/js/src/jsapi.h
@@ -1878,16 +1878,19 @@ typedef void
  * a string describing the reference traced with JS_CallTracer.
  */
 typedef void
 (* JSTraceNamePrinter)(JSTracer *trc, char *buf, size_t bufsize);
 
 typedef JSBool
 (* JSEqualityOp)(JSContext *cx, JSHandleObject obj, JSHandleValue v, JSBool *bp);
 
+typedef JSRawObject
+(* JSWeakmapKeyDelegateOp)(JSRawObject obj);
+
 /*
  * Typedef for native functions called by the JS VM.
  *
  * See jsapi.h, the JS_CALLEE, JS_THIS, etc. macros.
  */
 
 typedef JSBool
 (* JSNative)(JSContext *cx, unsigned argc, jsval *vp);
--- a/js/src/jsclass.h
+++ b/js/src/jsclass.h
@@ -250,19 +250,32 @@ struct ClassExtension
     JSIteratorOp        iteratorObject;
     void               *unused;
 
     /*
      * isWrappedNative is true only if the class is an XPCWrappedNative.
      * WeakMaps use this to override the wrapper disposal optimization.
      */
     bool                isWrappedNative;
+
+    /*
+     * If an object is used as a key in a weakmap, it may be desirable for the
+     * garbage collector to keep that object around longer than it otherwise
+     * would. A common case is when the key is a wrapper around an object in
+     * another compartment, and we want to avoid collecting the wrapper (and
+     * removing the weakmap entry) as long as the wrapped object is alive. In
+     * that case, the wrapped object is returned by the wrapper's
+     * weakmapKeyDelegateOp hook. As long as the wrapper is used as a weakmap
+     * key, it will not be collected (and remain in the weakmap) until the
+     * wrapped object is collected.
+     */
+    JSWeakmapKeyDelegateOp weakmapKeyDelegateOp;
 };
 
-#define JS_NULL_CLASS_EXT   {NULL,NULL,NULL,NULL,NULL,false}
+#define JS_NULL_CLASS_EXT   {NULL,NULL,NULL,NULL,NULL,false,NULL}
 
 struct ObjectOps
 {
     LookupGenericOp     lookupGeneric;
     LookupPropOp        lookupProperty;
     LookupElementOp     lookupElement;
     LookupSpecialOp     lookupSpecial;
     DefineGenericOp     defineGeneric;
--- a/js/src/jsproxy.cpp
+++ b/js/src/jsproxy.cpp
@@ -348,16 +348,22 @@ BaseProxyHandler::objectClassIs(JSObject
     return false;
 }
 
 void
 BaseProxyHandler::finalize(JSFreeOp *fop, JSObject *proxy)
 {
 }
 
+JSObject *
+BaseProxyHandler::weakmapKeyDelegate(JSObject *proxy)
+{
+    return NULL;
+}
+
 bool
 BaseProxyHandler::getPrototypeOf(JSContext *cx, JSObject *proxy, JSObject **proto)
 {
     // The default implementation here just uses proto of the proxy object.
     *proto = proxy->getTaggedProto().toObjectOrNull();
     return true;
 }
 
@@ -544,16 +550,22 @@ IndirectProxyHandler::iteratorNext(JSCon
         *vp = cx->iterValue;
         cx->iterValue = UndefinedValue();
     } else {
         *vp = MagicValue(JS_NO_ITER_VALUE);
     }
     return true;
 }
 
+JSObject *
+IndirectProxyHandler::weakmapKeyDelegate(JSObject *proxy)
+{
+    return UnwrapObject(proxy);
+}
+
 DirectProxyHandler::DirectProxyHandler(void *family)
   : IndirectProxyHandler(family)
 {
 }
 
 bool
 DirectProxyHandler::has(JSContext *cx, JSObject *proxy, jsid id, bool *bp)
 {
@@ -2852,29 +2864,35 @@ proxy_TraceFunction(JSTracer *trc, RawOb
 {
     // NB: If you add new slots here, make sure to change
     // js::NukeChromeCrossCompartmentWrappers to cope.
     MarkCrossCompartmentSlot(trc, &GetCall(obj), "call");
     MarkSlot(trc, &GetFunctionProxyConstruct(obj), "construct");
     proxy_TraceObject(trc, obj);
 }
 
+static JSObject *
+proxy_WeakmapKeyDelegate(RawObject obj)
+{
+    JS_ASSERT(obj->isProxy());
+    return GetProxyHandler(obj)->weakmapKeyDelegate(obj);
+}
+
 static JSBool
 proxy_Convert(JSContext *cx, HandleObject proxy, JSType hint, MutableHandleValue vp)
 {
     JS_ASSERT(proxy->isProxy());
     return Proxy::defaultValue(cx, proxy, hint, vp.address());
 }
 
 static void
 proxy_Finalize(FreeOp *fop, RawObject obj)
 {
     JS_ASSERT(obj->isProxy());
-    if (!obj->getSlot(JSSLOT_PROXY_HANDLER).isUndefined())
-        GetProxyHandler(obj)->finalize(fop, obj);
+    GetProxyHandler(obj)->finalize(fop, obj);
 }
 
 static JSBool
 proxy_HasInstance(JSContext *cx, HandleObject proxy, MutableHandleValue v, JSBool *bp)
 {
     bool b;
     if (!Proxy::hasInstance(cx, proxy, v, &b))
         return false;
@@ -2884,33 +2902,44 @@ proxy_HasInstance(JSContext *cx, HandleO
 
 static JSType
 proxy_TypeOf(JSContext *cx, HandleObject proxy)
 {
     JS_ASSERT(proxy->isProxy());
     return Proxy::typeOf(cx, proxy);
 }
 
+#define PROXY_CLASS_EXT                             \
+    {                                               \
+        NULL,                /* equality */         \
+        NULL,                /* outerObject */      \
+        NULL,                /* innerObject */      \
+        NULL,                /* iteratorObject */   \
+        NULL,                /* unused */           \
+        false,               /* isWrappedNative */  \
+        proxy_WeakmapKeyDelegate                    \
+    }
+
 JS_FRIEND_DATA(Class) js::ObjectProxyClass = {
     "Proxy",
     Class::NON_NATIVE | JSCLASS_IMPLEMENTS_BARRIERS | JSCLASS_HAS_RESERVED_SLOTS(4),
     JS_PropertyStub,         /* addProperty */
     JS_PropertyStub,         /* delProperty */
     JS_PropertyStub,         /* getProperty */
     JS_StrictPropertyStub,   /* setProperty */
     JS_EnumerateStub,
     JS_ResolveStub,
     proxy_Convert,
     proxy_Finalize,          /* finalize    */
     NULL,                    /* checkAccess */
     NULL,                    /* call        */
     proxy_HasInstance,       /* hasInstance */
     NULL,                    /* construct   */
     proxy_TraceObject,       /* trace       */
-    JS_NULL_CLASS_EXT,
+    PROXY_CLASS_EXT,
     {
         proxy_LookupGeneric,
         proxy_LookupProperty,
         proxy_LookupElement,
         proxy_LookupSpecial,
         proxy_DefineGeneric,
         proxy_DefineProperty,
         proxy_DefineElement,
@@ -2956,17 +2985,20 @@ JS_FRIEND_DATA(Class) js::OuterWindowPro
     NULL,                    /* call        */
     NULL,                    /* hasInstance */
     NULL,                    /* construct   */
     proxy_TraceObject,       /* trace       */
     {
         NULL,                /* equality    */
         NULL,                /* outerObject */
         proxy_innerObject,
-        NULL                 /* unused */
+        NULL,                /* iteratorObject */
+        NULL,                /* unused */
+        false,               /* isWrappedNative */
+        proxy_WeakmapKeyDelegate
     },
     {
         proxy_LookupGeneric,
         proxy_LookupProperty,
         proxy_LookupElement,
         proxy_LookupSpecial,
         proxy_DefineGeneric,
         proxy_DefineProperty,
@@ -3026,17 +3058,17 @@ JS_FRIEND_DATA(Class) js::FunctionProxyC
     JS_ResolveStub,
     JS_ConvertStub,
     proxy_Finalize,          /* finalize */
     NULL,                    /* checkAccess */
     proxy_Call,
     FunctionClass.hasInstance,
     proxy_Construct,
     proxy_TraceFunction,     /* trace       */
-    JS_NULL_CLASS_EXT,
+    PROXY_CLASS_EXT,
     {
         proxy_LookupGeneric,
         proxy_LookupProperty,
         proxy_LookupElement,
         proxy_LookupSpecial,
         proxy_DefineGeneric,
         proxy_DefineProperty,
         proxy_DefineElement,
--- a/js/src/jsproxy.h
+++ b/js/src/jsproxy.h
@@ -119,16 +119,19 @@ class JS_FRIEND_API(BaseProxyHandler) {
     virtual JSString *fun_toString(JSContext *cx, JSObject *proxy, unsigned indent);
     virtual bool regexp_toShared(JSContext *cx, JSObject *proxy, RegExpGuard *g);
     virtual bool defaultValue(JSContext *cx, JSObject *obj, JSType hint, Value *vp);
     virtual bool iteratorNext(JSContext *cx, JSObject *proxy, Value *vp);
     virtual void finalize(JSFreeOp *fop, JSObject *proxy);
     virtual bool getElementIfPresent(JSContext *cx, JSObject *obj, JSObject *receiver,
                                      uint32_t index, Value *vp, bool *present);
     virtual bool getPrototypeOf(JSContext *cx, JSObject *proxy, JSObject **protop);
+
+    /* See comment for weakmapKeyDelegateOp in jsclass.h. */
+    virtual JSObject *weakmapKeyDelegate(JSObject *proxy);
 };
 
 /*
  * IndirectProxyHandler assumes that a target exists. Moreover, it assumes the
  * target is a JSObject. Consequently, it provides default implementations for
  * the fundamental traps that forward their behavior to the target. The derived
  * traps, however, are inherited from BaseProxyHandler, and therefore still
  * implemented in terms of the fundamental ones. This allows consumers of this
@@ -171,16 +174,17 @@ class JS_PUBLIC_API(IndirectProxyHandler
     virtual JSString *fun_toString(JSContext *cx, JSObject *proxy,
                                    unsigned indent) MOZ_OVERRIDE;
     virtual bool regexp_toShared(JSContext *cx, JSObject *proxy,
                                  RegExpGuard *g) MOZ_OVERRIDE;
     virtual bool defaultValue(JSContext *cx, JSObject *obj, JSType hint,
                               Value *vp) MOZ_OVERRIDE;
     virtual bool iteratorNext(JSContext *cx, JSObject *proxy,
                               Value *vp) MOZ_OVERRIDE;
+    virtual JSObject *weakmapKeyDelegate(JSObject *proxy);
 };
 
 /*
  * DirectProxyHandler has the same assumptions about the target as its base,
  * IndirectProxyHandler. Its fundamental traps are inherited from this class,
  * and therefore forward their behavior to the target. The derived traps,
  * however, are overrided so that, they too, forward their behavior to the
  * target. This allows consumers of this class to forward to another object as
--- a/js/src/jsweakmap.cpp
+++ b/js/src/jsweakmap.cpp
@@ -103,24 +103,17 @@ GetObjectMap(JSObject *obj)
 static JSObject *
 GetKeyArg(JSContext *cx, CallArgs &args)
 {
     Value *vp = &args[0];
     if (vp->isPrimitive()) {
         JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_NOT_NONNULL_OBJECT);
         return NULL;
     }
-    JSObject &key = vp->toObject();
-
-    // 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);
+    return &vp->toObject();
 }
 
 JS_ALWAYS_INLINE bool
 IsWeakMap(const Value &v)
 {
     return v.isObject() && v.toObject().hasClass(&WeakMapClass);
 }
 
@@ -246,17 +239,17 @@ WeakMap_set_impl(JSContext *cx, CallArgs
             JS_ReportOutOfMemory(cx);
             return false;
         }
         thisObj->setPrivate(map);
     }
 
     // Preserve wrapped native keys to prevent wrapper optimization.
     if (key->getClass()->ext.isWrappedNative) {
-        MOZ_ASSERT(cx->runtime->preserveWrapperCallback, "wrapped native weak map key needs preserveWrapperCallback");
+        JS_ASSERT(cx->runtime->preserveWrapperCallback);
         if (!cx->runtime->preserveWrapperCallback(cx, key)) {
             JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_WEAKMAP_KEY);
             return false;
         }
     }
 
     if (!map->put(key, value)) {
         JS_ReportOutOfMemory(cx);
@@ -283,22 +276,17 @@ JS_NondeterministicGetWeakMapKeys(JSCont
         return true;
     }
     RootedObject arr(cx, NewDenseEmptyArray(cx));
     if (!arr)
         return false;
     ObjectValueMap *map = GetObjectMap(obj);
     if (map) {
         for (ObjectValueMap::Base::Range r = map->all(); !r.empty(); r.popFront()) {
-            RootedObject key(cx, r.front().key);
-            // Re-wrapping the key (see comment of GetKeyArg)
-            if (!JS_WrapObject(cx, key.address()))
-                return false;
-
-            if (!js_NewbornArrayPush(cx, arr, ObjectValue(*key)))
+            if (!js_NewbornArrayPush(cx, arr, ObjectValue(*r.front().key)))
                 return false;
         }
     }
     *ret = arr;
     return true;
 }
 
 static void
--- a/js/src/jsweakmap.h
+++ b/js/src/jsweakmap.h
@@ -22,52 +22,20 @@ namespace js {
 // a key is collected, the table entry disappears, dropping its reference to the value.
 //
 // More precisely:
 //
 //     A WeakMap entry is collected if and only if either the WeakMap or the entry's key
 //     is collected. If an entry is not collected, it remains in the WeakMap and it has a
 //     strong reference to the value.
 //
-// You must call this table's 'mark' method when the object of which it is a part is
+// You must call this table's 'trace' method when the object of which it is a part is
 // reached by the garbage collection tracer. Once a table is known to be live, the
 // implementation takes care of the iterative marking needed for weak tables and removing
 // table entries when collection is complete.
-//
-// You may provide your own MarkPolicy class to specify how keys and values are marked; a
-// policy template provides default definitions for some common key/value type
-// combinations.
-//
-// Details:
-//
-// The interface is as for a js::HashMap, with the following additions:
-//
-// - You must call the WeakMap's 'trace' member function when you discover that the map is
-//   part of a live object. (You'll typically call this from the containing type's 'trace'
-//   function.)
-//
-// - There is no AllocPolicy parameter; these are used with our garbage collector, so
-//   RuntimeAllocPolicy is hard-wired.
-//
-// - Optional fourth and fifth parameters are the MarkPolicies for the key and value type.
-//   A MarkPolicy has the constructor:
-//
-//     MarkPolicy(JSTracer *)
-//
-//   and the following member functions:
-//
-//     bool isMarked(const Type &x)
-//        Return true if x has been marked as live by the garbage collector.
-//
-//     bool mark(Type &x)
-//        Return false if x is already marked. Otherwise, mark x and return true.
-//
-//   If omitted, the MarkPolicy parameter defaults to js::DefaultMarkPolicy<Type>,
-//   a policy template with the obvious definitions for some typical
-//   SpiderMonkey type combinations.
 
 // The value for the next pointer for maps not in the map list.
 static WeakMapBase * const WeakMapNotInList = reinterpret_cast<WeakMapBase *>(1);
 
 typedef Vector<WeakMapBase *, 0, SystemAllocPolicy> WeakMapVector;
 
 // Common base class for all WeakMap specializations. The collector uses this to call
 // their markIteratively and sweep methods.
@@ -165,26 +133,49 @@ class WeakMap : public HashMap<Key, Valu
         return true;
     }
 
     void nonMarkingTrace(JSTracer *trc) {
         for (Range r = Base::all(); !r.empty(); r.popFront())
             markValue(trc, &r.front().value);
     }
 
+    bool keyNeedsMark(JSObject *key) {
+        if (JSWeakmapKeyDelegateOp op = key->getClass()->ext.weakmapKeyDelegateOp) {
+            JSObject *delegate = op(key);
+            /*
+             * Check if the delegate is marked with any color to properly handle
+             * gray marking when the key's delegate is black and the map is
+             * gray.
+             */
+            return delegate && gc::IsObjectMarked(&delegate);
+        }
+        return false;
+    }
+
+    bool keyNeedsMark(gc::Cell *cell) {
+        return false;
+    }
+
     bool markIteratively(JSTracer *trc) {
         bool markedAny = false;
         for (Enum e(*this); !e.empty(); e.popFront()) {
             /* If the entry is live, ensure its key and value are marked. */
             Key prior(e.front().key);
             if (gc::IsMarked(const_cast<Key *>(&e.front().key))) {
                 if (markValue(trc, &e.front().value))
                     markedAny = true;
                 if (prior != e.front().key)
                     e.rekeyFront(e.front().key);
+            } else if (keyNeedsMark(e.front().key)) {
+                gc::Mark(trc, const_cast<Key *>(&e.front().key), "proxy-preserved WeakMap key");
+                if (prior != e.front().key)
+                    e.rekeyFront(e.front().key);
+                gc::Mark(trc, &e.front().value, "WeakMap entry");
+                markedAny = true;
             }
         }
         return markedAny;
     }
 
     void sweep(JSTracer *trc) {
         /* Remove all entries whose keys remain unmarked. */
         for (Enum e(*this); !e.empty(); e.popFront()) {
--- a/js/xpconnect/tests/chrome/Makefile.in
+++ b/js/xpconnect/tests/chrome/Makefile.in
@@ -57,16 +57,17 @@ MOCHITEST_CHROME_FILES = \
 		test_expandosharing.xul \
 		file_expandosharing.jsm \
 		test_getweakmapkeys.xul \
 		test_mozMatchesSelector.xul \
 		test_nodelists.xul \
 		test_precisegc.xul \
 		test_sandboxImport.xul \
 		test_weakmaps.xul \
+		test_weakmap_keys_preserved.xul \
 		test_weakref.xul \
 		test_wrappers.xul \
 		$(NULL)
 
 # Disabled until this test gets updated to test the new proxy based
 # wrappers.
 #		test_wrappers-2.xul \
 
new file mode 100644
--- /dev/null
+++ b/js/xpconnect/tests/chrome/test_weakmap_keys_preserved.xul
@@ -0,0 +1,37 @@
+<?xml version="1.0"?>
+<?xml-stylesheet type="text/css" href="chrome://global/skin"?>
+<?xml-stylesheet type="text/css" href="chrome://mochikit/content/tests/SimpleTest/test.css"?>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=673468
+-->
+<window title="Mozilla Bug "
+        xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
+  <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"/>
+
+  <!-- test results are displayed in the html:body -->
+  <body xmlns="http://www.w3.org/1999/xhtml">
+  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id="
+     target="_blank">Mozilla Bug 673468</a>
+  </body>
+
+  <!-- test code goes here -->
+  <script type="application/javascript">
+  <![CDATA[
+  /** Test for Bug 673468 **/
+
+  let Cc = Components.classes;
+  let Cu = Components.utils;
+  let Ci = Components.interfaces;
+
+  let system = Cc["@mozilla.org/systemprincipal;1"].createInstance();
+  let sandbox = Cu.Sandbox(system);
+  let map = sandbox.WeakMap();
+  let obj = {};
+  map.set(obj, {});
+
+  Cu.forceGC();
+
+  ok(map.has(obj), "Weakmap still contains our wrapper!");
+  ]]>
+  </script>
+</window>