Bug 798678 - Refactor wrapper preservation for weakmaps (r=mccr8)
authorBill McCloskey <wmccloskey@mozilla.com>
Mon, 08 Oct 2012 18:22:47 -0700
changeset 110626 4ca5990b24d5654294d83ae25a428e58282cef5d
parent 110625 2a96aeb11ad44ab6ac55c32a2bf177ad89dabb32
child 110627 1e5bf398fc887b518462e789526ee4159fdaf632
push id23704
push useremorley@mozilla.com
push dateThu, 18 Oct 2012 17:12:58 +0000
treeherdermozilla-central@3779eb3f036f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs798678
milestone19.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 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>