Bug 1628440 - Use WeakHeapPtr for weakly-held vector elements in finalization registry registrations weakmap r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Thu, 09 Apr 2020 16:23:12 +0000
changeset 523255 d4be324b80a403215e60c3462272fbc46241d3cd
parent 523254 ce494e20e11c4d37fffc3ce91decb556e9ce265d
child 523256 c0e540ea69ddb21cb6dd6fafd7385a653ff3055b
push id37298
push userdvarga@mozilla.com
push dateFri, 10 Apr 2020 02:59:09 +0000
treeherdermozilla-central@b6fd7b67139a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1628440
milestone77.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 1628440 - Use WeakHeapPtr for weakly-held vector elements in finalization registry registrations weakmap r=sfink The problem is that HeapPtr<> has a prebarrier in its destructor. This isn't necessary for weakly held values, and cause problems if the values have already been finalized. The patch also renames FinalizationRecordVectorObject to FinalizationRegistrationsObject to better describe its purpose. Differential Revision: https://phabricator.services.mozilla.com/D70346
js/src/builtin/FinalizationRegistryObject.cpp
js/src/builtin/FinalizationRegistryObject.h
js/src/jit-test/tests/gc/bug-1628440.js
--- a/js/src/builtin/FinalizationRegistryObject.cpp
+++ b/js/src/builtin/FinalizationRegistryObject.cpp
@@ -128,100 +128,101 @@ void FinalizationRecordObject::trace(JST
   TraceManuallyBarrieredEdge(trc, &registry,
                              "FinalizationRecordObject weak registry");
   if (registry != record->registryUnbarriered()) {
     record->setReservedSlot(WeakRegistrySlot, PrivateValue(registry));
   }
 }
 
 ///////////////////////////////////////////////////////////////////////////
-// FinalizationRecordVectorObject
+// FinalizationRegistrationsObject
 
-const JSClass FinalizationRecordVectorObject::class_ = {
-    "FinalizationRecordVector",
+const JSClass FinalizationRegistrationsObject::class_ = {
+    "FinalizationRegistrations",
     JSCLASS_HAS_RESERVED_SLOTS(SlotCount) | JSCLASS_BACKGROUND_FINALIZE,
     &classOps_, JS_NULL_CLASS_SPEC};
 
-const JSClassOps FinalizationRecordVectorObject::classOps_ = {
-    nullptr,                                   // addProperty
-    nullptr,                                   // delProperty
-    nullptr,                                   // enumerate
-    nullptr,                                   // newEnumerate
-    nullptr,                                   // resolve
-    nullptr,                                   // mayResolve
-    FinalizationRecordVectorObject::finalize,  // finalize
-    nullptr,                                   // call
-    nullptr,                                   // hasInstance
-    nullptr,                                   // construct
-    nullptr,                                   // trace
+const JSClassOps FinalizationRegistrationsObject::classOps_ = {
+    nullptr,                                    // addProperty
+    nullptr,                                    // delProperty
+    nullptr,                                    // enumerate
+    nullptr,                                    // newEnumerate
+    nullptr,                                    // resolve
+    nullptr,                                    // mayResolve
+    FinalizationRegistrationsObject::finalize,  // finalize
+    nullptr,                                    // call
+    nullptr,                                    // hasInstance
+    nullptr,                                    // construct
+    nullptr,                                    // trace
 };
 
 /* static */
-FinalizationRecordVectorObject* FinalizationRecordVectorObject::create(
+FinalizationRegistrationsObject* FinalizationRegistrationsObject::create(
     JSContext* cx) {
   auto records = cx->make_unique<FinalizationRecordVector>(cx->zone());
   if (!records) {
     return nullptr;
   }
 
   auto object =
-      NewObjectWithNullTaggedProto<FinalizationRecordVectorObject>(cx);
+      NewObjectWithNullTaggedProto<FinalizationRegistrationsObject>(cx);
   if (!object) {
     return nullptr;
   }
 
   InitReservedSlot(object, RecordsSlot, records.release(),
                    MemoryUse::FinalizationRecordVector);
 
   return object;
 }
 
 /* static */
-void FinalizationRecordVectorObject::finalize(JSFreeOp* fop, JSObject* obj) {
-  auto rv = &obj->as<FinalizationRecordVectorObject>();
+void FinalizationRegistrationsObject::finalize(JSFreeOp* fop, JSObject* obj) {
+  auto rv = &obj->as<FinalizationRegistrationsObject>();
   fop->delete_(obj, rv->records(), MemoryUse::FinalizationRecordVector);
 }
 
-inline FinalizationRecordVector* FinalizationRecordVectorObject::records() {
-  return static_cast<FinalizationRecordVector*>(privatePtr());
+inline WeakFinalizationRecordVector*
+FinalizationRegistrationsObject::records() {
+  return static_cast<WeakFinalizationRecordVector*>(privatePtr());
 }
 
-inline const FinalizationRecordVector* FinalizationRecordVectorObject::records()
-    const {
-  return static_cast<const FinalizationRecordVector*>(privatePtr());
+inline const WeakFinalizationRecordVector*
+FinalizationRegistrationsObject::records() const {
+  return static_cast<const WeakFinalizationRecordVector*>(privatePtr());
 }
 
-inline void* FinalizationRecordVectorObject::privatePtr() const {
+inline void* FinalizationRegistrationsObject::privatePtr() const {
   Value value = getReservedSlot(RecordsSlot);
   if (value.isUndefined()) {
     return nullptr;
   }
   void* ptr = value.toPrivate();
   MOZ_ASSERT(ptr);
   return ptr;
 }
 
-inline bool FinalizationRecordVectorObject::isEmpty() const {
+inline bool FinalizationRegistrationsObject::isEmpty() const {
   MOZ_ASSERT(records());
   return records()->empty();
 }
 
-inline bool FinalizationRecordVectorObject::append(
+inline bool FinalizationRegistrationsObject::append(
     HandleFinalizationRecordObject record) {
   MOZ_ASSERT(records());
   return records()->append(record);
 }
 
-inline void FinalizationRecordVectorObject::remove(
+inline void FinalizationRegistrationsObject::remove(
     HandleFinalizationRecordObject record) {
   MOZ_ASSERT(records());
   records()->eraseIfEqual(record);
 }
 
-inline void FinalizationRecordVectorObject::sweep() {
+inline void FinalizationRegistrationsObject::sweep() {
   MOZ_ASSERT(records());
   return records()->sweep();
 }
 
 ///////////////////////////////////////////////////////////////////////////
 // FinalizationRegistryObject
 
 // Bug 1600300: FinalizationRegistryObject is foreground finalized so that
@@ -334,17 +335,17 @@ bool FinalizationRegistryObject::constru
   return true;
 }
 
 /* static */
 void FinalizationRegistryObject::trace(JSTracer* trc, JSObject* obj) {
   auto registry = &obj->as<FinalizationRegistryObject>();
 
   // Trace the registrations weak map. At most this traces the
-  // FinalizationRecordVectorObject values of the map; the contents of those
+  // FinalizationRegistrationsObject values of the map; the contents of those
   // objects are weakly held and are not traced.
   if (ObjectWeakMap* registrations = registry->registrations()) {
     registrations->trace(trc);
   }
 
   // The active record set is weakly held and is not traced.
 
   if (FinalizationRecordVector* records = registry->recordsToBeCleanedUp()) {
@@ -358,17 +359,17 @@ void FinalizationRegistryObject::sweep()
   MOZ_ASSERT(activeRecords());
   activeRecords()->sweep();
 
   // Sweep the contents of the registrations weak map's values.
   MOZ_ASSERT(registrations());
   for (ObjectValueWeakMap::Enum e(registrations()->valueMap()); !e.empty();
        e.popFront()) {
     auto registrations =
-        &e.front().value().toObject().as<FinalizationRecordVectorObject>();
+        &e.front().value().toObject().as<FinalizationRegistrationsObject>();
     registrations->sweep();
     if (registrations->isEmpty()) {
       e.removeFront();
     }
   }
 }
 
 /* static */
@@ -583,22 +584,22 @@ bool FinalizationRegistryObject::addRegi
     HandleObject unregisterToken, HandleFinalizationRecordObject record) {
   // Add the record to the list of records associated with this unregister
   // token.
 
   MOZ_ASSERT(unregisterToken);
   MOZ_ASSERT(registry->registrations());
 
   auto& map = *registry->registrations();
-  Rooted<FinalizationRecordVectorObject*> recordsObject(cx);
+  Rooted<FinalizationRegistrationsObject*> recordsObject(cx);
   JSObject* obj = map.lookup(unregisterToken);
   if (obj) {
-    recordsObject = &obj->as<FinalizationRecordVectorObject>();
+    recordsObject = &obj->as<FinalizationRegistrationsObject>();
   } else {
-    recordsObject = FinalizationRecordVectorObject::create(cx);
+    recordsObject = FinalizationRegistrationsObject::create(cx);
     if (!recordsObject || !map.add(cx, unregisterToken, recordsObject)) {
       return false;
     }
   }
 
   if (!recordsObject->append(record)) {
     ReportOutOfMemory(cx);
     return false;
@@ -616,17 +617,17 @@ bool FinalizationRegistryObject::addRegi
 
   MOZ_ASSERT(unregisterToken);
   MOZ_ASSERT(registry->registrations());
   JS::AutoAssertNoGC nogc;
 
   auto& map = *registry->registrations();
   JSObject* obj = map.lookup(unregisterToken);
   MOZ_ASSERT(obj);
-  auto records = &obj->as<FinalizationRecordVectorObject>();
+  auto records = &obj->as<FinalizationRegistrationsObject>();
   records->remove(record);
 
   if (records->empty()) {
     map.remove(unregisterToken);
   }
 }
 
 // FinalizationRegistry.prototype.unregister ( unregisterToken )
@@ -669,17 +670,17 @@ bool FinalizationRegistryObject::unregis
   //    that is an element of finalizationRegistry.[[Cells]], do
   //    a. If SameValue(cell.[[UnregisterToken]], unregisterToken) is true, then
   //       i. Remove cell from finalizationRegistry.[[Cells]].
   //       ii. Set removed to true.
 
   FinalizationRecordSet* activeRecords = registry->activeRecords();
   RootedObject obj(cx, registry->registrations()->lookup(unregisterToken));
   if (obj) {
-    auto* records = obj->as<FinalizationRecordVectorObject>().records();
+    auto* records = obj->as<FinalizationRegistrationsObject>().records();
     MOZ_ASSERT(records);
     MOZ_ASSERT(!records->empty());
     for (FinalizationRecordObject* record : *records) {
       if (record->isActive()) {
         // Clear the fields of this record; it will be removed from the target's
         // list when it is next swept.
         activeRecords->remove(record);
         record->clear();
--- a/js/src/builtin/FinalizationRegistryObject.h
+++ b/js/src/builtin/FinalizationRegistryObject.h
@@ -26,17 +26,17 @@
  *   |  |  +---------------------------+   |   +-------------------------+   |
  *   |  |  | Unregister  :   Records   |   |   |  Target  : Finalization-|   |
  *   |  |  |   token     :   object    |   |   |  object  : RecordVector |   |
  *   |  |  +--------------------+------+   |   +----+-------------+------+   |
  *   |  |                       |          |        |             |          |
  *   |  |                       v          |        v             |          |
  *   |  |  +--------------------+------+   |   +----+-----+       |          |
  *   |  |  |       Finalization-       |   |   |  Target  |       |          |
- *   |  |  |     RecordVectorObject    |   |   | JSObject |       |          |
+ *   |  |  |    RegistrationsObject    |   |   | JSObject |       |          |
  *   |  |  +-------------+-------------+   |   +----------+       |          |
  *   |  |  |       RecordVector        |   |                      |          |
  *   |  |  +-------------+-------------+   |                      |          |
  *   |  |                |                 |                      |          |
  *   |  |              * v                 |                      |          |
  *   |  |  +-------------+-------------+ * |                      |          |
  *   |  |  | FinalizationRecordObject  +<-------------------------+          |
  *   |  |  +---------------------------+   |                                 |
@@ -49,17 +49,17 @@
  *
  * Registering a target with a FinalizationRegistry creates a FinalizationRecord
  * containing the registry and the heldValue. This is added to a vector of
  * records associated with the target, implemented as a map on the target's
  * Zone. All finalization records are treated as GC roots.
  *
  * When a target is registered an unregister token may be supplied. If so, this
  * is also recorded by the registry and is stored in a weak map of
- * registrations. The values of this map are FinalizationRecordVectorObject
+ * registrations. The values of this map are FinalizationRegistrationsObject
  * objects. It's necessary to have another JSObject here because our weak map
  * implementation only supports JS types as values.
  *
  * After a target object has been registered with a finalization registry it is
  * expected that its callback will be called for that object even if the
  * finalization registry itself is no longer reachable from JS. Thus the values
  * of each zone's finalization record map are treated as roots and marked at the
  * start of GC.
@@ -126,34 +126,35 @@ class FinalizationRecordObject : public 
  private:
   static const JSClassOps classOps_;
 
   static void trace(JSTracer* trc, JSObject* obj);
 
   FinalizationRegistryObject* registryUnbarriered() const;
 };
 
-// A vector of FinalizationRecordObjects.
-using FinalizationRecordVector =
-    GCVector<HeapPtr<FinalizationRecordObject*>, 1, js::ZoneAllocPolicy>;
+// A vector of weakly-held FinalizationRecordObjects.
+using WeakFinalizationRecordVector =
+    GCVector<WeakHeapPtr<FinalizationRecordObject*>, 1, js::ZoneAllocPolicy>;
 
-// A JS object containing a vector of FinalizationRecordObjects, which holds the
-// records corresponding to the registrations for a particular registration
-// token. These are used as the values in the registration weakmap. The contents
-// of the vector are weak references and are not traced.
-class FinalizationRecordVectorObject : public NativeObject {
+// A JS object containing a vector of weakly-held FinalizationRecordObjects,
+// which holds the records corresponding to the registrations for a particular
+// registration token. These are used as the values in the registration
+// weakmap. Since the contents of the vector are weak references they are not
+// traced.
+class FinalizationRegistrationsObject : public NativeObject {
   enum { RecordsSlot = 0, SlotCount };
 
  public:
   static const JSClass class_;
 
-  static FinalizationRecordVectorObject* create(JSContext* cx);
+  static FinalizationRegistrationsObject* create(JSContext* cx);
 
-  FinalizationRecordVector* records();
-  const FinalizationRecordVector* records() const;
+  WeakFinalizationRecordVector* records();
+  const WeakFinalizationRecordVector* records() const;
 
   bool isEmpty() const;
 
   bool append(HandleFinalizationRecordObject record);
   void remove(HandleFinalizationRecordObject record);
 
   void sweep();
 
@@ -161,16 +162,19 @@ class FinalizationRecordVectorObject : p
   static const JSClassOps classOps_;
 
   void* privatePtr() const;
 
   static void trace(JSTracer* trc, JSObject* obj);
   static void finalize(JSFreeOp* fop, JSObject* obj);
 };
 
+using FinalizationRecordVector =
+    GCVector<HeapPtr<FinalizationRecordObject*>, 1, js::ZoneAllocPolicy>;
+
 using FinalizationRecordSet =
     GCHashSet<HeapPtrObject, MovableCellHasher<HeapPtrObject>, ZoneAllocPolicy>;
 
 // The FinalizationRegistry object itself.
 class FinalizationRegistryObject : public NativeObject {
   enum {
     CleanupCallbackSlot = 0,
     RegistrationsSlot,
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/gc/bug-1628440.js
@@ -0,0 +1,9 @@
+// |jit-test| error: ReferenceError
+gczeal(10, 2);
+let cleanup = function(iter) {}
+let key = {"k": "this is my key"};
+let fg = new FinalizationRegistry(cleanup);
+let object = {};
+fg.register(object, {}, key);
+let success = fg.unregister(key);
+throw "x";