Commit 632921f1 authored by Jon Coppeard's avatar Jon Coppeard
Browse files

Bug 1600488 - Allow FinalizationGroup targets to be unregistered after they...

Bug 1600488 - Allow FinalizationGroup targets to be unregistered after they have been queued for cleanup, in accordance with the spec r=sfink

The fix is to queue pointers to finalization records rather than just holdings when the target dies.  These are still in the resgistration map and so can be cleared by unregister().  We detect this in the iterator's next() method and skip any such records.

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

--HG--
extra : moz-landing-system : lando
parent 029fc78a
...@@ -53,6 +53,11 @@ Value FinalizationRecordObject::holdings() const { ...@@ -53,6 +53,11 @@ Value FinalizationRecordObject::holdings() const {
return getReservedSlot(HoldingsSlot); return getReservedSlot(HoldingsSlot);
} }
bool FinalizationRecordObject::wasCleared() const {
MOZ_ASSERT_IF(!group(), holdings().isUndefined());
return !group();
}
void FinalizationRecordObject::clear() { void FinalizationRecordObject::clear() {
MOZ_ASSERT(group()); MOZ_ASSERT(group());
setReservedSlot(GroupSlot, NullValue()); setReservedSlot(GroupSlot, NullValue());
...@@ -83,7 +88,7 @@ const JSClassOps FinalizationRecordVectorObject::classOps_ = { ...@@ -83,7 +88,7 @@ const JSClassOps FinalizationRecordVectorObject::classOps_ = {
/* static */ /* static */
FinalizationRecordVectorObject* FinalizationRecordVectorObject::create( FinalizationRecordVectorObject* FinalizationRecordVectorObject::create(
JSContext* cx) { JSContext* cx) {
auto records = cx->make_unique<RecordVector>(cx->zone()); auto records = cx->make_unique<FinalizationRecordVector>(cx->zone());
if (!records) { if (!records) {
return nullptr; return nullptr;
} }
...@@ -114,14 +119,13 @@ void FinalizationRecordVectorObject::finalize(JSFreeOp* fop, JSObject* obj) { ...@@ -114,14 +119,13 @@ void FinalizationRecordVectorObject::finalize(JSFreeOp* fop, JSObject* obj) {
fop->delete_(obj, rv->records(), MemoryUse::FinalizationRecordVector); fop->delete_(obj, rv->records(), MemoryUse::FinalizationRecordVector);
} }
inline FinalizationRecordVectorObject::RecordVector* inline FinalizationRecordVector* FinalizationRecordVectorObject::records() {
FinalizationRecordVectorObject::records() { return static_cast<FinalizationRecordVector*>(privatePtr());
return static_cast<RecordVector*>(privatePtr());
} }
inline const FinalizationRecordVectorObject::RecordVector* inline const FinalizationRecordVector* FinalizationRecordVectorObject::records()
FinalizationRecordVectorObject::records() const { const {
return static_cast<const RecordVector*>(privatePtr()); return static_cast<const FinalizationRecordVector*>(privatePtr());
} }
inline void* FinalizationRecordVectorObject::privatePtr() const { inline void* FinalizationRecordVectorObject::privatePtr() const {
...@@ -225,8 +229,8 @@ bool FinalizationGroupObject::construct(JSContext* cx, unsigned argc, ...@@ -225,8 +229,8 @@ bool FinalizationGroupObject::construct(JSContext* cx, unsigned argc,
return false; return false;
} }
Rooted<UniquePtr<HoldingsVector>> holdings( Rooted<UniquePtr<FinalizationRecordVector>> holdings(
cx, cx->make_unique<HoldingsVector>(cx->zone())); cx, cx->make_unique<FinalizationRecordVector>(cx->zone()));
if (!holdings) { if (!holdings) {
return false; return false;
} }
...@@ -240,8 +244,8 @@ bool FinalizationGroupObject::construct(JSContext* cx, unsigned argc, ...@@ -240,8 +244,8 @@ bool FinalizationGroupObject::construct(JSContext* cx, unsigned argc,
group->initReservedSlot(CleanupCallbackSlot, ObjectValue(*cleanupCallback)); group->initReservedSlot(CleanupCallbackSlot, ObjectValue(*cleanupCallback));
InitReservedSlot(group, RegistrationsSlot, registrations.release(), InitReservedSlot(group, RegistrationsSlot, registrations.release(),
MemoryUse::FinalizationGroupRegistrations); MemoryUse::FinalizationGroupRegistrations);
InitReservedSlot(group, HoldingsToBeCleanedUpSlot, holdings.release(), InitReservedSlot(group, RecordsToBeCleanedUpSlot, holdings.release(),
MemoryUse::FinalizationGroupHoldingsVector); MemoryUse::FinalizationGroupRecordVector);
group->initReservedSlot(IsQueuedForCleanupSlot, BooleanValue(false)); group->initReservedSlot(IsQueuedForCleanupSlot, BooleanValue(false));
group->initReservedSlot(IsCleanupJobActiveSlot, BooleanValue(false)); group->initReservedSlot(IsCleanupJobActiveSlot, BooleanValue(false));
...@@ -252,7 +256,7 @@ bool FinalizationGroupObject::construct(JSContext* cx, unsigned argc, ...@@ -252,7 +256,7 @@ bool FinalizationGroupObject::construct(JSContext* cx, unsigned argc,
/* static */ /* static */
void FinalizationGroupObject::trace(JSTracer* trc, JSObject* obj) { void FinalizationGroupObject::trace(JSTracer* trc, JSObject* obj) {
auto group = &obj->as<FinalizationGroupObject>(); auto group = &obj->as<FinalizationGroupObject>();
if (HoldingsVector* holdings = group->holdingsToBeCleanedUp()) { if (FinalizationRecordVector* holdings = group->recordsToBeCleanedUp()) {
holdings->trace(trc); holdings->trace(trc);
} }
if (ObjectWeakMap* registrations = group->registrations()) { if (ObjectWeakMap* registrations = group->registrations()) {
...@@ -263,8 +267,8 @@ void FinalizationGroupObject::trace(JSTracer* trc, JSObject* obj) { ...@@ -263,8 +267,8 @@ void FinalizationGroupObject::trace(JSTracer* trc, JSObject* obj) {
/* static */ /* static */
void FinalizationGroupObject::finalize(JSFreeOp* fop, JSObject* obj) { void FinalizationGroupObject::finalize(JSFreeOp* fop, JSObject* obj) {
auto group = &obj->as<FinalizationGroupObject>(); auto group = &obj->as<FinalizationGroupObject>();
fop->delete_(obj, group->holdingsToBeCleanedUp(), fop->delete_(obj, group->recordsToBeCleanedUp(),
MemoryUse::FinalizationGroupHoldingsVector); MemoryUse::FinalizationGroupRecordVector);
fop->delete_(obj, group->registrations(), fop->delete_(obj, group->registrations(),
MemoryUse::FinalizationGroupRegistrations); MemoryUse::FinalizationGroupRegistrations);
} }
...@@ -285,13 +289,13 @@ ObjectWeakMap* FinalizationGroupObject::registrations() const { ...@@ -285,13 +289,13 @@ ObjectWeakMap* FinalizationGroupObject::registrations() const {
return static_cast<ObjectWeakMap*>(value.toPrivate()); return static_cast<ObjectWeakMap*>(value.toPrivate());
} }
FinalizationGroupObject::HoldingsVector* FinalizationRecordVector* FinalizationGroupObject::recordsToBeCleanedUp()
FinalizationGroupObject::holdingsToBeCleanedUp() const { const {
Value value = getReservedSlot(HoldingsToBeCleanedUpSlot); Value value = getReservedSlot(RecordsToBeCleanedUpSlot);
if (value.isUndefined()) { if (value.isUndefined()) {
return nullptr; return nullptr;
} }
return static_cast<HoldingsVector*>(value.toPrivate()); return static_cast<FinalizationRecordVector*>(value.toPrivate());
} }
bool FinalizationGroupObject::isQueuedForCleanup() const { bool FinalizationGroupObject::isQueuedForCleanup() const {
...@@ -302,11 +306,11 @@ bool FinalizationGroupObject::isCleanupJobActive() const { ...@@ -302,11 +306,11 @@ bool FinalizationGroupObject::isCleanupJobActive() const {
return getReservedSlot(IsCleanupJobActiveSlot).toBoolean(); return getReservedSlot(IsCleanupJobActiveSlot).toBoolean();
} }
void FinalizationGroupObject::queueHoldingsToBeCleanedUp( void FinalizationGroupObject::queueRecordToBeCleanedUp(
const Value& holdings) { FinalizationRecordObject* record) {
AutoEnterOOMUnsafeRegion oomUnsafe; AutoEnterOOMUnsafeRegion oomUnsafe;
if (!holdingsToBeCleanedUp()->append(holdings)) { if (!recordsToBeCleanedUp()->append(record)) {
oomUnsafe.crash("FinalizationGroupObject::queueHoldingsToBeCleanedUp"); oomUnsafe.crash("FinalizationGroupObject::queueRecordsToBeCleanedUp");
} }
} }
...@@ -501,20 +505,33 @@ bool FinalizationGroupObject::unregister(JSContext* cx, unsigned argc, ...@@ -501,20 +505,33 @@ bool FinalizationGroupObject::unregister(JSContext* cx, unsigned argc,
RootedObject unregisterToken(cx, &args[0].toObject()); RootedObject unregisterToken(cx, &args[0].toObject());
// 5. Let removed be false.
bool removed = false;
// 6. For each Record { [[Target]], [[Holdings]], [[UnregisterToken]] } cell
// that is an element of finalizationGroup.[[Cells]], do
// a. If SameValue(cell.[[UnregisterToken]], unregisterToken) is true, then
// i. Remove cell from finalizationGroup.[[Cells]].
// ii. Set removed to true.
RootedObject obj(cx, group->registrations()->lookup(unregisterToken)); RootedObject obj(cx, group->registrations()->lookup(unregisterToken));
if (obj) { if (obj) {
auto* records = obj->as<FinalizationRecordVectorObject>().records(); auto* records = obj->as<FinalizationRecordVectorObject>().records();
MOZ_ASSERT(records); MOZ_ASSERT(records);
MOZ_ASSERT(!records->empty()); MOZ_ASSERT(!records->empty());
for (FinalizationRecordObject* record : *records) { for (FinalizationRecordObject* record : *records) {
// Clear the fields of this record; it will be removed from the target's if (!record->wasCleared()) {
// list when it is next swept. // Clear the fields of this record; it will be removed from the target's
record->clear(); // list when it is next swept.
record->clear();
removed = true;
}
} }
group->registrations()->remove(unregisterToken); group->registrations()->remove(unregisterToken);
} }
args.rval().setBoolean(bool(obj)); // 7. Return removed.
args.rval().setBoolean(removed);
return true; return true;
} }
...@@ -557,7 +574,7 @@ bool FinalizationGroupObject::cleanupSome(JSContext* cx, unsigned argc, ...@@ -557,7 +574,7 @@ bool FinalizationGroupObject::cleanupSome(JSContext* cx, unsigned argc,
} }
} }
if (!cleanupQueuedHoldings(cx, group, cleanupCallback)) { if (!cleanupQueuedRecords(cx, group, cleanupCallback)) {
return false; return false;
} }
...@@ -568,14 +585,14 @@ bool FinalizationGroupObject::cleanupSome(JSContext* cx, unsigned argc, ...@@ -568,14 +585,14 @@ bool FinalizationGroupObject::cleanupSome(JSContext* cx, unsigned argc,
// CleanupFinalizationGroup ( finalizationGroup [ , callback ] ) // CleanupFinalizationGroup ( finalizationGroup [ , callback ] )
// https://tc39.es/proposal-weakrefs/#sec-cleanup-finalization-group // https://tc39.es/proposal-weakrefs/#sec-cleanup-finalization-group
/* static */ /* static */
bool FinalizationGroupObject::cleanupQueuedHoldings( bool FinalizationGroupObject::cleanupQueuedRecords(
JSContext* cx, HandleFinalizationGroupObject group, JSContext* cx, HandleFinalizationGroupObject group,
HandleObject callbackArg) { HandleObject callbackArg) {
MOZ_ASSERT(cx->compartment() == group->compartment()); MOZ_ASSERT(cx->compartment() == group->compartment());
// 2. If CheckForEmptyCells(finalizationGroup) is false, return. // 2. If CheckForEmptyCells(finalizationGroup) is false, return.
HoldingsVector* holdings = group->holdingsToBeCleanedUp(); FinalizationRecordVector* records = group->recordsToBeCleanedUp();
size_t initialLength = holdings->length(); size_t initialLength = records->length();
if (initialLength == 0) { if (initialLength == 0) {
return true; return true;
} }
...@@ -607,12 +624,12 @@ bool FinalizationGroupObject::cleanupQueuedHoldings( ...@@ -607,12 +624,12 @@ bool FinalizationGroupObject::cleanupQueuedHoldings(
RootedValue rval(cx); RootedValue rval(cx);
bool ok = Call(cx, callback, UndefinedHandleValue, iteratorVal, &rval); bool ok = Call(cx, callback, UndefinedHandleValue, iteratorVal, &rval);
// Remove holdings that were iterated over. // Remove records that were iterated over.
size_t index = iterator->index(); size_t index = iterator->index();
MOZ_ASSERT(index <= holdings->length()); MOZ_ASSERT(index <= records->length());
MOZ_ASSERT(initialLength <= holdings->length()); MOZ_ASSERT(initialLength <= records->length());
if (index > 0) { if (index > 0) {
holdings->erase(holdings->begin(), holdings->begin() + index); records->erase(records->begin(), records->begin() + index);
} }
// 7. Set finalizationGroup.[[IsFinalizationGroupCleanupJobActive]] to false. // 7. Set finalizationGroup.[[IsFinalizationGroupCleanupJobActive]] to false.
...@@ -696,10 +713,9 @@ size_t FinalizationIteratorObject::index() const { ...@@ -696,10 +713,9 @@ size_t FinalizationIteratorObject::index() const {
return size_t(i); return size_t(i);
} }
void FinalizationIteratorObject::incIndex() { void FinalizationIteratorObject::setIndex(size_t i) {
int32_t i = index(); MOZ_ASSERT(i <= INT32_MAX);
MOZ_ASSERT(i < INT32_MAX); setReservedSlot(IndexSlot, Int32Value(int32_t(i)));
setReservedSlot(IndexSlot, Int32Value(i + 1));
} }
void FinalizationIteratorObject::clearFinalizationGroup() { void FinalizationIteratorObject::clearFinalizationGroup() {
...@@ -742,17 +758,27 @@ bool FinalizationIteratorObject::next(JSContext* cx, unsigned argc, Value* vp) { ...@@ -742,17 +758,27 @@ bool FinalizationIteratorObject::next(JSContext* cx, unsigned argc, Value* vp) {
// a. Choose any such cell. // a. Choose any such cell.
// b. Remove cell from finalizationGroup.[[Cells]]. // b. Remove cell from finalizationGroup.[[Cells]].
// c. Return CreateIterResultObject(cell.[[Holdings]], false). // c. Return CreateIterResultObject(cell.[[Holdings]], false).
auto* holdings = group->holdingsToBeCleanedUp(); FinalizationRecordVector* records = group->recordsToBeCleanedUp();
size_t index = iterator->index(); size_t index = iterator->index();
MOZ_ASSERT(index <= holdings->length()); MOZ_ASSERT(index <= records->length());
if (index < holdings->length() && index < INT32_MAX) {
RootedValue value(cx, (*holdings)[index]); // Advance until we find a record that hasn't been unregistered.
JSObject* result = CreateIterResultObject(cx, value, false); while (index < records->length() && index < INT32_MAX &&
(*records)[index]->wasCleared()) {
index++;
iterator->setIndex(index);
}
if (index < records->length() && index < INT32_MAX) {
RootedFinalizationRecordObject record(cx, (*records)[index]);
RootedValue holdings(cx, record->holdings());
JSObject* result = CreateIterResultObject(cx, holdings, false);
if (!result) { if (!result) {
return false; return false;
} }
iterator->incIndex(); record->clear();
iterator->setIndex(index + 1);
args.rval().setObject(*result); args.rval().setObject(*result);
return true; return true;
......
...@@ -102,24 +102,27 @@ class FinalizationRecordObject : public NativeObject { ...@@ -102,24 +102,27 @@ class FinalizationRecordObject : public NativeObject {
FinalizationGroupObject* group() const; FinalizationGroupObject* group() const;
Value holdings() const; Value holdings() const;
bool wasCleared() const;
void clear(); void clear();
}; };
// A JS object containing a vector of finalization records. Used as the values // A vector of FinalizationRecordObjects.
// in the registration weakmap. using FinalizationRecordVector =
GCVector<HeapPtr<FinalizationRecordObject*>, 1, js::ZoneAllocPolicy>;
// A JS object that wraps a FinalizationRecordVector. Used as the values in the
// registration weakmap.
class FinalizationRecordVectorObject : public NativeObject { class FinalizationRecordVectorObject : public NativeObject {
enum { RecordsSlot = 0, SlotCount }; enum { RecordsSlot = 0, SlotCount };
public: public:
using RecordVector =
GCVector<HeapPtr<FinalizationRecordObject*>, 1, js::ZoneAllocPolicy>;
static const JSClass class_; static const JSClass class_;
static FinalizationRecordVectorObject* create(JSContext* cx); static FinalizationRecordVectorObject* create(JSContext* cx);
RecordVector* records(); FinalizationRecordVector* records();
const RecordVector* records() const; const FinalizationRecordVector* records() const;
bool isEmpty() const; bool isEmpty() const;
...@@ -140,31 +143,29 @@ class FinalizationGroupObject : public NativeObject { ...@@ -140,31 +143,29 @@ class FinalizationGroupObject : public NativeObject {
enum { enum {
CleanupCallbackSlot = 0, CleanupCallbackSlot = 0,
RegistrationsSlot, RegistrationsSlot,
HoldingsToBeCleanedUpSlot, RecordsToBeCleanedUpSlot,
IsQueuedForCleanupSlot, IsQueuedForCleanupSlot,
IsCleanupJobActiveSlot, IsCleanupJobActiveSlot,
SlotCount SlotCount
}; };
public: public:
using HoldingsVector = GCVector<HeapPtrValue, 0, ZoneAllocPolicy>;
static const JSClass class_; static const JSClass class_;
static const JSClass protoClass_; static const JSClass protoClass_;
JSObject* cleanupCallback() const; JSObject* cleanupCallback() const;
ObjectWeakMap* registrations() const; ObjectWeakMap* registrations() const;
HoldingsVector* holdingsToBeCleanedUp() const; FinalizationRecordVector* recordsToBeCleanedUp() const;
bool isQueuedForCleanup() const; bool isQueuedForCleanup() const;
bool isCleanupJobActive() const; bool isCleanupJobActive() const;
void queueHoldingsToBeCleanedUp(const Value& holdings); void queueRecordToBeCleanedUp(FinalizationRecordObject* record);
void setQueuedForCleanup(bool value); void setQueuedForCleanup(bool value);
void setCleanupJobActive(bool value); void setCleanupJobActive(bool value);
static bool cleanupQueuedHoldings(JSContext* cx, static bool cleanupQueuedRecords(JSContext* cx,
HandleFinalizationGroupObject group, HandleFinalizationGroupObject group,
HandleObject callback = nullptr); HandleObject callback = nullptr);
private: private:
static const JSClassOps classOps_; static const JSClassOps classOps_;
...@@ -203,7 +204,7 @@ class FinalizationIteratorObject : public NativeObject { ...@@ -203,7 +204,7 @@ class FinalizationIteratorObject : public NativeObject {
FinalizationGroupObject* finalizationGroup() const; FinalizationGroupObject* finalizationGroup() const;
size_t index() const; size_t index() const;
void incIndex(); void setIndex(size_t index);
void clearFinalizationGroup(); void clearFinalizationGroup();
private: private:
......
...@@ -70,7 +70,7 @@ void GCRuntime::sweepFinalizationGroups(Zone* zone) { ...@@ -70,7 +70,7 @@ void GCRuntime::sweepFinalizationGroups(Zone* zone) {
auto record = &obj->as<FinalizationRecordObject>(); auto record = &obj->as<FinalizationRecordObject>();
FinalizationGroupObject* group = record->group(); FinalizationGroupObject* group = record->group();
if (group) { if (group) {
group->queueHoldingsToBeCleanedUp(record->holdings()); group->queueRecordToBeCleanedUp(record);
queueFinalizationGroupForCleanup(group); queueFinalizationGroupForCleanup(group);
} }
} }
...@@ -106,6 +106,6 @@ void GCRuntime::queueFinalizationGroupForCleanup( ...@@ -106,6 +106,6 @@ void GCRuntime::queueFinalizationGroupForCleanup(
bool GCRuntime::cleanupQueuedFinalizationGroup( bool GCRuntime::cleanupQueuedFinalizationGroup(
JSContext* cx, HandleFinalizationGroupObject group) { JSContext* cx, HandleFinalizationGroupObject group) {
group->setQueuedForCleanup(false); group->setQueuedForCleanup(false);
bool ok = FinalizationGroupObject::cleanupQueuedHoldings(cx, group); bool ok = FinalizationGroupObject::cleanupQueuedRecords(cx, group);
return ok; return ok;
} }
...@@ -138,7 +138,7 @@ enum class ZealMode { ...@@ -138,7 +138,7 @@ enum class ZealMode {
_(DebuggerOnPopHandler) \ _(DebuggerOnPopHandler) \
_(RealmInstrumentation) \ _(RealmInstrumentation) \
_(ICUObject) \ _(ICUObject) \
_(FinalizationGroupHoldingsVector) \ _(FinalizationGroupRecordVector) \
_(FinalizationGroupRegistrations) \ _(FinalizationGroupRegistrations) \
_(FinalizationRecordVector) \ _(FinalizationRecordVector) \
_(ZoneAllocPolicy) _(ZoneAllocPolicy)
......
// |jit-test| --enable-weak-refs
const token = {};
let iterated;
const finalizationGroup = new FinalizationGroup(items => {
iterated = items.next().value;
});
{
let object = {};
finalizationGroup.register(object, token, token);
object = undefined;
}
gc();
finalizationGroup.cleanupSome();
assertEq(iterated, token);
assertEq(finalizationGroup.unregister(token), false);
// |jit-test| --enable-weak-refs
const token = {};
let iterated;
const finalizationGroup = new FinalizationGroup(items => {
iterated = items.next().value;
});
{
let object = {};
finalizationGroup.register(object, token, token);
object = undefined;
}
gc();
assertEq(finalizationGroup.unregister(token), true);
finalizationGroup.cleanupSome();
assertEq(iterated, undefined);
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment