gumjs: Fix crash in Module finalizers #987
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oleavr
force-pushed
the
fix/gumjs-module-finalizers
branch
3 times, most recently
from
c5155c7 to
a4214fe
Compare
Our QuickJS suspend/resume patch isn't elaborate enough to support usage from finalizers. Given that modules are created and destroyed in waves, however, it's probably a bit on the expensive side to suspend/resume JS execution during the destruction of each value. So to avoid both issues we defer the unref using an idle source. The better longer term solution will probably be to introduce a ModuleObserver of sorts that integrates with the platform's runtime loader and manages the lifetime of each Module. This will also allow us to emit signals anytime a Module is added/removed, which is a feature that a lot of agents would benefit from. Kudos to @mrmacete for reporting.
oleavr
force-pushed
the
fix/gumjs-module-finalizers
branch
3 times, most recently
from
f9e70dd to
5b8b6a5
Compare
By using a single lock for all Module objects. We should be able to revert this once we've improved our GLib static allocation cleanup patch to use a more suitable data structure for keeping track of mutexes. Kudos to @mrmacete for profiling.
oleavr
force-pushed
the
fix/gumjs-module-finalizers
branch
from
5b8b6a5 to
a68a566
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our QuickJS suspend/resume patch isn't elaborate enough to support usage from finalizers. Given that modules are created and destroyed in waves, however, it's probably a bit on the expensive side to suspend/resume JS execution during the destruction of each value. So to avoid both issues we defer the unref using an idle source.
The better longer term solution will probably be to introduce a ModuleObserver of sorts that integrates with the platform's runtime loader and manages the lifetime of each Module. This will also allow us to emit signals anytime a Module is added/removed, which is a feature that a lot of agents would benefit from.
Kudos to @mrmacete for reporting.