script: Remember which nodes the devtools know about

[simonwuelker]

Currently, script and devtools use a node's unique id to identify it across requests. The unique ID is part of a node's rare data field and is really only meant for debugging. Instantiating it on a node causes it's memory usage to go up significantly. Now, when the devtools ask for information about a specific Node then they send the unique ID to script, and the script thread then walks the whole DOM tree searching for that specific ID. This happens here:

servo/components/script/devtools.rs

Lines 142 to 153 in 6d0b651

	fn find_node_by_unique_id(
	documents: &DocumentCollection,
	pipeline: PipelineId,
	node_id: &str,
	) -> Option<DomRoot<Node>> {
	documents.find_document(pipeline).and_then(|document| {
	document
	.upcast::<Node>()
	.traverse_preorder(ShadowIncluding::Yes)
	.find(|candidate| candidate.unique_id(pipeline) == node_id)
	})
	}

So, in the worst case, all of the nodes in the tree now have a unique ID. That's not great!

Also, when script notifies devtools about changes to a DOM node then devtools expects a NodeActor to exist for that Node. The actor might not exist if the inspector doesn't know about that node yet - that happens when the user hasn't expanded their parent yet.
That is an oversight from #42601 and causes problems now(#42784) because for the longest time, devtools was mostly the one sending requests. Of course, we could make devtools simply ignore updates for nodes that it doesn't know about, but ideally we shouldn't send these updates in the first place.

This change implements a lookup map on the ScriptThread that contains all nodes that have been sent to the devtools inspector. The map allows us to efficiently resolve a unique ID to a Node in O(1), without creating unique IDs for the whole tree. It also allows us to only send DOM updates for nodes that the inspector cares about.

For now, entries from the cache are not evicted unless the relevant pipeline is closed. That reflects reality, because the inspector also keeps using them forever.
In the future we will tell the inspector when nodes are removed from the tree - then it can't interact with them anymore, and we can remove them from the script-side map.

This is change is not all that complicated but it involves moving a lot of code around, so feel free to ask for clarification when something is unclear!

Testing: This change adds a test
Fixes part of #42784

[eerii]

I will properly review this later today, but we have tried this locally and it fixes the crashes and it seems to work great :) thank you so much!

[eerii]

This is great! :) Just a few nits.

I am wondering whether it is best to have all of the functions be members of DevtoolsState, or to pass DevtoolsState as a parameter to the functions that are called from script thread (i.e. handle_highlight_dom_node), and keep find_node_by_unique_id and similar in DevtoolsState. Not saying I have a preference for any of them, I am wondering what do you think it's best, specially thinking of future maintainability.

[eerii]

This is a very small nit, feel free to keep it like this, but PipelineState sounds better to me. This would also apply to per_pipeline_state.

[simonwuelker]

I am wondering whether it is best to have all of the functions be members of DevtoolsState, or to pass DevtoolsState as a parameter to the functions that are called from script thread (i.e. handle_highlight_dom_node), and keep find_node_by_unique_id and similar in DevtoolsState. Not saying I have a preference for any of them, I am wondering what do you think it's best, specially thinking of future maintainability.

I think I slightly prefer your version for consistency, so I've adapted the code accordingly.