Bug 1766656 - Take account of dynamic elements when swapping object r=jandem

We need to copy nursery allocated elements into malloc memory when swapping a
nursery obect into the tenured heap, and update memory accounting in a few
places.

The patch also fixes a bug in calculating how much of the nursery was tenured
which came up during testing (we don't know how big proxy objects if they've
been swpping into the nursery so assume the minimum size).

Differential Revision: https://phabricator.services.mozilla.com/D145722

js/src/gc/Nursery.cpp

@@ -683,6 +683,7 @@ void* js::Nursery::allocateZeroedBuffer(
 void* js::Nursery::reallocateBuffer(Zone* zone, Cell* cell, void* oldBuffer,
                                     size_t oldBytes, size_t newBytes) {
   if (!IsInsideNursery(cell)) {
+    MOZ_ASSERT(!isInside(oldBuffer));
     return zone->pod_realloc<uint8_t>((uint8_t*)oldBuffer, oldBytes, newBytes);
   }
 

js/src/gc/Scheduling.cpp

@@ -765,7 +765,8 @@ void MemoryTracker::swapGCMemory(Cell* a, Cell* b, MemoryUse use) {
 
   AutoEnterOOMUnsafeRegion oomUnsafe;
 
-  if ((sa && !gcMap.put(kb, sa)) || (sb && !gcMap.put(ka, sb))) {
+  if ((sa && b->isTenured() && !gcMap.put(kb, sa)) ||
+      (sb && a->isTenured() && !gcMap.put(ka, sb))) {
     oomUnsafe.crash("MemoryTracker::swapGCMemory");
   }
 }

js/src/gc/Tenuring.cpp

@@ -534,9 +534,6 @@ JSObject* js::TenuringTracer::moveToTenuredSlow(JSObject* src) {
     NativeObject* nsrc = &src->as<NativeObject>();
     tenuredSize += moveSlotsToTenured(ndst, nsrc);
     tenuredSize += moveElementsToTenured(ndst, nsrc, dstKind);
-
-    // There is a pointer into a dictionary mode object from the head of its
-    // shape list. This is updated in Nursery::sweepDictionaryModeObjects().
   }
 
   JSObjectMovedOp op = dst->getClass()->extObjectMovedOp();

js/src/jit-test/tests/gc/bug-1766656.js

@@ -0,0 +1,17 @@
+// |jit-test| error: Error
+
+const thisGlobal = this;
+const otherGlobalSameCompartment = newGlobal({sameCompartmentAs: thisGlobal});
+const globals = [thisGlobal, otherGlobalSameCompartment, undefined];
+function testProperties(global, count) {
+  let {object: source, transplant} = transplantableObject();
+  for (let i9 = 0; i9 < count; i9++) {
+    source[(0) + i9] = i9;
+  }
+  transplant(global);
+}
+for (let global of globals) {
+  for (let count of [0, 10, 30]) {
+    testProperties(global, count);
+  }
+}

js/src/jsapi-tests/testObjectSwap.cpp

@@ -23,6 +23,7 @@ using namespace js;
 
 struct NativeConfig {
   uint32_t propCount;
+  uint32_t elementCount;
   bool inDictionaryMode;
 };
 
@@ -59,6 +60,8 @@ static const JSClass TestDOMClasses[] = {
 
 static const uint32_t TestPropertyCounts[] = {0, 1, 2, 7, 8, 20};
 
+static const uint32_t TestElementCounts[] = {0, 20};
+
 static bool Verbose = false;
 
 class TenuredProxyHandler final : public Wrapper {
@@ -86,26 +89,28 @@ BEGIN_TEST(testObjectSwap) {
 
   for (const ObjectConfig& config1 : objectConfigs) {
     for (const ObjectConfig& config2 : objectConfigs) {
-      uint32_t id1;
-      RootedObject obj1(cx, CreateObject(config1, &id1));
-      CHECK(obj1);
-
-      uint32_t id2;
-      RootedObject obj2(cx, CreateObject(config2, &id2));
-      CHECK(obj2);
-
-      if (Verbose) {
-        fprintf(stderr, "Swap %p (%s) and %p (%s)\n", obj1.get(),
-                GetLocation(obj1), obj2.get(), GetLocation(obj2));
-      }
-
       {
-        AutoEnterOOMUnsafeRegion oomUnsafe;
-        JSObject::swap(cx, obj1, obj2, oomUnsafe);
-      }
+        uint32_t id1;
+        RootedObject obj1(cx, CreateObject(config1, &id1));
+        CHECK(obj1);
 
-      CHECK(CheckObject(obj1, config2, id2));
-      CHECK(CheckObject(obj2, config1, id1));
+        uint32_t id2;
+        RootedObject obj2(cx, CreateObject(config2, &id2));
+        CHECK(obj2);
+
+        if (Verbose) {
+          fprintf(stderr, "Swap %p (%s) and %p (%s)\n", obj1.get(),
+                  GetLocation(obj1), obj2.get(), GetLocation(obj2));
+        }
+
+        {
+          AutoEnterOOMUnsafeRegion oomUnsafe;
+          JSObject::swap(cx, obj1, obj2, oomUnsafe);
+        }
+
+        CHECK(CheckObject(obj1, config2, id2));
+        CHECK(CheckObject(obj2, config1, id1));
+      }
 
       if (Verbose) {
         fprintf(stderr, "\n");
@@ -132,16 +137,20 @@ ObjectConfigVector CreateObjectConfigs() {
     for (uint32_t propCount : TestPropertyCounts) {
       config.native.propCount = propCount;
 
-      for (bool nurseryAllocated : {false, true}) {
-        config.nurseryAllocated = nurseryAllocated;
+      for (uint32_t elementCount : TestElementCounts) {
+        config.native.elementCount = elementCount;
 
-        for (bool inDictionaryMode : {false, true}) {
-          if (inDictionaryMode && propCount == 0) {
-            continue;
+        for (bool nurseryAllocated : {false, true}) {
+          config.nurseryAllocated = nurseryAllocated;
+
+          for (bool inDictionaryMode : {false, true}) {
+            if (inDictionaryMode && propCount == 0) {
+              continue;
+            }
+
+            config.native.inDictionaryMode = inDictionaryMode;
+            MOZ_RELEASE_ASSERT(configs.append(config));
           }
-
-          config.native.inDictionaryMode = inDictionaryMode;
-          MOZ_RELEASE_ASSERT(configs.append(config));
         }
       }
     }
@@ -213,6 +222,17 @@ JSObject* CreateNativeObject(const ObjectConfig& config) {
     }
   }
 
+  if (config.native.elementCount) {
+    if (!obj->ensureElements(cx, config.native.elementCount)) {
+      return nullptr;
+    }
+    for (uint32_t i = 0; i < config.native.elementCount; i++) {
+      obj->setDenseInitializedLength(i + 1);
+      obj->initDenseElement(i, Int32Value(nextId++));
+    }
+    MOZ_ASSERT(obj->hasDynamicElements());
+  }
+
   if (config.native.inDictionaryMode) {
     JS::ObjectOpResult result;
     JS_DeleteProperty(cx, obj, "dummy", result);
@@ -289,8 +309,9 @@ bool CheckObject(HandleObject obj, const ObjectConfig& config, uint32_t id) {
     fprintf(stderr, "Check %p is a %s object with %u reserved slots", obj.get(),
             config.isNative ? "native" : "proxy", reservedSlots);
     if (config.isNative) {
-      fprintf(stderr, ", %u properties and %s in dictionary mode\n",
-              config.native.propCount,
+      fprintf(stderr,
+              ", %u properties, %u elements and %s in dictionary mode\n",
+              config.native.propCount, config.native.elementCount,
               config.native.inDictionaryMode ? "is" : "is not");
     } else {
       fprintf(stderr, " with %s values\n",
@@ -315,7 +336,7 @@ bool CheckObject(HandleObject obj, const ObjectConfig& config, uint32_t id) {
   }
 
   if (config.isNative) {
-    NativeObject* nobj = &obj->as<NativeObject>();
+    RootedNativeObject nobj(cx, &obj->as<NativeObject>());
     uint32_t propCount = GetPropertyCount(nobj);
 
     CHECK(propCount == config.native.propCount);
@@ -328,6 +349,12 @@ bool CheckObject(HandleObject obj, const ObjectConfig& config, uint32_t id) {
       CHECK(JS_GetPropertyById(cx, obj, name, &value));
       CHECK(value == Int32Value(id++));
     }
+
+    CHECK(nobj->getDenseInitializedLength() == config.native.elementCount);
+    for (uint32_t i = 0; i < config.native.elementCount; i++) {
+      Value value = nobj->getDenseElement(i);
+      CHECK(value == Int32Value(id++));
+    }
   }
 
   return true;

js/src/vm/JSObject.cpp

@@ -1048,12 +1048,12 @@ bool js::ObjectMayBeSwapped(const JSObject* obj) {
   return clasp->isProxyObject() || clasp->isDOMClass();
 }
 
-bool NativeObject::copyAndFreeSlotsBeforeSwap(JSContext* cx,
-                                              MutableHandleValueVector values) {
-  MOZ_ASSERT(values.empty());
+bool NativeObject::prepareForSwap(JSContext* cx,
+                                  MutableHandleValueVector slotValuesOut) {
+  MOZ_ASSERT(slotValuesOut.empty());
 
   for (size_t i = 0; i < slotSpan(); i++) {
-    if (!values.append(getSlot(i))) {
+    if (!slotValuesOut.append(getSlot(i))) {
       return false;
     }
   }
@@ -1063,22 +1063,50 @@ bool NativeObject::copyAndFreeSlotsBeforeSwap(JSContext* cx,
     size_t size = ObjectSlots::allocSize(slotsHeader->capacity());
     RemoveCellMemory(this, size, MemoryUse::ObjectSlots);
     if (!cx->nursery().isInside(slotsHeader)) {
+      if (!isTenured()) {
+        cx->nursery().removeMallocedBuffer(slotsHeader, size);
+      }
       js_free(slotsHeader);
     }
     setEmptyDynamicSlots(0);
   }
 
+  if (hasDynamicElements()) {
+    ObjectElements* elements = getElementsHeader();
+    size_t count = elements->numAllocatedElements();
+    size_t size = count * sizeof(HeapSlot);
+
+    if (isTenured()) {
+      RemoveCellMemory(this, size, MemoryUse::ObjectElements);
+    } else if (cx->nursery().isInside(elements)) {
+      // Move nursery allocated elements in case they end up in a tenured
+      // object.
+      ObjectElements* newElements =
+          reinterpret_cast<ObjectElements*>(js_pod_malloc<HeapSlot>(count));
+      if (!newElements) {
+        return false;
+      }
+
+      memmove(newElements, elements, size);
+      elements_ = newElements->elements();
+    } else {
+      cx->nursery().removeMallocedBuffer(elements, size);
+    }
+    MOZ_ASSERT(hasDynamicElements());
+  }
+
   return true;
 }
 
 /* static */
-bool NativeObject::fillInSlotsAfterSwap(JSContext* cx, HandleNativeObject obj,
-                                        gc::AllocKind kind,
-                                        HandleValueVector values) {
+bool NativeObject::fixupAfterSwap(JSContext* cx, HandleNativeObject obj,
+                                  gc::AllocKind kind,
+                                  HandleValueVector slotValues) {
   // This object has just been swapped with some other object, and its shape
   // no longer reflects its allocated size. Correct this information and
   // fill the slots in with the specified values.
-  MOZ_ASSERT_IF(!obj->inDictionaryMode(), obj->slotSpan() == values.length());
+  MOZ_ASSERT_IF(!obj->inDictionaryMode(),
+                obj->slotSpan() == slotValues.length());
 
   // Make sure the shape's numFixedSlots() is correct.
   size_t nfixed = gc::GetGCKindSlots(kind);
@@ -1090,10 +1118,10 @@ bool NativeObject::fillInSlotsAfterSwap(JSContext* cx, HandleNativeObject obj,
   }
 
   uint32_t oldDictionarySlotSpan =
-      obj->inDictionaryMode() ? values.length() : 0;
+      obj->inDictionaryMode() ? slotValues.length() : 0;
 
   size_t ndynamic =
-      calculateDynamicSlots(nfixed, values.length(), obj->getClass());
+      calculateDynamicSlots(nfixed, slotValues.length(), obj->getClass());
   size_t currentSlots = obj->getSlotsHeader()->capacity();
   MOZ_ASSERT(ndynamic >= currentSlots);
   if (ndynamic > currentSlots) {
@@ -1106,35 +1134,46 @@ bool NativeObject::fillInSlotsAfterSwap(JSContext* cx, HandleNativeObject obj,
     obj->setDictionaryModeSlotSpan(oldDictionarySlotSpan);
   }
 
-  for (size_t i = 0, len = values.length(); i < len; i++) {
-    obj->initSlotUnchecked(i, values[i]);
+  for (size_t i = 0, len = slotValues.length(); i < len; i++) {
+    obj->initSlotUnchecked(i, slotValues[i]);
+  }
+
+  if (obj->hasDynamicElements()) {
+    ObjectElements* elements = obj->getElementsHeader();
+    MOZ_ASSERT(!cx->nursery().isInside(elements));
+    size_t size = elements->numAllocatedElements() * sizeof(HeapSlot);
+    if (obj->isTenured()) {
+      AddCellMemory(obj, size, MemoryUse::ObjectElements);
+    } else if (!cx->nursery().registerMallocedBuffer(elements, size)) {
+      return false;
+    }
   }
 
   return true;
 }
 
-[[nodiscard]] bool ProxyObject::copyAndFreeValuesBeforeSwap(
-    JSContext* cx, MutableHandleValueVector values) {
-  MOZ_ASSERT(values.empty());
+[[nodiscard]] bool ProxyObject::prepareForSwap(
+    JSContext* cx, MutableHandleValueVector valuesOut) {
+  MOZ_ASSERT(valuesOut.empty());
 
   // Remove the GCPtrValues we're about to swap from the store buffer, to
   // ensure we don't trace bogus values.
   gc::StoreBuffer& sb = cx->runtime()->gc.storeBuffer();
 
   // Reserve space for the expando, private slot and the reserved slots.
-  if (!values.reserve(2 + numReservedSlots())) {
+  if (!valuesOut.reserve(2 + numReservedSlots())) {
     return false;
   }
 
   js::detail::ProxyValueArray* valArray = data.values();
   sb.unputValue(&valArray->expandoSlot);
   sb.unputValue(&valArray->privateSlot);
-  values.infallibleAppend(valArray->expandoSlot);
-  values.infallibleAppend(valArray->privateSlot);
+  valuesOut.infallibleAppend(valArray->expandoSlot);
+  valuesOut.infallibleAppend(valArray->privateSlot);
 
   for (size_t i = 0; i < numReservedSlots(); i++) {
     sb.unputValue(&valArray->reservedSlots.slots[i]);
-    values.infallibleAppend(valArray->reservedSlots.slots[i]);
+    valuesOut.infallibleAppend(valArray->reservedSlots.slots[i]);
   }
 
   if (isTenured() && !usingInlineValueArray()) {
@@ -1148,8 +1187,8 @@ bool NativeObject::fillInSlotsAfterSwap(JSContext* cx, HandleNativeObject obj,
   return true;
 }
 
-bool ProxyObject::fillInValuesAfterSwap(JSContext* cx,
-                                        const HandleValueVector values) {
+bool ProxyObject::fixupAfterSwap(JSContext* cx,
+                                 const HandleValueVector values) {
   MOZ_ASSERT(getClass()->isProxyObject());
 
   size_t nreserved = numReservedSlots();
@@ -1213,13 +1252,7 @@ static gc::AllocKind SwappableObjectAllocKind(JSObject* obj) {
     return obj->as<NativeObject>().allocKindForTenure();
   }
 
-  ProxyObject* proxy = &obj->as<ProxyObject>();
-  if (proxy->usingInlineValueArray()) {
-    return proxy->allocKindForTenure();
-  }
-
-  // Assume minimum size for nursery allocated proxies with out-of-line values.
-  return gc::AllocKind::OBJECT0;
+  return obj->as<ProxyObject>().allocKindForTenure();
 }
 
 /* Use this method with extreme caution. It trades the guts of two objects. */
@@ -1279,7 +1312,6 @@ void JSObject::swap(JSContext* cx, HandleObject a, HandleObject b,
 
   // Swap element associations.
   Zone* zone = a->zone();
-  zone->swapCellMemory(a, b, MemoryUse::ObjectElements);
 
   gc::AllocKind ka = SwappableObjectAllocKind(a);
   gc::AllocKind kb = SwappableObjectAllocKind(b);
@@ -1301,6 +1333,7 @@ void JSObject::swap(JSContext* cx, HandleObject a, HandleObject b,
     js_memcpy(a, b, size);
     js_memcpy(b, tmp, size);
 
+    zone->swapCellMemory(a, b, MemoryUse::ObjectElements);
     zone->swapCellMemory(a, b, MemoryUse::ProxyExternalValueArray);
 
     if (aIsProxyWithInlineValues) {
@@ -1322,21 +1355,21 @@ void JSObject::swap(JSContext* cx, HandleObject a, HandleObject b,
     RootedValueVector bvals(cx);
     NativeObject* na = a->is<NativeObject>() ? &a->as<NativeObject>() : nullptr;
     NativeObject* nb = b->is<NativeObject>() ? &b->as<NativeObject>() : nullptr;
-    if (na && !na->copyAndFreeSlotsBeforeSwap(cx, &avals)) {
-      oomUnsafe.crash("NativeObject::copyAndFreeSlotsBeforeSwap");
+    if (na && !na->prepareForSwap(cx, &avals)) {
+      oomUnsafe.crash("NativeObject::prepareForSwap");
     }
-    if (nb && !nb->copyAndFreeSlotsBeforeSwap(cx, &bvals)) {
-      oomUnsafe.crash("NativeObject::copyAndFreeSlotsBeforeSwap");
+    if (nb && !nb->prepareForSwap(cx, &bvals)) {
+      oomUnsafe.crash("NativeObject::prepareForSwap");
     }
 
     // Do the same for proxy value arrays.
     ProxyObject* pa = a->is<ProxyObject>() ? &a->as<ProxyObject>() : nullptr;
     ProxyObject* pb = b->is<ProxyObject>() ? &b->as<ProxyObject>() : nullptr;
-    if (pa && !pa->copyAndFreeValuesBeforeSwap(cx, &avals)) {
-      oomUnsafe.crash("ProxyObject::copyAndFreeValuesBeforeSwap");
+    if (pa && !pa->prepareForSwap(cx, &avals)) {
+      oomUnsafe.crash("ProxyObject::prepareForSwap");
     }
-    if (pb && !pb->copyAndFreeValuesBeforeSwap(cx, &bvals)) {
-      oomUnsafe.crash("ProxyObject::copyAndFreeValuesBeforeSwap");
+    if (pb && !pb->prepareForSwap(cx, &bvals)) {
+      oomUnsafe.crash("ProxyObject::prepareForSwap");
     }
 
     // Swap the main fields of the objects, whether they are native objects or
@@ -1346,20 +1379,20 @@ void JSObject::swap(JSContext* cx, HandleObject a, HandleObject b,
     js_memcpy(a, b, sizeof tmp);
     js_memcpy(b, &tmp, sizeof tmp);
 
-    if (na && !NativeObject::fillInSlotsAfterSwap(cx, b.as<NativeObject>(), kb,
-                                                  avals)) {
-      oomUnsafe.crash("NativeObject::fillInSlotsAfterSwap");
+    if (na &&
+        !NativeObject::fixupAfterSwap(cx, b.as<NativeObject>(), kb, avals)) {
+      oomUnsafe.crash("NativeObject::fixupAfterSwap");
     }
-    if (nb && !NativeObject::fillInSlotsAfterSwap(cx, a.as<NativeObject>(), ka,
-                                                  bvals)) {
-      oomUnsafe.crash("NativeObject::fillInSlotsAfterSwap");
+    if (nb &&
+        !NativeObject::fixupAfterSwap(cx, a.as<NativeObject>(), ka, bvals)) {
+      oomUnsafe.crash("NativeObject::fixupAfterSwap");
     }
 
-    if (pa && !b->as<ProxyObject>().fillInValuesAfterSwap(cx, avals)) {
-      oomUnsafe.crash("ProxyObject::fillInValuesAfterSwap");
+    if (pa && !b->as<ProxyObject>().fixupAfterSwap(cx, avals)) {
+      oomUnsafe.crash("ProxyObject::fixupAfterSwap");
     }
-    if (pb && !a->as<ProxyObject>().fillInValuesAfterSwap(cx, bvals)) {
-      oomUnsafe.crash("ProxyObject::fillInValuesAfterSwap");
+    if (pb && !a->as<ProxyObject>().fixupAfterSwap(cx, bvals)) {
+      oomUnsafe.crash("ProxyObject::fixupAfterSwap");
     }
   }
 

js/src/vm/NativeObject.h

@@ -998,12 +998,13 @@ class NativeObject : public JSObject {
                                            HandleNativeObject obj,
                                            uint32_t nfixed);
 
-  [[nodiscard]] bool copyAndFreeSlotsBeforeSwap(
-      JSContext* cx, MutableHandleValueVector values);
-  [[nodiscard]] static bool fillInSlotsAfterSwap(JSContext* cx,
-                                                 HandleNativeObject obj,
-                                                 gc::AllocKind kind,
-                                                 HandleValueVector values);
+  // For use from JSObject::swap.
+  [[nodiscard]] bool prepareForSwap(JSContext* cx,
+                                    MutableHandleValueVector slotValuesOut);
+  [[nodiscard]] static bool fixupAfterSwap(JSContext* cx,
+                                           HandleNativeObject obj,
+                                           gc::AllocKind kind,
+                                           HandleValueVector slotValues);
 
  public:
   // Return true if this object has been converted from shared-immutable

js/src/vm/ProxyObject.cpp

@@ -19,7 +19,8 @@ using namespace js;
 
 static gc::AllocKind GetProxyGCObjectKind(const JSClass* clasp,
                                           const BaseProxyHandler* handler,
-                                          const Value& priv) {
+                                          const Value& priv,
+                                          bool withInlineValues) {
   MOZ_ASSERT(clasp->isProxyObject());
 
   uint32_t nreserved = JSCLASS_RESERVED_SLOTS(clasp);
@@ -29,9 +30,12 @@ static gc::AllocKind GetProxyGCObjectKind(const JSClass* clasp,
   // JSCLASS_HAS_RESERVED_SLOTS since bug 1360523.
   MOZ_ASSERT(nreserved > 0);
 
-  uint32_t nslots = detail::ProxyValueArray::allocCount(nreserved);
-  MOZ_ASSERT(nslots <= NativeObject::MAX_FIXED_SLOTS);
+  uint32_t nslots = 0;
+  if (withInlineValues) {
+    nslots = detail::ProxyValueArray::allocCount(nreserved);
+  }
 
+  MOZ_ASSERT(nslots <= NativeObject::MAX_FIXED_SLOTS);
   gc::AllocKind kind = gc::GetGCObjectKind(nslots);
   if (handler->finalizeInBackground(priv)) {
     kind = ForegroundToBackgroundAllocKind(kind);
@@ -81,7 +85,8 @@ ProxyObject* ProxyObject::New(JSContext* cx, const BaseProxyHandler* handler,
   }
 #endif
 
-  gc::AllocKind allocKind = GetProxyGCObjectKind(clasp, handler, priv);
+  gc::AllocKind allocKind = GetProxyGCObjectKind(clasp, handler, priv,
+                                                 /* withInlineValues = */ true);
 
   Realm* realm = cx->realm();
 
@@ -134,7 +139,8 @@ ProxyObject* ProxyObject::New(JSContext* cx, const BaseProxyHandler* handler,
 
 gc::AllocKind ProxyObject::allocKindForTenure() const {
   Value priv = private_();
-  return GetProxyGCObjectKind(getClass(), data.handler, priv);
+  return GetProxyGCObjectKind(getClass(), data.handler, priv,
+                              usingInlineValueArray());
 }
 
 void ProxyObject::setCrossCompartmentPrivate(const Value& priv) {

js/src/vm/ProxyObject.h

@@ -61,10 +61,9 @@ class ProxyObject : public JSObject {
   }
 
   // For use from JSObject::swap.
-  [[nodiscard]] bool copyAndFreeValuesBeforeSwap(
-      JSContext* cx, MutableHandleValueVector values);
-  [[nodiscard]] bool fillInValuesAfterSwap(JSContext* cx,
-                                           HandleValueVector values);
+  [[nodiscard]] bool prepareForSwap(JSContext* cx,
+                                    MutableHandleValueVector valuesOut);
+  [[nodiscard]] bool fixupAfterSwap(JSContext* cx, HandleValueVector values);
 
   const Value& private_() const { return GetProxyPrivate(this); }
   const Value& expando() const { return GetProxyExpando(this); }