diff --git a/js/src/gc/Nursery.cpp b/js/src/gc/Nursery.cpp index 486e295f5d38..a8624a196a3f 100644 --- a/js/src/gc/Nursery.cpp +++ b/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*)oldBuffer, oldBytes, newBytes); } diff --git a/js/src/gc/Scheduling.cpp b/js/src/gc/Scheduling.cpp index 0c1299db9722..6c02e8300a47 100644 --- a/js/src/gc/Scheduling.cpp +++ b/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"); } } diff --git a/js/src/gc/Tenuring.cpp b/js/src/gc/Tenuring.cpp index 7140e445b4c6..56ffac9f33e9 100644 --- a/js/src/gc/Tenuring.cpp +++ b/js/src/gc/Tenuring.cpp @@ -534,9 +534,6 @@ JSObject* js::TenuringTracer::moveToTenuredSlow(JSObject* src) { NativeObject* nsrc = &src->as(); 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(); diff --git a/js/src/jit-test/tests/gc/bug-1766656.js b/js/src/jit-test/tests/gc/bug-1766656.js new file mode 100644 index 000000000000..7096f6b27420 --- /dev/null +++ b/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); + } +} diff --git a/js/src/jsapi-tests/testObjectSwap.cpp b/js/src/jsapi-tests/testObjectSwap.cpp index 15fe369f2849..fd8774fdc454 100644 --- a/js/src/jsapi-tests/testObjectSwap.cpp +++ b/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(); + RootedNativeObject nobj(cx, &obj->as()); 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; diff --git a/js/src/vm/JSObject.cpp b/js/src/vm/JSObject.cpp index a1b7f78f5a6f..90a791150b87 100644 --- a/js/src/vm/JSObject.cpp +++ b/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(js_pod_malloc(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().allocKindForTenure(); } - ProxyObject* proxy = &obj->as(); - 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().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() ? &a->as() : nullptr; NativeObject* nb = b->is() ? &b->as() : 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() ? &a->as() : nullptr; ProxyObject* pb = b->is() ? &b->as() : 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(), kb, - avals)) { - oomUnsafe.crash("NativeObject::fillInSlotsAfterSwap"); + if (na && + !NativeObject::fixupAfterSwap(cx, b.as(), kb, avals)) { + oomUnsafe.crash("NativeObject::fixupAfterSwap"); } - if (nb && !NativeObject::fillInSlotsAfterSwap(cx, a.as(), ka, - bvals)) { - oomUnsafe.crash("NativeObject::fillInSlotsAfterSwap"); + if (nb && + !NativeObject::fixupAfterSwap(cx, a.as(), ka, bvals)) { + oomUnsafe.crash("NativeObject::fixupAfterSwap"); } - if (pa && !b->as().fillInValuesAfterSwap(cx, avals)) { - oomUnsafe.crash("ProxyObject::fillInValuesAfterSwap"); + if (pa && !b->as().fixupAfterSwap(cx, avals)) { + oomUnsafe.crash("ProxyObject::fixupAfterSwap"); } - if (pb && !a->as().fillInValuesAfterSwap(cx, bvals)) { - oomUnsafe.crash("ProxyObject::fillInValuesAfterSwap"); + if (pb && !a->as().fixupAfterSwap(cx, bvals)) { + oomUnsafe.crash("ProxyObject::fixupAfterSwap"); } } diff --git a/js/src/vm/NativeObject.h b/js/src/vm/NativeObject.h index d248764b748d..999ab2cc6200 100644 --- a/js/src/vm/NativeObject.h +++ b/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 diff --git a/js/src/vm/ProxyObject.cpp b/js/src/vm/ProxyObject.cpp index 8438ec1630a0..905d42abbef8 100644 --- a/js/src/vm/ProxyObject.cpp +++ b/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) { diff --git a/js/src/vm/ProxyObject.h b/js/src/vm/ProxyObject.h index 70918a811c79..f067883d99fc 100644 --- a/js/src/vm/ProxyObject.h +++ b/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); }