Bug 1513108 - Remove the separate class extension hook for getting a weakmap key delegate, r=jonco

hotsphink · hotsphink · commit 10953d54c22f · 2018-12-07T14:38:01.000-08:00
Replace with just unwrapping the key, since there are no users that return anything else for a delegate.

--HG--
extra : rebase_source : e72b825121ca3493364c9347f65e5dddd1ef53e0
diff --git a/dom/bindings/Codegen.py b/dom/bindings/Codegen.py
@@ -508,7 +508,6 @@ def define(self):
             };
 
             static const js::ClassExtension sClassExtension = {
-              nullptr, /* weakmapKeyDelegateOp */
               ${objectMoved} /* objectMovedOp */
             };
 
diff --git a/dom/bindings/SimpleGlobalObject.cpp b/dom/bindings/SimpleGlobalObject.cpp
@@ -74,7 +74,7 @@ static const js::ClassOps SimpleGlobalClassOps = {
 };
 
 static const js::ClassExtension SimpleGlobalClassExtension = {
-    nullptr, SimpleGlobal_moved};
+    SimpleGlobal_moved};
 
 const js::Class SimpleGlobalClass = {
     "",
diff --git a/js/public/Class.h b/js/public/Class.h
@@ -493,8 +493,6 @@ typedef bool (*JSHasInstanceOp)(JSContext* cx, JS::HandleObject obj,
  */
 typedef void (*JSTraceOp)(JSTracer* trc, JSObject* obj);
 
-typedef JSObject* (*JSWeakmapKeyDelegateOp)(JSObject* obj);
-
 typedef size_t (*JSObjectMovedOp)(JSObject* obj, JSObject* old);
 
 /* js::Class operation signatures. */
@@ -687,19 +685,6 @@ struct MOZ_STATIC_CLASS ClassSpec {
 };
 
 struct MOZ_STATIC_CLASS ClassExtension {
-  /**
-   * 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;
-
   /**
    * Optional hook called when an object is moved by generational or
    * compacting GC.
@@ -952,9 +937,6 @@ struct MOZ_STATIC_CLASS Class {
     return spec ? spec->finishInit : nullptr;
   }
 
-  JSWeakmapKeyDelegateOp extWeakmapKeyDelegateOp() const {
-    return ext ? ext->weakmapKeyDelegateOp : nullptr;
-  }
   JSObjectMovedOp extObjectMovedOp() const {
     return ext ? ext->objectMovedOp : nullptr;
   }
diff --git a/js/public/Proxy.h b/js/public/Proxy.h
@@ -373,8 +373,6 @@ class JS_FRIEND_API BaseProxyHandler {
   virtual bool getElements(JSContext* cx, HandleObject proxy, uint32_t begin,
                            uint32_t end, ElementAdder* adder) const;
 
-  /* See comment for weakmapKeyDelegateOp in js/Class.h. */
-  virtual JSObject* weakmapKeyDelegate(JSObject* proxy) const;
   virtual bool isScripted() const { return false; }
 };
 
diff --git a/js/public/Wrapper.h b/js/public/Wrapper.h
@@ -133,7 +133,6 @@ class JS_FRIEND_API Wrapper : public ForwardingProxyHandler {
         mFlags(aFlags) {}
 
   virtual bool finalizeInBackground(const Value& priv) const override;
-  virtual JSObject* weakmapKeyDelegate(JSObject* proxy) const override;
 
   using BaseProxyHandler::Action;
 
diff --git a/js/rust/src/jsglue.cpp b/js/rust/src/jsglue.cpp
@@ -94,7 +94,6 @@ struct ProxyTraps {
 
   // getElements
 
-  // weakmapKeyDelegate
   // isScripted
 };
 
diff --git a/js/src/builtin/MapObject.cpp b/js/src/builtin/MapObject.cpp
@@ -141,7 +141,6 @@ static const ClassOps MapIteratorObjectClassOps = {nullptr, /* addProperty */
                                                    MapIteratorObject::finalize};
 
 static const ClassExtension MapIteratorObjectClassExtension = {
-    nullptr, /* weakmapKeyDelegateOp */
     MapIteratorObject::objectMoved};
 
 const Class MapIteratorObject::class_ = {
@@ -912,7 +911,6 @@ static const ClassOps SetIteratorObjectClassOps = {nullptr, /* addProperty */
                                                    SetIteratorObject::finalize};
 
 static const ClassExtension SetIteratorObjectClassExtension = {
-    nullptr, /* weakmapKeyDelegateOp */
     SetIteratorObject::objectMoved};
 
 const Class SetIteratorObject::class_ = {
diff --git a/js/src/builtin/TypedObject.cpp b/js/src/builtin/TypedObject.cpp
@@ -2301,8 +2301,7 @@ const ObjectOps TypedObject::objectOps_ = {
       Trace,                                                               \
   };                                                                       \
   static const ClassExtension Name##ClassExt = {                           \
-      nullptr, /* weakmapKeyDelegateOp */                                  \
-      Moved    /* objectMovedOp */                                         \
+      Moved /* objectMovedOp */                                            \
   };                                                                       \
   const Class Name::class_ = {                                             \
       #Name,           Class::NON_NATIVE | JSCLASS_DELAY_METADATA_BUILDER, \
diff --git a/js/src/builtin/WeakMapObject-inl.h b/js/src/builtin/WeakMapObject-inl.h
@@ -47,11 +47,9 @@ static MOZ_ALWAYS_INLINE bool WeakCollectionPutEntryInternal(
     return false;
   }
 
-  if (JSWeakmapKeyDelegateOp op = key->getClass()->extWeakmapKeyDelegateOp()) {
-    RootedObject delegate(cx, op(key));
-    if (delegate && !TryPreserveReflector(cx, delegate)) {
-      return false;
-    }
+  RootedObject delegate(cx, UncheckedUnwrapWithoutExpose(key));
+  if (delegate && !TryPreserveReflector(cx, delegate)) {
+    return false;
   }
 
   MOZ_ASSERT(key->compartment() == obj->compartment());
diff --git a/js/src/devtools/rootAnalysis/annotations.js b/js/src/devtools/rootAnalysis/annotations.js
@@ -31,9 +31,6 @@ function indirectCallCannotGC(fullCaller, fullVariable)
     if (name == "params" && caller == "PR_ExplodeTime")
         return true;
 
-    if (name == "op" && /GetWeakmapKeyDelegate/.test(caller))
-        return true;
-
     // hook called during script finalization which cannot GC.
     if (/CallDestroyScriptHook/.test(caller))
         return true;
@@ -168,7 +165,6 @@ var ignoreFunctions = {
     "PR_ExplodeTime" : true,
     "PR_ErrorInstallTable" : true,
     "PR_SetThreadPrivate" : true,
-    "JSObject* js::GetWeakmapKeyDelegate(JSObject*)" : true, // FIXME: mark with AutoSuppressGCAnalysis instead
     "uint8 NS_IsMainThread()" : true,
 
     // Has an indirect call under it by the name "__f", which seemed too
diff --git a/js/src/gc/Verifier.cpp b/js/src/gc/Verifier.cpp
@@ -751,13 +751,7 @@ static JSObject* MaybeGetDelegate(Cell* cell) {
   }
 
   JSObject* object = cell->as<JSObject>();
-  JSWeakmapKeyDelegateOp op = object->getClass()->extWeakmapKeyDelegateOp();
-  if (!op) {
-    return nullptr;
-  }
-
-  JS::AutoSuppressGCAnalysis nogc; // Calling the delegate op cannot GC.
-  return op(object);
+  return js::UncheckedUnwrapWithoutExpose(object);
 }
 
 bool js::gc::CheckWeakMapEntryMarking(const WeakMapBase* map, Cell* key,
diff --git a/js/src/gc/WeakMap-inl.h b/js/src/gc/WeakMap-inl.h
@@ -168,20 +168,7 @@ bool WeakMap<K, V>::markIteratively(GCMarker* marker) {
 
 template <class K, class V>
 inline JSObject* WeakMap<K, V>::getDelegate(JSObject* key) const {
-  JS::AutoSuppressGCAnalysis nogc;
-
-  JSWeakmapKeyDelegateOp op = key->getClass()->extWeakmapKeyDelegateOp();
-  if (!op) {
-    return nullptr;
-  }
-
-  JSObject* obj = op(key);
-  if (!obj) {
-    return nullptr;
-  }
-
-  MOZ_ASSERT(obj->runtimeFromMainThread() == zone()->runtimeFromMainThread());
-  return obj;
+  return UncheckedUnwrapWithoutExpose(key);
 }
 
 template <class K, class V>
diff --git a/js/src/gc/WeakMap.h b/js/src/gc/WeakMap.h
@@ -183,6 +183,15 @@ class WeakMap
 
   bool markIteratively(GCMarker* marker) override;
 
+  /**
+   * If a wrapper is used as a key in a weakmap, the garbage collector should
+   * keep that object around longer than it otherwise would. We want to avoid
+   * collecting the wrapper (and removing the weakmap entry) as long as the
+   * wrapped object is alive (because the object can be rewrapped and looked up
+   * again). 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.
+   */
   JSObject* getDelegate(JSObject* key) const;
   JSObject* getDelegate(JSScript* script) const;
   JSObject* getDelegate(LazyScript* script) const;
diff --git a/js/src/jsapi-tests/testGCGrayMarking.cpp b/js/src/jsapi-tests/testGCGrayMarking.cpp
@@ -8,6 +8,7 @@
 #include "gc/Heap.h"
 #include "gc/WeakMap.h"
 #include "gc/Zone.h"
+#include "js/Proxy.h"
 #include "jsapi-tests/tests.h"
 
 using namespace js;
@@ -227,7 +228,7 @@ bool TestWeakMaps() {
   // Test that a weakmap key is marked gray if it has a gray delegate and the
   // map is either gray or black.
 
-  JSObject* delegate = AllocDelegateForKey(key);
+  JSObject* delegate = UncheckedUnwrapWithoutExpose(key);
   blackRoot1 = weakMap;
   blackRoot2 = nullptr;
   grayRoots.grayRoot1 = delegate;
@@ -307,7 +308,6 @@ bool TestWeakMaps() {
   CHECK(IsMarkedBlack(weakMap));
   CHECK(IsMarkedBlack(value));
 
-  CHECK(AllocDelegateForKey(key));
   blackRoot1 = nullptr;
   blackRoot2 = nullptr;
   grayRoots.grayRoot1 = weakMap;
@@ -371,7 +371,7 @@ bool TestUnassociatedWeakMaps() {
 
   // Test that a weakmap key is marked gray if it has a gray delegate.
 
-  JSObject* delegate = AllocDelegateForKey(key);
+  JSObject* delegate = UncheckedUnwrapWithoutExpose(key);
   blackRoot = nullptr;
   grayRoots.grayRoot1 = delegate;
   grayRoots.grayRoot2 = nullptr;
@@ -409,7 +409,6 @@ bool TestUnassociatedWeakMaps() {
   CHECK(IsMarkedBlack(key));
   CHECK(IsMarkedBlack(value));
 
-  CHECK(AllocDelegateForKey(key));
   blackRoot = nullptr;
   grayRoots.grayRoot1 = key;
   grayRoots.grayRoot2 = nullptr;
@@ -652,32 +651,16 @@ JSObject* GetCrossCompartmentWrapper(JSObject* target) {
   return obj;
 }
 
-static JSObject* GetKeyDelegate(JSObject* obj) {
-  return static_cast<JSObject*>(obj->as<NativeObject>().getPrivate());
-}
-
 JSObject* AllocWeakmapKeyObject() {
-  static const js::ClassExtension KeyClassExtension = {GetKeyDelegate};
-
-  static const js::Class KeyClass = {"keyWithDelegate",  JSCLASS_HAS_PRIVATE,
-                                     JS_NULL_CLASS_OPS,  JS_NULL_CLASS_SPEC,
-                                     &KeyClassExtension, JS_NULL_OBJECT_OPS};
-
-  JS::RootedObject key(cx, JS_NewObject(cx, Jsvalify(&KeyClass)));
-  if (!key) {
+  JS::RootedObject delegate(cx, JS_NewPlainObject(cx));
+  if (!delegate) {
     return nullptr;
   }
 
-  EvictNursery();
-  return key;
-}
+  JS::RootedObject key(cx, js::Wrapper::New(cx, delegate, &js::Wrapper::singleton));
 
-JSObject* AllocDelegateForKey(JSObject* key) {
-  JS::RootedObject obj(cx, JS_NewPlainObject(cx));
   EvictNursery();
-
-  key->as<NativeObject>().setPrivate(obj);
-  return obj;
+  return key;
 }
 
 JSObject* AllocObjectChain(size_t length) {
diff --git a/js/src/jsapi-tests/testWeakMap.cpp b/js/src/jsapi-tests/testWeakMap.cpp
@@ -70,11 +70,14 @@ BEGIN_TEST(testWeakMap_keyDelegates) {
   JS::RootedObject map(cx, JS::NewWeakMapObject(cx));
   CHECK(map);
 
-  JS::RootedObject key(cx, newKey());
-  CHECK(key);
-
   JS::RootedObject delegate(cx, newDelegate());
+  JS::RootedObject key(cx, delegate);
+  if (!JS_WrapObject(cx, &key)) {
+    return false;
+  }
+  CHECK(key);
   CHECK(delegate);
+
   keyDelegate = delegate;
 
   JS::RootedObject delegateRoot(cx);
@@ -146,15 +149,11 @@ static size_t DelegateObjectMoved(JSObject* obj, JSObject* old) {
   return 0;
 }
 
-static JSObject* GetKeyDelegate(JSObject* obj) { return keyDelegate; }
-
 JSObject* newKey() {
-  static const js::ClassExtension keyClassExtension = {GetKeyDelegate};
-
   static const js::Class keyClass = {
       "keyWithDelegate",  JSCLASS_HAS_PRIVATE | JSCLASS_HAS_RESERVED_SLOTS(1),
       JS_NULL_CLASS_OPS,  JS_NULL_CLASS_SPEC,
-      &keyClassExtension, JS_NULL_OBJECT_OPS};
+      JS_NULL_CLASS_EXT, JS_NULL_OBJECT_OPS};
 
   JS::RootedObject key(cx, JS_NewObject(cx, Jsvalify(&keyClass)));
   if (!key) {
@@ -167,8 +166,8 @@ JSObject* newKey() {
 JSObject* newCCW(JS::HandleObject sourceZone, JS::HandleObject destZone) {
   /*
    * Now ensure that this zone will be swept first by adding a cross
-   * compartment wrapper to a new objct in the same zone as the
-   * delegate obejct.
+   * compartment wrapper to a new object in the same zone as the
+   * delegate object.
    */
   JS::RootedObject object(cx);
   {
@@ -208,7 +207,7 @@ JSObject* newDelegate() {
   };
 
   static const js::ClassExtension delegateClassExtension = {
-      nullptr, DelegateObjectMoved};
+      DelegateObjectMoved};
 
   static const js::Class delegateClass = {
       "delegate",
diff --git a/js/src/jsfriendapi.cpp b/js/src/jsfriendapi.cpp
@@ -575,13 +575,6 @@ JS_FRIEND_API void js::VisitGrayWrapperTargets(Zone* zone,
   }
 }
 
-JS_FRIEND_API JSObject* js::GetWeakmapKeyDelegate(JSObject* key) {
-  if (JSWeakmapKeyDelegateOp op = key->getClass()->extWeakmapKeyDelegateOp()) {
-    return op(key);
-  }
-  return nullptr;
-}
-
 JS_FRIEND_API JSLinearString* js::StringToLinearStringSlow(JSContext* cx,
                                                            JSString* str) {
   return str->ensureLinear(cx);
@@ -1046,7 +1039,7 @@ struct DumpHeapTracer : public JS::CallbackTracer, public WeakMapTracer {
   void trace(JSObject* map, JS::GCCellPtr key, JS::GCCellPtr value) override {
     JSObject* kdelegate = nullptr;
     if (key.is<JSObject>()) {
-      kdelegate = js::GetWeakmapKeyDelegate(&key.as<JSObject>());
+      kdelegate = UncheckedUnwrapWithoutExpose(&key.as<JSObject>());
     }
 
     fprintf(output, "WeakMapEntry map=%p key=%p keyDelegate=%p value=%p\n", map,
diff --git a/js/src/jsfriendapi.h b/js/src/jsfriendapi.h
@@ -495,8 +495,6 @@ extern JS_FRIEND_API void VisitGrayWrapperTargets(JS::Zone* zone,
                                                   GCThingCallback callback,
                                                   void* closure);
 
-extern JS_FRIEND_API JSObject* GetWeakmapKeyDelegate(JSObject* key);
-
 /**
  * Invoke cellCallback on every gray JSObject in the given zone.
  */
diff --git a/js/src/proxy/BaseProxyHandler.cpp b/js/src/proxy/BaseProxyHandler.cpp
@@ -377,10 +377,6 @@ size_t BaseProxyHandler::objectMoved(JSObject* proxy, JSObject* old) const {
   return 0;
 }
 
-JSObject* BaseProxyHandler::weakmapKeyDelegate(JSObject* proxy) const {
-  return nullptr;
-}
-
 bool BaseProxyHandler::getPrototype(JSContext* cx, HandleObject proxy,
                                     MutableHandleObject protop) const {
   MOZ_CRASH("must override getPrototype with dynamic prototype");
diff --git a/js/src/proxy/Proxy.cpp b/js/src/proxy/Proxy.cpp
@@ -745,11 +745,6 @@ static bool proxy_DeleteProperty(JSContext* cx, HandleObject obj, HandleId id,
   Proxy::trace(trc, obj);
 }
 
-static JSObject* proxy_WeakmapKeyDelegate(JSObject* obj) {
-  MOZ_ASSERT(obj->is<ProxyObject>());
-  return obj->as<ProxyObject>().handler()->weakmapKeyDelegate(obj);
-}
-
 static void proxy_Finalize(FreeOp* fop, JSObject* obj) {
   // Suppress a bogus warning about finalize().
   JS::AutoSuppressGCAnalysis nogc;
@@ -789,8 +784,7 @@ const ClassOps js::ProxyClassOps = {
     ProxyObject::trace, /* trace       */
 };
 
-const ClassExtension js::ProxyClassExtension = {proxy_WeakmapKeyDelegate,
-                                                proxy_ObjectMoved};
+const ClassExtension js::ProxyClassExtension = {proxy_ObjectMoved};
 
 const ObjectOps js::ProxyObjectOps = {
     proxy_LookupProperty, Proxy::defineProperty,
diff --git a/js/src/proxy/Wrapper.cpp b/js/src/proxy/Wrapper.cpp
diff --git a/js/src/vm/ArgumentsObject.cpp b/js/src/vm/ArgumentsObject.cpp
diff --git a/js/src/vm/ArrayBufferObject.cpp b/js/src/vm/ArrayBufferObject.cpp
diff --git a/js/src/vm/TypedArrayObject.cpp b/js/src/vm/TypedArrayObject.cpp
diff --git a/js/xpconnect/src/Sandbox.cpp b/js/xpconnect/src/Sandbox.cpp
diff --git a/js/xpconnect/src/XPCWrappedNativeJSOps.cpp b/js/xpconnect/src/XPCWrappedNativeJSOps.cpp
diff --git a/xpcom/base/CycleCollectedJSRuntime.cpp b/xpcom/base/CycleCollectedJSRuntime.cpp

Original file line number	Diff line number	Diff line change
`@@ -47,11 +47,9 @@ static MOZ_ALWAYS_INLINE bool WeakCollectionPutEntryInternal(`
`47`	`47`	`return false;`
`48`	`48`	`}`
`49`	`49`
`50`		`- if (JSWeakmapKeyDelegateOp op = key->getClass()->extWeakmapKeyDelegateOp()) {`
`51`		`- RootedObject delegate(cx, op(key));`
`52`		`- if (delegate && !TryPreserveReflector(cx, delegate)) {`
`53`		`- return false;`
`54`		`- }`
	`50`	`+ RootedObject delegate(cx, UncheckedUnwrapWithoutExpose(key));`
	`51`	`+ if (delegate && !TryPreserveReflector(cx, delegate)) {`
	`52`	`+ return false;`
`55`	`53`	`}`
`56`	`54`
`57`	`55`	`MOZ_ASSERT(key->compartment() == obj->compartment());`