Refactor the ScriptManager #1191
Merged
+316
−483
Conversation
This file contains hidden or 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
This PR introduces two major changes to the ScriptManager. 1 - Simplification. Rather than having a `Script`, `PendingScript`, `AsyncModule` and `SyncModule`, there is only a `Script`, with an added `mode` union. All of the previous objects had the same behavior (collect the response in a buffer), up to the point of execution, which is where the mode comes in. 2 - Correctness Whether or not the previous version was "incorrect", it was difficult to use correctly. Specifically, the previous version would execute async scripts and async modules as soon as they're done. That seems allowed, but it caused issues with module loading in Context.js. Specifically, between compiling and instantiating a module, or between instantiation and evaluation, an async script or module could be evaluated. It isn't clear whether v8 allows that, but if it does, it introduces a lot of new potential states (specifically, unexpected changes to the v8.Module's status) that we have to handle. This version only evaluate scripts in the `evaluate`, which doesn't allow recursive calls (so a waitForImport, which continues to pump the HTTP loop, can never result in `evaluate` being called again). This undoes the change made in #1158 because I do not think it's possible to have multiple waiters waiting for the same (or even different) modules. The linked issue points to a crash in https://www.nytimes.com which doesn't crash with this version.
krichprollsch
approved these changes
Nov 3, 2025
| // So this is more like a cache. When an imported module is completed, its | ||
| // source is placed here (keyed by the full url) for some point in the future | ||
| // when v8 asks for it. | ||
| // The type is confusing (too confusing? move to a union). Starts of as `null` |
There was a problem hiding this comment.
the type seems OK for me :)
|
Well, I have a crash with https://www.xange.vc/ (not using fetch, but with cdp + cdpcli) EDIT: I reporduce after fetching https://www.nytimes.com/ and https://www.xange.vc/ |
|
@krichprollsch I was able to reproduce and fix the crash. |
|
I have another crash: |
karlseguin
added a commit
that referenced
this pull request
Nov 4, 2025
This does two changes to module loading. First, for normal imports, it only
instantiates and evaluates the top-level module. This ensures that circular
dependencies can be resolved. This bug was introduced when I tried to
deduplicate code between dynamic and normal modules - but it turns out that
non-top-level normal modules do have a simpler flow (they just need to be
compiled, and we let v8 deal with the rest).
The other change is to handle more edge cases. Code like this should now be ok:
```
<script type=module>
var a = await import('a.js');
</script>
<script type=module>
import a from a.js
</script>
```
Previously, the dynamic import of a.js (first block) could interact badly with
the normal import of a.js in the 2nd block.
This change is built on top of #1191
which also helps reduce the number of cases by ensure that a script isn't
evaluated while we're trying to evaluate a script.
Open
krichprollsch
approved these changes
Nov 4, 2025
Co-authored-by: Pierre Tachoire <pierre@lightpanda.io>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR introduces two major changes to the ScriptManager.
1 - Simplification.
Rather than having a
Script,PendingScript,AsyncModuleandSyncModule, there is only aScript, with an addedmodeunion. All of the previous objects had the same behavior (collect the response in a buffer), up to the point of execution, which is where the mode comes in.2 - Correctness
Whether or not the previous version was "incorrect", it was difficult to use correctly. Specifically, the previous version would execute async scripts and async modules as soon as they're done. That seems allowed, but it caused issues with module loading in Context.zig. Specifically, between compiling and instantiating a module, or between instantiation and evaluation, an async script or module could be evaluated. It isn't clear whether v8 allows that, but if it does, it introduces a lot of new potential states (specifically, unexpected changes to the v8.Module's status) that we have to handle.
This version only evaluate scripts in the
evaluate, which doesn't allow recursive calls (so a waitForImport, which continues to pump the HTTP loop, can never result inevaluatebeing called again).This undoes the change made in #1158 because I do not think it's possible to have multiple waiters waiting for the same (or even different) modules. The linked issue points to a crash in https://www.nytimes.com which doesn't crash with this version.