1600488 - FinalizationGroup unregister doesn't work correctly on empty cells

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Mathieu Hofman]

Attachment: unregister-during-cleanup.js

FinalizationGroup unregister doesn't work correctly when called with the token for an already emptied cell.

If the cell was already removed and its held value was yielded by the iterator, unregister returns true instead of false.

If the cell has not yet been removed, e.g. because the iterator wasn't fully consumed, unregister doesn't remove the cell and the held value is yielded in subsequent iterator.next() calls (either in current or following cleanup jobs) even though unregister returns true.

Comment 1

[Mathieu Hofman]

Attachment: unregister-after-cleanup.js

Here is the test case for unregister being called outside the cleanup callback.

Comment 2

[Mathieu Hofman]

Attachment: unregister-after-cleanup.js

Comment 3

[Mathieu Hofman]

Attachment: unregister-before-cleanup.js

Comment 4

[Mathieu Hofman]

I took a look at the implementation. The problem is that when an object is collected, the held value is extracted from the record list and put into a new vector without the token. At that point there is no connection anymore between the cell / held value and the unregister token, so unregister doesn't prevent yielding the held value, and yielding the held value doesn't remove the record for the registration.

I've updated the attachments with simplified tests that highlight both those effects.

Comment 5

[Mathieu Hofman]

My test "unregister-before-cleanup.js" is mostly equivalent to https://github.com/tc39/test262/blob/master/test/built-ins/FinalizationGroup/prototype/cleanupSome/cleanup-prevented-with-unregister.js which should fail as well, I'm just not sure how to run test262 with jsshell.

I'll create a PR for test262 to cover the other test (unregister for a removed cell).

Comment 6

[Mathieu Hofman]

Here is the PR on test262 https://github.com/tc39/test262/pull/2440

The PR also updates test/built-ins/FinalizationGroup/prototype/cleanupSome/cleanup-prevented-with-unregister.js
That test previously passed because it called unregister before the target was collected, which didn't capture enough cases where unregister should work.

Comment 7

[Jon Coppeard (:jonco)]

Thanks for the test cases.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 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.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/37b5be84c26c
Allow FinalizationGroup targets to be unregistered after they have been queued for cleanup, in accordance with the spec r=sfink

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/37b5be84c26c