Skip to content

Cdp node children #502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 8, 2025
Merged

Cdp node children #502

merged 5 commits into from
Apr 8, 2025

Conversation

karlseguin
Copy link
Collaborator

Add child nodes to CDP registry.

Add custom CDP Node writer to deal with different messages requiring a different representation (i.e. differnent child depth) of a node.

Include direct descendant, with hooks for other serialization options.

Don't include parentId if null.
Add Node writer. Different CDP messages want different child depths. For now,
only support immediate children, but the new writer should make it easy to
support variable.
@krichprollsch
Copy link
Member

I think we have 2 issues with this approach:

  1. if you register the root element, you will register the whole DOM tree in cascade, even if you want to return only the root and its direct chlidren.
  2. the DOM tree is dynamic, so the children list for an element can change during the page's lifetime

I think we shouldn't store the children of the node into the registry but compute and return them on demand, with the correct depth.


Moreover the current code generates a crash.
Indeed, filling the children betweeen the first self.lookup_by_node.getOrPut(self.allocator, n) and the final node_lookup_gop.value_ptr.* = node; leads to never fill the final value if lookup_by_node is reallocated in the meantime.

General protection exception (no address available)
/home/pierre/wrk/browser-review/src/cdp/Node.zig:229:58: 0x20db67d in create (lightpanda)
                node_id.* = (try registry.register(node)).id;
                                                         ^
/home/pierre/wrk/browser-review/src/cdp/domains/dom.zig:72:50: 0x20dbffa in performSearch__anon_899731 (lightpanda)
    const search = try bc.node_search_list.create(list.nodes.items);
                                                 ^
/home/pierre/wrk/browser-review/src/cdp/domains/dom.zig:36:47: 0x20dc39d in processMessage__anon_899463 (lightpanda)
        .performSearch => return performSearch(cmd),

this patch fix the issue:

diff --git i/src/cdp/Node.zig w/src/cdp/Node.zig
index ed682a0..7cdb34f 100644
--- i/src/cdp/Node.zig
+++ w/src/cdp/Node.zig
@@ -94,14 +94,16 @@ pub const Registry = struct {
         // but, just in case, let's try to keep things tidy.
         errdefer _ = self.lookup_by_node.remove(n);
 
+        const node = try self.node_pool.create();
+        errdefer self.node_pool.destroy(node);
+
+        node_lookup_gop.value_ptr.* = node;
+
         const id = self.node_id;
         self.node_id = id + 1;
 
         const child_nodes = try self.registerChildNodes(n);
 
-        const node = try self.node_pool.create();
-        errdefer self.node_pool.destroy(node);
-
         node.* = .{
             ._node = n,
             .id = id,
@@ -125,7 +127,6 @@ pub const Registry = struct {
         //     // TODO
         // }
 
-        node_lookup_gop.value_ptr.* = node;
         try self.lookup_by_id.putNoClobber(self.allocator, id, node);
         return node;
     }

Node registry now only tracks the node id (which we need to be consistent) and
the underlying parser.Node. All other data is loaded on-demand (i.e. when we
serialize the node). This allows us to serialize node values as they appear
when they are serialized, as opposed to when they are registered.
@karlseguin
Copy link
Collaborator Author

Not just the children, other properties, like the value!

I changed the CDP Node to just be the id: Id and the underlying _node: *parser.Node. Everything else, including the children, is loaded ondemand when we serialize the object.

Copy link
Contributor

@sjorsdonkers sjorsdonkers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
I suppose later we can pass in the ParentID, no need to fetch and register it, except for the top level nodes.

@karlseguin
Copy link
Collaborator Author

LGTM I suppose later we can pass in the ParentID, no need to fetch and register it, except for the top level nodes.

There's a netsurf function to get the parent node, so it would be pretty easy to add now. However, there's this DOM.setChildNodes event (https://chromedevtools.github.io/devtools-protocol/tot/DOM/#event-setChildNodes) that we might have to raise. If we do, then we'd need to do that whenever we get a parent. So I'm waiting to see if we do, in fact, need to raise that event, before dealing with the parent.

@sjorsdonkers sjorsdonkers mentioned this pull request Apr 7, 2025
@sjorsdonkers
Copy link
Contributor

I was working in an older version of this branch patched with the fix mention here:
#502 (comment)

After pulling the latest changes I get a panic: runtime error: invalid memory address or nil pointer dereference

./cdpcli dump http://127.0.0.1:1234/campfire-commerce/

I will investigate tomorrow

@karlseguin
Copy link
Collaborator Author

I was working in an older version of this branch patched with the fix mention here: #502 (comment)

After pulling the latest changes I get a panic: runtime error: invalid memory address or nil pointer dereference

./cdpcli dump http://127.0.0.1:1234/campfire-commerce/

I will investigate tomorrow

Fixed here:
0139437

@krichprollsch krichprollsch merged commit 3c2b348 into main Apr 8, 2025
12 checks passed
@krichprollsch krichprollsch deleted the cdp_node_children branch April 8, 2025 13:45
@github-actions github-actions bot locked and limited conversation to collaborators Apr 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants