Fix blockingGet during blockingGet

ScriptManager should only ever has one in-flight blockingGet. The is_blocking
flag is used to assert this, as well as enforce it in evaluate(). If is_blocking
is true, evaluate() exits.

This doesn't work for async scripts however, as they aren't executed via
evaluate(), but rather execute directly once complete.

This PR changes the execution behavior of async scripts. They are now only
executed in evaluate() (and thus won't execute when is_blocking == true).
However, unlike normal/deferred scripts, async scripts continue to execute in
their completion order (not their declared order).

Fixes #1016

Author: karlseguin

src/browser/ScriptManager.zig

@@ -35,7 +35,7 @@ const ScriptManager = @This();
 
 page: *Page,
 
-// used to prevent recursive evalution
+// used to prevent recursive evalutaion
 is_evaluating: bool,
 
 // used to prevent executing scripts while we're doing a blocking load
@@ -45,10 +45,16 @@ is_blocking: bool = false,
 static_scripts_done: bool,
 
 // List of async scripts. We don't care about the execution order of these, but
-// on shutdown/abort, we need to co cleanup any pending ones.
+// on shutdown/abort, we need to cleanup any pending ones.
 asyncs: OrderList,
 
-// Normal scripts (non-deffered & non-async). These must be executed ni order
+// When an async script is ready to be evaluated, it's moved from asyncs to
+// this list. You might think we can evaluate an async script as soon as it's
+// done, but we can only evaluate scripts when `is_blocking == false`. So this
+// becomes a list of scripts to execute on the next evaluate().
+asyncs_ready: OrderList,
+
+// Normal scripts (non-deferred & non-async). These must be executed in order
 scripts: OrderList,
 
 // List of deferred scripts. These must be executed in order, but only once
@@ -72,6 +78,7 @@ pub fn init(browser: *Browser, page: *Page) ScriptManager {
         .asyncs = .{},
         .scripts = .{},
         .deferreds = .{},
+        .asyncs_ready = .{},
         .is_evaluating = false,
         .allocator = allocator,
         .client = browser.http_client,
@@ -91,6 +98,7 @@ pub fn reset(self: *ScriptManager) void {
     self.clearList(&self.asyncs);
     self.clearList(&self.scripts);
     self.clearList(&self.deferreds);
+    self.clearList(&self.asyncs_ready);
     self.static_scripts_done = false;
 }
 
@@ -114,7 +122,7 @@ pub fn addFromElement(self: *ScriptManager, element: *parser.Element) !void {
     //    document.getElementsByTagName('head')[0].appendChild(script)
     // that script tag will immediately get executed by our scriptAddedCallback.
     // However, if the location where the script tag is inserted happens to be
-    // below where processHTMLDoc curently is, then we'll re-run that same script
+    // below where processHTMLDoc currently is, then we'll re-run that same script
     // again in processHTMLDoc. This flag is used to let us know if a specific
     // <script> has already been processed.
     if (try parser.scriptGetProcessed(@ptrCast(element))) {
@@ -169,7 +177,7 @@ pub fn addFromElement(self: *ScriptManager, element: *parser.Element) !void {
     if (source == .@"inline" and self.scripts.first == null) {
         // inline script with no pending scripts, execute it immediately.
         // (if there is a pending script, then we cannot execute this immediately
-        // as it needs to be executed in order)
+        // as it needs to best executed in order)
         return script.eval(page);
     }
 
@@ -193,7 +201,7 @@ pub fn addFromElement(self: *ScriptManager, element: *parser.Element) !void {
         log.debug(.http, "script queue", .{ .url = remote_url.? });
     }
 
-    self.getList(&pending_script.script).append(&pending_script.node);
+    pending_script.getList().append(&pending_script.node);
 
     errdefer pending_script.deinit();
 
@@ -313,14 +321,21 @@ fn evaluate(self: *ScriptManager) void {
     if (self.is_blocking) {
         // Cannot evaluate scripts while a blocking-load is in progress. Not
         // only could that result in incorrect evaluation order, it could
-        // triger another blocking request, while we're doing a blocking request.
+        // trigger another blocking request, while we're doing a blocking request.
         return;
     }
 
     const page = self.page;
     self.is_evaluating = true;
     defer self.is_evaluating = false;
 
+    // every script in asyncs_ready is ready to be evaluated.
+    while (self.asyncs_ready.first) |n| {
+        var pending_script: *PendingScript = @fieldParentPtr("node", n);
+        defer pending_script.deinit();
+        pending_script.script.eval(page);
+    }
+
     while (self.scripts.first) |n| {
         var pending_script: *PendingScript = @fieldParentPtr("node", n);
         if (pending_script.complete == false) {
@@ -355,7 +370,6 @@ fn evaluate(self: *ScriptManager) void {
     page.documentIsLoaded();
 
     if (self.asyncs.first == null) {
-        // if we're here, then its like `asyncDone`
         // 1 - there are no async scripts pending
         // 2 - we checkecked static_scripts_done == true above
         // 3 - we drained self.scripts above
@@ -371,28 +385,6 @@ pub fn isDone(self: *const ScriptManager) bool {
         self.deferreds.first == null; // and there are no more <script defer src=> to wait for
 }
 
-fn asyncDone(self: *ScriptManager) void {
-    if (self.isDone()) {
-        // then the document is considered complete
-        self.page.documentIsComplete();
-    }
-}
-
-fn getList(self: *ScriptManager, script: *const Script) *OrderList {
-    // When a script has both the async and defer flag set, it should be
-    // treated as async. Async is newer, so some websites use both so that
-    // if async isn't known, it'll fallback to defer.
-    if (script.is_async) {
-        return &self.asyncs;
-    }
-
-    if (script.is_defer) {
-        return &self.deferreds;
-    }
-
-    return &self.scripts;
-}
-
 fn startCallback(transfer: *Http.Transfer) !void {
     const script: *PendingScript = @ptrCast(@alignCast(transfer.ctx));
     script.startCallback(transfer) catch |err| {
@@ -441,12 +433,12 @@ pub const PendingScript = struct {
         if (script.source == .remote) {
             manager.buffer_pool.release(script.source.remote);
         }
-        manager.getList(script).remove(&self.node);
+        self.getList().remove(&self.node);
     }
 
     fn remove(self: *PendingScript) void {
         if (self.node) |*node| {
-            self.manager.getList(&self.script).remove(node);
+            self.getList().remove(node);
             self.node = null;
         }
     }
@@ -500,15 +492,12 @@ pub const PendingScript = struct {
         log.debug(.http, "script fetch complete", .{ .req = self.script.url });
 
         const manager = self.manager;
+        self.complete = true;
         if (self.script.is_async) {
-            // async script can be evaluated immediately
-            self.script.eval(self.manager.page);
-            self.deinit();
-            manager.asyncDone();
-        } else {
-            self.complete = true;
-            manager.evaluate();
+            manager.asyncs.remove(&self.node);
+            manager.asyncs_ready.append(&self.node);
         }
+        manager.evaluate();
     }
 
     fn errorCallback(self: *PendingScript, err: anyerror) void {
@@ -524,6 +513,23 @@ pub const PendingScript = struct {
 
         manager.evaluate();
     }
+
+    fn getList(self: *const PendingScript) *OrderList {
+        // When a script has both the async and defer flag set, it should be
+        // treated as async. Async is newer, so some websites use both so that
+        // if async isn't known, it'll fallback to defer.
+
+        const script = &self.script;
+        if (script.is_async) {
+            return if (self.complete) &self.manager.asyncs_ready else &self.manager.asyncs;
+        }
+
+        if (script.is_defer) {
+            return &self.manager.deferreds;
+        }
+
+        return &self.manager.scripts;
+    }
 };
 
 const Script = struct {