Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix ES module deserialization #942

Merged
merged 1 commit into from
Mar 6, 2025
Merged

Conversation

bnoordhuis
Copy link
Contributor

A module's imports are not serialized along with the module itself and that left the deserialized module with dangling references. Fix that by checking the module cache first, the module loader second.

A glaring problem with cache checking is that the cached module doesn't have to be the module it was at the time of serialization.

Why not call out to the module loader right away? Because then a module can get loaded twice and that's arguably even worse.

The alternative of serializing modules transitively doesn't work for C modules and is also prone to loading the same module twice.

Fixes: #913


As you can probably tell, I don't think it's a great fix, merely the least terrible.

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something tells me this might backfire, but I don't have a better suggestion 😅 Maybe leave a comment in the code?

A module's imports are not serialized along with the module itself and
that left the deserialized module with dangling references. Fix that by
checking the module cache first, the module loader second.

A glaring problem with cache checking is that the cached module doesn't
have to be the module it was at the time of serialization.

Why not call out to the module loader right away? Because then a module
can get loaded twice and that's arguably even worse.

The alternative of serializing modules transitively doesn't work for
C modules and is also prone to loading the same module twice.

Fixes: quickjs-ng#913
@bnoordhuis
Copy link
Contributor Author

Done.

@bnoordhuis bnoordhuis merged commit ff5cbb8 into quickjs-ng:master Mar 6, 2025
38 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Null pointer is accessed when calling JS_EvalFunction
2 participants