Closed Bug 1648453 Opened 2 years ago Closed 1 year ago

FinalizationRegistry should track the incumbent global for the callback function

Categories

(Core :: JavaScript: GC, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED

People

(Reporter: jonco, Assigned: jonco)

References

(Blocks 1 open bug)

Details

Attachments

(5 files)

As discussed elsewhere, FinalizationRegistry should behave the same as Promise and track the incumbent global.

Spec discussion of this issue:

https://github.com/whatwg/html/pull/4571/files/5d1c10025a1181f148cc75ab5d7eea777ca28cf4#diff-36cd38f49b9afa08222c0dc9ebfe35ebR88957-R88962

We are aiming for Shu's option 1, tracking the incumbent for both Promise and FinalizationRegistry.

Severity: -- → S4
Priority: -- → P1

This changes the way the the host triggers cleanup, from calling an API to calling a JS function. This is done so we can use the existing DOM infrastructure that handles setting up the incumbent global for us.

Assignee: nobody → jcoppeard
Status: NEW → ASSIGNED

This also factors out FinalizationRegistry support into a separate class.

The JS engine now passes a callback function and the incumbent global which are recorded in QueueCallback. FinalizationRegistryCleanup::DoCleanup creates a CallbackObject can calls it immediately (I originally tried creating it in QueueCallback but there were problems because this is called during GC).

I coped most of this from the way promise reaction jobs work. I added FinalizationRegistryCleanupCallback; I don't know if that's overkill.

Depends on D83613

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a9ad01b4ab2e
Record incumbent global in FinalizationRegistry constructor and use when calling back into the JS engine to call callbacks r=sfink
https://hg.mozilla.org/integration/autoland/rev/4cac71f274b8
Use CallbackObject to trigger cleanup while setting up the incumbent global r=smaug
https://hg.mozilla.org/integration/autoland/rev/8b21977bb2df
Add tests to check that FinalizationRegistry sets the incumbent global correctly when calling the callback r=smaug

These patches have triggered a case where a MacOS header defines 'check' as a macro which then conflicts with use of 'check' as a method name in AccessCheck.h, probably due to unified builds.

This was fixed independently in a couple of places before, but I think it makes sense to move the fix to AccessCheck.h itself.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/623252539387
Record incumbent global in FinalizationRegistry constructor and use when calling back into the JS engine to call callbacks r=sfink
https://hg.mozilla.org/integration/autoland/rev/0afcee198ecd
Use CallbackObject to trigger cleanup while setting up the incumbent global r=smaug
https://hg.mozilla.org/integration/autoland/rev/473e7d55a25e
Add tests to check that FinalizationRegistry sets the incumbent global correctly when calling the callback r=smaug
https://hg.mozilla.org/integration/autoland/rev/9598a75cca47
Fix build errors caused by MacOS headers defining check macro r=smaug
Flags: needinfo?(jcoppeard)
Keywords: leave-open

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 that can be problematic too 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.

Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ffa51e994ee8
Record incumbent global in FinalizationRegistry constructor and use when calling back into the JS engine to call callbacks r=sfink
https://hg.mozilla.org/integration/autoland/rev/13ee873ffdff
Use CallbackObject to trigger cleanup while setting up the incumbent global r=smaug
https://hg.mozilla.org/integration/autoland/rev/623bebcf0cd6
Add tests to check that FinalizationRegistry sets the incumbent global correctly when calling the callback r=smaug
https://hg.mozilla.org/integration/autoland/rev/0ebd8b6fce88
Fix build errors caused by MacOS headers defining check macro r=smaug
https://hg.mozilla.org/integration/autoland/rev/fcb43544e8a2
Don't try and store the incumbent global directly r=sfink
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Flags: needinfo?(jcoppeard)
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.