mirror of
https://github.com/lightpanda-io/browser.git
synced 2026-02-04 06:23:45 +00:00
Add defensiveness around Parser.appendCallback
We're seeing an assertion in Page.appendNew fail because the node has a parent. According to html5ever, this shouldn't be possible (appendNew is only called from the Parser). BUT, it's possible we're mutating the node in a way that we shouldn't...maybe there's JavaScript executing as we're parsing which is mutating the node. In release, this will be more defensive. In debug, this still crashes. It's possible this is valid (like I said, maybe there's JS interleaved which is mutating the node), but if so, I'd like to know the exact scenario that produces this case.
This commit is contained in:
@@ -24,6 +24,7 @@ const Page = @import("../Page.zig");
|
|||||||
const Node = @import("../webapi/Node.zig");
|
const Node = @import("../webapi/Node.zig");
|
||||||
const Element = @import("../webapi/Element.zig");
|
const Element = @import("../webapi/Element.zig");
|
||||||
const Allocator = std.mem.Allocator;
|
const Allocator = std.mem.Allocator;
|
||||||
|
const IS_DEBUG = @import("builtin").mode == .Debug;
|
||||||
|
|
||||||
pub const ParsedNode = struct {
|
pub const ParsedNode = struct {
|
||||||
node: *Node,
|
node: *Node,
|
||||||
@@ -373,6 +374,17 @@ fn _appendCallback(self: *Parser, parent: *Node, node_or_text: h5e.NodeOrText) !
|
|||||||
switch (node_or_text.toUnion()) {
|
switch (node_or_text.toUnion()) {
|
||||||
.node => |cpn| {
|
.node => |cpn| {
|
||||||
const child = getNode(cpn);
|
const child = getNode(cpn);
|
||||||
|
if (child._parent) |previous_parent| {
|
||||||
|
// html5ever says this can't happen, but we might be screwing up
|
||||||
|
// the node on our side. We shouldn't be, but we're seeing this
|
||||||
|
// in the wild, and I'm not sure why. In debug, let's crash so
|
||||||
|
// we can try to figure it out. In release, let's disconnect
|
||||||
|
// the child first.
|
||||||
|
if (comptime IS_DEBUG) {
|
||||||
|
unreachable;
|
||||||
|
}
|
||||||
|
self._page.removeNode(previous_parent, child, .{ .will_be_reconnected = parent.isConnected() });
|
||||||
|
}
|
||||||
try self.page.appendNew(parent, .{ .node = child });
|
try self.page.appendNew(parent, .{ .node = child });
|
||||||
},
|
},
|
||||||
.text => |txt| try self.page.appendNew(parent, .{ .text = txt }),
|
.text => |txt| try self.page.appendNew(parent, .{ .text = txt }),
|
||||||
|
|||||||
@@ -356,7 +356,7 @@ test "cdp Node: Registry register" {
|
|||||||
}
|
}
|
||||||
|
|
||||||
{
|
{
|
||||||
const dom_node = (try doc.querySelector(.wrap ("p"), page)).?.asNode();
|
const dom_node = (try doc.querySelector(.wrap("p"), page)).?.asNode();
|
||||||
const node = try registry.register(dom_node);
|
const node = try registry.register(dom_node);
|
||||||
const n1b = registry.lookup_by_id.get(2).?;
|
const n1b = registry.lookup_by_id.get(2).?;
|
||||||
const n1c = registry.lookup_by_node.get(node.dom).?;
|
const n1c = registry.lookup_by_node.get(node.dom).?;
|
||||||
@@ -400,18 +400,18 @@ test "cdp Node: search list" {
|
|||||||
defer page._session.removePage();
|
defer page._session.removePage();
|
||||||
var doc = page.window._document;
|
var doc = page.window._document;
|
||||||
|
|
||||||
const s1 = try search_list.create((try doc.querySelectorAll(.wrap ("a"), page))._nodes);
|
const s1 = try search_list.create((try doc.querySelectorAll(.wrap("a"), page))._nodes);
|
||||||
try testing.expectEqual("1", s1.name);
|
try testing.expectEqual("1", s1.name);
|
||||||
try testing.expectEqualSlices(u32, &.{ 1, 2 }, s1.node_ids);
|
try testing.expectEqualSlices(u32, &.{ 1, 2 }, s1.node_ids);
|
||||||
|
|
||||||
try testing.expectEqual(2, registry.lookup_by_id.count());
|
try testing.expectEqual(2, registry.lookup_by_id.count());
|
||||||
try testing.expectEqual(2, registry.lookup_by_node.count());
|
try testing.expectEqual(2, registry.lookup_by_node.count());
|
||||||
|
|
||||||
const s2 = try search_list.create((try doc.querySelectorAll(.wrap ("#a1"), page))._nodes);
|
const s2 = try search_list.create((try doc.querySelectorAll(.wrap("#a1"), page))._nodes);
|
||||||
try testing.expectEqual("2", s2.name);
|
try testing.expectEqual("2", s2.name);
|
||||||
try testing.expectEqualSlices(u32, &.{1}, s2.node_ids);
|
try testing.expectEqualSlices(u32, &.{1}, s2.node_ids);
|
||||||
|
|
||||||
const s3 = try search_list.create((try doc.querySelectorAll(.wrap ("#a2"), page))._nodes);
|
const s3 = try search_list.create((try doc.querySelectorAll(.wrap("#a2"), page))._nodes);
|
||||||
try testing.expectEqual("3", s3.name);
|
try testing.expectEqual("3", s3.name);
|
||||||
try testing.expectEqualSlices(u32, &.{2}, s3.node_ids);
|
try testing.expectEqualSlices(u32, &.{2}, s3.node_ids);
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user