don't allow concurrent blocking calls

Author: karlseguin

src/browser/ScriptManager.zig

@@ -38,6 +38,9 @@ page: *Page,
 // used to prevent recursive evalution
 is_evaluating: bool,
 
+// used to prevent executing scripts while we're doing a blocking load
+is_blocking: bool = false,
+
 // Only once this is true can deferred scripts be run
 static_scripts_done: bool,
 
@@ -54,6 +57,7 @@ deferreds: OrderList,
 
 shutdown: bool = false,
 
+
 client: *HttpClient,
 allocator: Allocator,
 buffer_pool: BufferPool,
@@ -269,6 +273,17 @@ pub fn addFromElement(self: *ScriptManager, element: *parser.Element) !void {
 // http handle, these are executed without there necessarily being a free handle.
 // Thus, Http/Client.zig maintains a dedicated handle for these calls.
 pub fn blockingGet(self: *ScriptManager, url: [:0]const u8) !BlockingResult {
+    std.debug.assert(self.is_blocking == false);
+
+    self.is_blocking = true;
+    defer {
+        self.is_blocking = false;
+
+        // we blocked evaluation while loading this script, there could be
+        // scripts ready to process.
+        self.evaluate();
+    }
+
     var blocking = Blocking{
         .allocator = self.allocator,
         .buffer_pool = &self.buffer_pool,
@@ -314,6 +329,13 @@ fn evaluate(self: *ScriptManager) void {
         return;
     }
 
+    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.
+        return;
+    }
+
     const page = self.page;
     self.is_evaluating = true;
     defer self.is_evaluating = false;

src/browser/session.zig

@@ -118,13 +118,15 @@ pub const Session = struct {
 
         std.debug.assert(self.page != null);
 
-        self.page.?.deinit();
-        self.page = null;
-
         // RemoveJsContext() will execute the destructor of any type that
         // registered a destructor (e.g. XMLHttpRequest).
+        // Should be called before we deinit the page, because these objects
+        // could be referencing it.
         self.executor.removeJsContext();
 
+        self.page.?.deinit();
+        self.page = null;
+
         // clear netsurf memory arena.
         parser.deinit();
 

src/browser/xhr/xhr.zig

@@ -171,12 +171,12 @@ pub const XMLHttpRequest = struct {
         };
     }
 
-    // pub fn destructor(self: *XMLHttpRequest) void {
-    //     if (self.transfer) |transfer| {
-    //         transfer.abort();
-    //         self.transfer = null;
-    //     }
-    // }
+    pub fn destructor(self: *XMLHttpRequest) void {
+        if (self.transfer) |transfer| {
+            transfer.abort();
+            self.transfer = null;
+        }
+    }
 
     pub fn reset(self: *XMLHttpRequest) void {
         self.url = null;

src/http/Client.zig

@@ -74,10 +74,6 @@ transfer_pool: std.heap.MemoryPool(Transfer),
 // see ScriptManager.blockingGet
 blocking: Handle,
 
-// Boolean to check that we don't make a blocking request while an existing
-// blocking request is already being processed.
-blocking_active: if (builtin.mode == .Debug) bool else void = if (builtin.mode == .Debug) false else {},
-
 // The only place this is meant to be used is in `makeRequest` BEFORE `perform`
 // is called. It is used to generate our Cookie header. It can be used for other
 // purposes, but keep in mind that, while single-threaded, calls like makeRequest
@@ -199,14 +195,6 @@ pub fn request(self: *Client, req: Request) !void {
 
 // See ScriptManager.blockingGet
 pub fn blockingRequest(self: *Client, req: Request) !void {
-    if (comptime builtin.mode == .Debug) {
-        std.debug.assert(self.blocking_active == false);
-        self.blocking_active = true;
-    }
-    defer if (comptime builtin.mode == .Debug) {
-        self.blocking_active = false;
-    };
-
     return self.makeRequest(&self.blocking, req);
 }
 
@@ -361,7 +349,7 @@ fn endTransfer(self: *Client, transfer: *Transfer) void {
     self.transfer_pool.destroy(transfer);
 
     errorMCheck(c.curl_multi_remove_handle(self.multi, handle.conn.easy)) catch |err| {
-        log.fatal(.http, "Failed to abort", .{ .err = err });
+        log.fatal(.http, "Failed to remove handle", .{ .err = err });
     };
 
     self.handles.release(handle);
@@ -372,11 +360,6 @@ fn ensureNoActiveConnection(self: *const Client) !void {
     if (self.active > 0) {
         return error.InflightConnection;
     }
-    if (comptime builtin.mode == .Debug) {
-        if (self.blocking_active) {
-            return error.InflightConnection;
-        }
-    }
 }
 
 const Handles = struct {
@@ -429,8 +412,8 @@ const Handles = struct {
     }
 
     fn release(self: *Handles, handle: *Handle) void {
-        // client.blocking is a handle without a node, it doesn't exist in the
-        // eitehr the in_use or available lists.
+        // client.blocking is a handle without a node, it doesn't exist in
+        // either the in_use or available lists.
         const node = &(handle.node orelse return);
 
         self.in_use.remove(node);