Skip to content

Commit b1ca242

Browse files
committed
Terminate execution on internal navigation
Currently, if there's an internal navigation event, we continue to process the page normally. This has negative performance implication, and can result in user-scripts producing unexpected results. For example, imagine a page with a script that does. ```js if (x) { form.submit(); } reloadProduct(); ``` The call to `form.submit()` should stop the script from executing. And, if the page has 10 other <script> tags after this, they shouldn't be loaded nor executed. This code terminates the execution of the current script on an internal navigation event, and stops the rest of the page from load. While I believe this creates a more "correct" behavior, it also introduces new edge cases. There's no a period of time, between the termination being stopped and then being resumed, where executing code is not safe.
1 parent 97c769e commit b1ca242

File tree

2 files changed

+76
-31
lines changed

2 files changed

+76
-31
lines changed

src/browser/page.zig

Lines changed: 59 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -388,11 +388,15 @@ pub const Page = struct {
388388
// > immediately before the browser continues to parse the
389389
// > page.
390390
// https://developer.mozilla.org/en-US/docs/Web/HTML/Element/script#notes
391-
self.evalScript(&script);
391+
if (self.evalScript(&script) == false) {
392+
return;
393+
}
392394
}
393395

394396
for (defer_scripts.items) |*script| {
395-
self.evalScript(script);
397+
if (self.evalScript(script) == false) {
398+
return;
399+
}
396400
}
397401
// dispatch DOMContentLoaded before the transition to "complete",
398402
// at the point where all subresources apart from async script elements
@@ -402,7 +406,9 @@ pub const Page = struct {
402406

403407
// eval async scripts.
404408
for (async_scripts.items) |*script| {
405-
self.evalScript(script);
409+
if (self.evalScript(script) == false) {
410+
return;
411+
}
406412
}
407413

408414
try HTMLDocument.documentIsComplete(html_doc, self);
@@ -419,10 +425,13 @@ pub const Page = struct {
419425
);
420426
}
421427

422-
fn evalScript(self: *Page, script: *const Script) void {
423-
self.tryEvalScript(script) catch |err| {
424-
log.err(.js, "eval script error", .{ .err = err, .src = script.src });
428+
fn evalScript(self: *Page, script: *const Script) bool {
429+
self.tryEvalScript(script) catch |err| switch (err) {
430+
error.JsErr => {}, // already been logged with detail
431+
error.Terminated => return false,
432+
else => log.err(.js, "eval script error", .{ .err = err, .src = script.src }),
425433
};
434+
return true;
426435
}
427436

428437
// evalScript evaluates the src in priority.
@@ -436,29 +445,26 @@ pub const Page = struct {
436445
log.err(.browser, "clear document script", .{ .err = err });
437446
};
438447

439-
const src = script.src orelse {
448+
var script_source: ?[]const u8 = null;
449+
if (script.src) |src| {
450+
self.current_script = script;
451+
defer self.current_script = null;
452+
453+
// https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script
454+
script_source = (try self.fetchData(src, null)) orelse {
455+
// TODO If el's result is null, then fire an event named error at
456+
// el, and return
457+
return;
458+
};
459+
} else {
440460
// source is inline
441461
// TODO handle charset attribute
442-
if (try parser.nodeTextContent(parser.elementToNode(script.element))) |text| {
443-
try script.eval(self, text);
444-
}
445-
return;
446-
};
447-
448-
self.current_script = script;
449-
defer self.current_script = null;
450-
451-
// https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script
452-
const body = (try self.fetchData(src, null)) orelse {
453-
// TODO If el's result is null, then fire an event named error at
454-
// el, and return
455-
return;
456-
};
462+
script_source = try parser.nodeTextContent(parser.elementToNode(script.element));
463+
}
457464

458-
script.eval(self, body) catch |err| switch (err) {
459-
error.JsErr => {}, // nothing to do here.
460-
else => return err,
461-
};
465+
if (script_source) |ss| {
466+
try script.eval(self, ss);
467+
}
462468

463469
// TODO If el's from an external file is true, then fire an event
464470
// named load at el.
@@ -701,13 +707,18 @@ pub const Page = struct {
701707
.reason = opts.reason,
702708
});
703709
self.delayed_navigation = true;
704-
const arena = self.session.transfer_arena;
710+
711+
const session = self.session;
712+
const arena = session.transfer_arena;
705713
const navi = try arena.create(DelayedNavigation);
706714
navi.* = .{
707715
.opts = opts,
708-
.session = self.session,
716+
.session = session,
709717
.url = try self.url.resolve(arena, url),
710718
};
719+
720+
// In v8, this throws an exception which JS code cannot catch.
721+
session.executor.terminateExecution();
711722
_ = try self.loop.timeout(0, &navi.navigate_node);
712723
}
713724

@@ -822,6 +833,11 @@ const DelayedNavigation = struct {
822833
const initial = self.initial;
823834

824835
if (initial) {
836+
// Prior to schedule this task, we terminated excution to stop
837+
// the running script. If we don't resume it before doing a shutdown
838+
// we'll get an error.
839+
session.executor.resumeExecution();
840+
825841
session.removePage();
826842
_ = session.createPage() catch |err| {
827843
log.err(.browser, "delayed navigation page error", .{
@@ -985,9 +1001,17 @@ const Script = struct {
9851001
}
9861002
},
9871003
} catch {
1004+
if (page.delayed_navigation) {
1005+
return error.Terminated;
1006+
}
1007+
9881008
if (try try_catch.err(page.arena)) |msg| {
989-
log.warn(.user_script, "eval script", .{ .src = src, .err = msg });
1009+
log.warn(.user_script, "eval script", .{
1010+
.src = src,
1011+
.err = msg,
1012+
});
9901013
}
1014+
9911015
try self.executeCallback("onerror", page);
9921016
return error.JsErr;
9931017
};
@@ -1060,10 +1084,15 @@ fn timestamp() u32 {
10601084
// immediately.
10611085
pub export fn scriptAddedCallback(ctx: ?*anyopaque, element: ?*parser.Element) callconv(.C) void {
10621086
const self: *Page = @alignCast(@ptrCast(ctx.?));
1087+
if (self.delayed_navigation) {
1088+
// if we're planning on navigating to another page, don't run this script
1089+
return;
1090+
}
1091+
10631092
var script = Script.init(element.?, self) catch |err| {
10641093
log.warn(.browser, "script added init error", .{ .err = err });
10651094
return;
10661095
} orelse return;
10671096

1068-
self.evalScript(&script);
1097+
_ = self.evalScript(&script);
10691098
}

src/runtime/js.zig

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,14 @@ pub fn Env(comptime State: type, comptime WebApis: type) type {
472472
self.scope = null;
473473
_ = self.scope_arena.reset(.{ .retain_with_limit = SCOPE_ARENA_RETAIN });
474474
}
475+
476+
pub fn terminateExecution(self: *const ExecutionWorld) void {
477+
self.env.isolate.terminateExecution();
478+
}
479+
480+
pub fn resumeExecution(self: *const ExecutionWorld) void {
481+
self.env.isolate.cancelTerminateExecution();
482+
}
475483
};
476484

477485
const PersistentObject = v8.Persistent(v8.Object);
@@ -1562,7 +1570,15 @@ pub fn Env(comptime State: type, comptime WebApis: type) type {
15621570
}
15631571

15641572
pub fn send(self: *const Inspector, msg: []const u8) void {
1565-
self.session.dispatchProtocolMessage(self.isolate, msg);
1573+
// Can't assume there's an existing Scope (with its HandleScope)
1574+
// available when doing this. Pages (and thus Scopes) come and
1575+
// go, but CDP can keep sending messages.
1576+
const isolate = self.isolate;
1577+
var temp_scope: v8.HandleScope = undefined;
1578+
v8.HandleScope.init(&temp_scope, isolate);
1579+
defer temp_scope.deinit();
1580+
1581+
self.session.dispatchProtocolMessage(isolate, msg);
15661582
}
15671583

15681584
// From CDP docs

0 commit comments

Comments
 (0)