Bug 1648453 - Don't try and store the incumbent global directly r=sfink
authorJon Coppeard <jcoppeard@mozilla.com>
Fri, 17 Jul 2020 17:36:04 +0000
changeset 540952 fcb43544e8a2539f33f83ac771005304ee4425ad
parent 540951 0ebd8b6fce88c207493c81571caa11c360ed65cd
child 540953 7ec3739f95e4e712af8108ebff81843754ce7288
push id37612
push userabutkovits@mozilla.com
push dateFri, 17 Jul 2020 21:28:41 +0000
treeherdermozilla-central@bd7c35b7e234 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1648453
milestone80.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 1648453 - Don't try and store the incumbent global directly r=sfink The previous patches store the incumbent global directly, which is going to be a problem if it's in another compartment. It needs to be wrapped into the current compartment. But apparently that can be problematic since you don't know how far to unwrap it to get the original gobal object back (it might itself be a wrapper). The solution used for promises is to store a CCW to another object with the same global. This patch applies the same fix to FinalizationRegistry. Differential Revision: https://phabricator.services.mozilla.com/D83831
js/src/builtin/FinalizationRegistryObject.cpp
js/src/builtin/FinalizationRegistryObject.h
js/src/gc/FinalizationRegistry.cpp
--- a/js/src/builtin/FinalizationRegistryObject.cpp
+++ b/js/src/builtin/FinalizationRegistryObject.cpp
@@ -302,18 +302,24 @@ bool FinalizationRegistryObject::constru
   }
 
   RootedObject cleanupCallback(
       cx, ValueToCallable(cx, args.get(0), 1, NO_CONSTRUCT));
   if (!cleanupCallback) {
     return false;
   }
 
-  Rooted<GlobalObject*> incumbentGlobal(cx,
-                                        cx->runtime()->getIncumbentGlobal(cx));
+  // It's problematic storing a CCW to a global in another compartment because
+  // you don't know how far to unwrap it to get the original object
+  // back. Instead store a CCW to a plain object in the same compartment as the
+  // global (this uses Object.prototype).
+  RootedObject incumbentObject(cx);
+  if (!GetObjectFromIncumbentGlobal(cx, &incumbentObject)) {
+    return false;
+  }
 
   Rooted<UniquePtr<ObjectWeakMap>> registrations(
       cx, cx->make_unique<ObjectWeakMap>(cx));
   if (!registrations) {
     return false;
   }
 
   Rooted<UniquePtr<FinalizationRecordSet>> activeRecords(
@@ -339,18 +345,18 @@ bool FinalizationRegistryObject::constru
   FinalizationRegistryObject* registry =
       NewObjectWithClassProto<FinalizationRegistryObject>(cx, proto);
   if (!registry) {
     return false;
   }
 
   registry->initReservedSlot(CleanupCallbackSlot,
                              ObjectValue(*cleanupCallback));
-  registry->initReservedSlot(IncumbentGlobalSlot,
-                             ObjectValue(*incumbentGlobal));
+  registry->initReservedSlot(IncumbentObjectSlot,
+                             ObjectValue(*incumbentObject));
   InitReservedSlot(registry, RegistrationsSlot, registrations.release(),
                    MemoryUse::FinalizationRegistryRegistrations);
   InitReservedSlot(registry, ActiveRecords, activeRecords.release(),
                    MemoryUse::FinalizationRegistryRecordSet);
   InitReservedSlot(registry, RecordsToBeCleanedUpSlot,
                    recordsToBeCleanedUp.release(),
                    MemoryUse::FinalizationRegistryRecordVector);
   registry->initReservedSlot(IsQueuedForCleanupSlot, BooleanValue(false));
@@ -439,22 +445,22 @@ void FinalizationRegistryObject::finaliz
 JSObject* FinalizationRegistryObject::cleanupCallback() const {
   Value value = getReservedSlot(CleanupCallbackSlot);
   if (value.isUndefined()) {
     return nullptr;
   }
   return &value.toObject();
 }
 
-GlobalObject* FinalizationRegistryObject::incumbentGlobal() const {
-  Value value = getReservedSlot(IncumbentGlobalSlot);
+JSObject* FinalizationRegistryObject::incumbentObject() const {
+  Value value = getReservedSlot(IncumbentObjectSlot);
   if (value.isUndefined()) {
     return nullptr;
   }
-  return &value.toObject().as<GlobalObject>();
+  return &value.toObject();
 }
 
 ObjectWeakMap* FinalizationRegistryObject::registrations() const {
   Value value = getReservedSlot(RegistrationsSlot);
   if (value.isUndefined()) {
     return nullptr;
   }
   return static_cast<ObjectWeakMap*>(value.toPrivate());
--- a/js/src/builtin/FinalizationRegistryObject.h
+++ b/js/src/builtin/FinalizationRegistryObject.h
@@ -170,17 +170,17 @@ using FinalizationRecordVector =
 
 using FinalizationRecordSet =
     GCHashSet<HeapPtrObject, MovableCellHasher<HeapPtrObject>, ZoneAllocPolicy>;
 
 // The FinalizationRegistry object itself.
 class FinalizationRegistryObject : public NativeObject {
   enum {
     CleanupCallbackSlot = 0,
-    IncumbentGlobalSlot,
+    IncumbentObjectSlot,
     RegistrationsSlot,
     ActiveRecords,
     RecordsToBeCleanedUpSlot,
     IsQueuedForCleanupSlot,
     DoCleanupFunctionSlot,
     SlotCount
   };
 
@@ -188,17 +188,17 @@ class FinalizationRegistryObject : publi
     DoCleanupFunction_RegistrySlot = 0,
   };
 
  public:
   static const JSClass class_;
   static const JSClass protoClass_;
 
   JSObject* cleanupCallback() const;
-  GlobalObject* incumbentGlobal() const;
+  JSObject* incumbentObject() const;
   ObjectWeakMap* registrations() const;
   FinalizationRecordSet* activeRecords() const;
   FinalizationRecordVector* recordsToBeCleanedUp() const;
   bool isQueuedForCleanup() const;
   JSFunction* doCleanupFunction() const;
 
   void queueRecordToBeCleanedUp(FinalizationRecordObject* record);
   void setQueuedForCleanup(bool value);
--- a/js/src/gc/FinalizationRegistry.cpp
+++ b/js/src/gc/FinalizationRegistry.cpp
@@ -9,16 +9,17 @@
  */
 
 #include "builtin/FinalizationRegistryObject.h"
 #include "gc/GCRuntime.h"
 #include "gc/Zone.h"
 #include "vm/JSContext.h"
 
 #include "gc/PrivateIterators-inl.h"
+#include "vm/JSObject-inl.h"
 
 using namespace js;
 using namespace js::gc;
 
 bool GCRuntime::addFinalizationRegistry(JSContext* cx,
                                         FinalizationRegistryObject* registry) {
   if (!cx->zone()->finalizationRegistries().put(registry)) {
     ReportOutOfMemory(cx);
@@ -111,15 +112,27 @@ void GCRuntime::sweepFinalizationRegistr
       }
       e.removeFront();
     }
   }
 }
 
 void GCRuntime::queueFinalizationRegistryForCleanup(
     FinalizationRegistryObject* registry) {
-  // Prod the embedding to call us back later to run the finalization callbacks.
-  if (!registry->isQueuedForCleanup()) {
-    callHostCleanupFinalizationRegistryCallback(registry->doCleanupFunction(),
-                                                registry->incumbentGlobal());
-    registry->setQueuedForCleanup(true);
+  // Prod the embedding to call us back later to run the finalization callbacks,
+  // if necessary.
+
+  if (registry->isQueuedForCleanup()) {
+    return;
   }
+
+  // Derive the incumbent global by unwrapping the incumbent global object and
+  // then getting its global.
+  JSObject* object =
+      UncheckedUnwrapWithoutExpose(registry->incumbentObject());
+  MOZ_ASSERT(object);
+  GlobalObject* incumbentGlobal = &object->nonCCWGlobal();
+
+  callHostCleanupFinalizationRegistryCallback(registry->doCleanupFunction(),
+                                              incumbentGlobal);
+
+  registry->setQueuedForCleanup(true);
 }