Improve module loading

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.

Author: karlseguin

src/browser/ScriptManager.zig

@@ -392,6 +392,15 @@ pub fn getAsyncImport(self: *ScriptManager, url: [:0]const u8, cb: ImportAsync.C
         .stack = self.page.js.stackTrace() catch "???",
     });
 
+    // It's possible, but unlikely, for client.request to immediately finish
+    // a request, thus calling our callback. We generally don't want a call
+    // from v8 (which is why we're here), to result in a new script evaluation.
+    // So we block even the slightest change that `client.request` immediately
+    // executes a callback.
+    const was_evaluating = self.is_evaluating;
+    self.is_evaluating = true;
+    defer self.is_evaluating = was_evaluating;
+
     try self.client.request(.{
         .url = url,
         .method = .GET,

src/browser/js/Context.zig

@@ -222,63 +222,54 @@ pub fn exec(self: *Context, src: []const u8, name: ?[]const u8) !js.Value {
 }
 
 pub fn module(self: *Context, comptime want_result: bool, src: []const u8, url: []const u8, cacheable: bool) !(if (want_result) ModuleEntry else void) {
-    if (cacheable) {
-        if (self.module_cache.get(url)) |entry| {
-            // The dynamic import will create an entry without the
-            // module to prevent multiple calls from asynchronously
-            // loading the same module. If we're here, without the
-            // module, then it's time to load it.
-            if (entry.module != null) {
-                return if (comptime want_result) entry else {};
+    const mod, const owned_url = blk: {
+        const arena = self.arena;
+
+        // gop will _always_ initiated if cacheable == true
+        var gop: std.StringHashMapUnmanaged(ModuleEntry).GetOrPutResult = undefined;
+        if (cacheable) {
+            gop = try self.module_cache.getOrPut(arena, url);
+            if (gop.found_existing) {
+                if (gop.value_ptr.module != null) {
+                    return if (comptime want_result) gop.value_ptr.* else {};
+                }
+            } else {
+                // first time seing this
+                gop.value_ptr.* = .{};
             }
         }
-    }
-
-    const m = try compileModule(self.isolate, src, url);
 
-    const arena = self.arena;
-    const owned_url = try arena.dupe(u8, url);
+        const m = try compileModule(self.isolate, src, url);
+        const owned_url = try arena.dupeZ(u8, url);
 
-    try self.module_identifier.putNoClobber(arena, m.getIdentityHash(), owned_url);
-    errdefer _ = self.module_identifier.remove(m.getIdentityHash());
+        if (cacheable) {
+            // compileModule is synchronous - nothing can modify the cache during compilation
+            std.debug.assert(gop.value_ptr.module == null);
 
-    const v8_context = self.v8_context;
-    {
-        // Non-async modules are blocking. We can download them in
-        // parallel, but they need to be processed serially. So we
-        // want to get the list of dependent modules this module has
-        // and start downloading them asap.
-        const requests = m.getModuleRequests();
-        for (0..requests.length()) |i| {
-            const req = requests.get(v8_context, @intCast(i)).castTo(v8.ModuleRequest);
-            const specifier = try self.jsStringToZig(req.getSpecifier(), .{});
-            const normalized_specifier = try self.script_manager.?.resolveSpecifier(
-                self.call_arena,
-                specifier,
-                owned_url,
-            );
-            const gop = try self.module_cache.getOrPut(self.arena, normalized_specifier);
+            gop.value_ptr.module = PersistentModule.init(self.isolate, m);
             if (!gop.found_existing) {
-                const owned_specifier = try self.arena.dupeZ(u8, normalized_specifier);
-                gop.key_ptr.* = owned_specifier;
-                gop.value_ptr.* = .{};
-                try self.script_manager.?.preloadImport(owned_specifier, url);
+                gop.key_ptr.* = owned_url;
             }
         }
-    }
 
-    if (try m.instantiate(v8_context, resolveModuleCallback) == false) {
+        break :blk .{ m, owned_url };
+    };
+
+    try self.postCompileModule(mod, owned_url);
+
+    const v8_context = self.v8_context;
+    if (try mod.instantiate(v8_context, resolveModuleCallback) == false) {
         return error.ModuleInstantiationError;
     }
 
-    const evaluated = m.evaluate(v8_context) catch {
-        std.debug.assert(m.getStatus() == .kErrored);
+    const evaluated = mod.evaluate(v8_context) catch {
+        std.debug.assert(mod.getStatus() == .kErrored);
 
         // Some module-loading errors aren't handled by TryCatch. We need to
         // get the error from the module itself.
         log.warn(.js, "evaluate module", .{
             .specifier = owned_url,
-            .message = self.valueToString(m.getException(), .{}) catch "???",
+            .message = self.valueToString(mod.getException(), .{}) catch "???",
         });
         return error.EvaluationError;
     };
@@ -301,28 +292,46 @@ pub fn module(self: *Context, comptime want_result: bool, src: []const u8, url:
     // be cached
     std.debug.assert(cacheable);
 
-    const persisted_module = PersistentModule.init(self.isolate, m);
-    const persisted_promise = PersistentPromise.init(self.isolate, .{ .handle = evaluated.handle });
-
-    var gop = try self.module_cache.getOrPut(arena, owned_url);
-    if (gop.found_existing) {
-        // If we're here, it's because we had a cache entry, but no
-        // module. This happens because both our synch and async
-        // module loaders create the entry to prevent concurrent
-        // loads of the same resource (like Go's Singleflight).
-        std.debug.assert(gop.value_ptr.module == null);
-        std.debug.assert(gop.value_ptr.module_promise == null);
-
-        gop.value_ptr.module = persisted_module;
-        gop.value_ptr.module_promise = persisted_promise;
-    } else {
-        gop.value_ptr.* = ModuleEntry{
-            .module = persisted_module,
-            .module_promise = persisted_promise,
-            .resolver_promise = null,
-        };
+    // entry has to have been created atop this function
+    const entry = self.module_cache.getPtr(owned_url).?;
+
+    // and the module must have been set after we compiled it
+    std.debug.assert(entry.module != null);
+    std.debug.assert(entry.module_promise == null);
+
+    entry.module_promise = PersistentPromise.init(self.isolate, .{ .handle = evaluated.handle });
+    return if (comptime want_result) entry.* else {};
+}
+
+// After we compile a module, whether it's a top-level one, or a nested one,
+// we always want to track its identity (so that, if this module imports other
+// modules, we can resolve the full URL), and preload any dependent modules.
+fn postCompileModule(self: *Context, mod: v8.Module, url: [:0]const u8) !void {
+    try self.module_identifier.putNoClobber(self.arena, mod.getIdentityHash(), url);
+
+    const v8_context = self.v8_context;
+
+    // Non-async modules are blocking. We can download them in parallel, but
+    // they need to be processed serially. So we want to get the list of
+    // dependent modules this module has and start downloading them asap.
+    const requests = mod.getModuleRequests();
+    const script_manager = self.script_manager.?;
+    for (0..requests.length()) |i| {
+        const req = requests.get(v8_context, @intCast(i)).castTo(v8.ModuleRequest);
+        const specifier = try self.jsStringToZig(req.getSpecifier(), .{});
+        const normalized_specifier = try script_manager.resolveSpecifier(
+            self.call_arena,
+            specifier,
+            url,
+        );
+        const nested_gop = try self.module_cache.getOrPut(self.arena, normalized_specifier);
+        if (!nested_gop.found_existing) {
+            const owned_specifier = try self.arena.dupeZ(u8, normalized_specifier);
+            nested_gop.key_ptr.* = owned_specifier;
+            nested_gop.value_ptr.* = .{};
+            try script_manager.preloadImport(owned_specifier, url);
+        }
     }
-    return if (comptime want_result) gop.value_ptr.* else {};
 }
 
 // == Creators ==
@@ -1189,31 +1198,14 @@ fn _resolveModuleCallback(self: *Context, referrer: v8.Module, specifier: []cons
     };
 
     const normalized_specifier = try self.script_manager.?.resolveSpecifier(
-        self.arena, // might need to survive until the module is loaded
+        self.arena,
         specifier,
         referrer_path,
     );
 
-    const gop = try self.module_cache.getOrPut(self.arena, normalized_specifier);
-    if (gop.found_existing) {
-        if (gop.value_ptr.module) |m| {
-            return m.handle;
-        }
-        // We don't have a module, but we do have a cache entry for it
-        // That means we're already trying to load it. We just have
-        // to wait for it to be done.
-    } else {
-        // I don't think it's possible for us to be here. This is
-        // only ever called by v8 when we evaluate a module. But
-        // before evaluating, we should have already started
-        // downloading all of the module's nested modules. So it
-        // should be impossible that this is the first time we've
-        // heard about this module.
-        // But, I'm not confident enough in that, and ther's little
-        // harm in handling this case.
-        @branchHint(.unlikely);
-        gop.value_ptr.* = .{};
-        try self.script_manager.?.preloadImport(normalized_specifier, referrer_path);
+    const entry = self.module_cache.getPtr(normalized_specifier).?;
+    if (entry.module) |m| {
+        return m.castToModule().handle;
     }
 
     var source = try self.script_manager.?.waitForImport(normalized_specifier);
@@ -1223,26 +1215,10 @@ fn _resolveModuleCallback(self: *Context, referrer: v8.Module, specifier: []cons
     try_catch.init(self);
     defer try_catch.deinit();
 
-    const entry = self.module(true, source.src(), normalized_specifier, true) catch |err| {
-        switch (err) {
-            error.EvaluationError => {
-                // This is a sentinel value telling us that the error was already
-                // logged. Some module-loading errors aren't captured by Try/Catch.
-                // We need to handle those errors differently, where the module
-                // exists.
-            },
-            else => log.warn(.js, "compile resolved module", .{
-                .specifier = normalized_specifier,
-                .stack = try_catch.stack(self.call_arena) catch null,
-                .src = try_catch.sourceLine(self.call_arena) catch "err",
-                .line = try_catch.sourceLineNumber() orelse 0,
-                .exception = (try_catch.exception(self.call_arena) catch @errorName(err)) orelse @errorName(err),
-            }),
-        }
-        return null;
-    };
-    // entry.module is always set when returning from self.module()
-    return entry.module.?.handle;
+    const mod = try compileModule(self.isolate, source.src(), normalized_specifier);
+    try self.postCompileModule(mod, normalized_specifier);
+    entry.module = PersistentModule.init(self.isolate, mod);
+    return entry.module.?.castToModule().handle;
 }
 
 // Will get passed to ScriptManager and then passed back to us when
@@ -1317,7 +1293,31 @@ fn _dynamicModuleCallback(self: *Context, specifier: [:0]const u8, referrer: []c
     // `dynamicModuleSourceCallback`, but we can skip some steps
     // since the module is alrady loaded,
     std.debug.assert(gop.value_ptr.module != null);
-    std.debug.assert(gop.value_ptr.module_promise != null);
+
+    // If the module hasn't been evaluated yet (it was only instantiated
+    // as a static import dependency), we need to evaluate it now.
+    if (gop.value_ptr.module_promise == null) {
+        const mod = gop.value_ptr.module.?.castToModule();
+        if (mod.getStatus() == .kEvaluated) {
+            // Module was already evaluated (shouldn't normally happen, but handle it).
+            // Create a pre-resolved promise with the module namespace.
+            const persisted_module_resolver = v8.Persistent(v8.PromiseResolver).init(isolate, v8.PromiseResolver.init(self.v8_context));
+            try self.persisted_promise_resolvers.append(self.arena, persisted_module_resolver);
+            var module_resolver = persisted_module_resolver.castToPromiseResolver();
+            _ = module_resolver.resolve(self.v8_context, mod.getModuleNamespace());
+            gop.value_ptr.module_promise = PersistentPromise.init(self.isolate, module_resolver.getPromise());
+        } else {
+            // the module was loaded, but not evaluated, we _have_ to evaluate it now
+            const evaluated = mod.evaluate(self.v8_context) catch {
+                std.debug.assert(mod.getStatus() == .kErrored);
+                const error_msg = v8.String.initUtf8(isolate, "Module evaluation failed");
+                _ = resolver.reject(self.v8_context, error_msg.toValue());
+                return promise;
+            };
+            std.debug.assert(evaluated.isPromise());
+            gop.value_ptr.module_promise = PersistentPromise.init(self.isolate, .{ .handle = evaluated.handle });
+        }
+    }
 
     // like before, we want to set this up so that if anything else
     // tries to load this module, it can just return our promise