FinalizationRegistry should track the incumbent global for the callback function
Categories
(Core :: JavaScript: GC, enhancement, P1)
Tracking
()
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:
We are aiming for Shu's option 1, tracking the incumbent for both Promise and FinalizationRegistry.
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 1•3 years ago
|
||
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.
|
||
Updated•3 years ago
|
|
|
Assignee | |
Comment 2•3 years ago
|
||
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
|
|
Assignee | |
Comment 3•3 years ago
|
||
Depends on D83614
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
|
||
Comment 5•3 years ago
|
||
Backed out 3 changesets (Bug 1648453) for causing bustages in AccessCheck.h
Backout link: https://hg.mozilla.org/integration/autoland/rev/135690ce0e61273da1a1feb2244c34a1d7434c55
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=309992408&repo=autoland&lineNumber=11656
|
|
Assignee | |
Comment 6•3 years ago
|
||
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
|
|
Assignee | |
Updated•3 years ago
|
|
|
Assignee | |
Comment 8•3 years ago
|
||
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.
|
||
Comment 9•3 years ago
|
||
Backed out for hazzard bustages.
Log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310044338&repo=autoland&lineNumber=73886
Backout: https://hg.mozilla.org/integration/autoland/rev/5b891bc9d106cee46ad8ef2dc85da684406ac785
|
||
Comment 10•3 years ago
|
||
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
|
||
Comment 11•3 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Updated•3 years ago
|
Description
•