perf(fast-browser): batch value-free observation metadata

2026-09-21 13:18:18 -04:00
parent 4360f01947
commit 574e083e75
2 changed files with 301 additions and 54 deletions
@@ -21,7 +21,7 @@ import time
 import uuid
 import base64
 import re
-from dataclasses import dataclass
+from dataclasses import dataclass, replace
 from pathlib import Path
 from typing import Any, Iterable, Protocol
 from urllib.parse import urlsplit, urlunsplit
@@ -33,6 +33,7 @@ from urllib.parse import urlsplit, urlunsplit
 MAX_LINE_BYTES = 12 * 1024 * 1024
 MAX_OUTPUT_BYTES = 12 * 1024 * 1024
 MAX_CANDIDATES = 60
+MAX_METADATA_BATCH = 16
 MAX_OBSERVATIONS = 8
 MAX_STRING = 4_000
 MAX_NAME = 240
@@ -348,6 +349,50 @@ class BrowserWorker:
        observation_id = f"obs-{self._counter}-{secrets.token_hex(4)}"
        records: list[CandidateRecord] = []
        seen: set[int] = set()
+        batch_metadata = getattr(self.backend, "metadata_many", None)
+        pending: list[tuple[int, str, str]] = []
+
+        def consume(items: list[tuple[int, str, str]]) -> None:
+            if not items:
+                return
+            metadata_by_id: dict[int, ElementMetadata] = {}
+            if callable(batch_metadata):
+                try:
+                    result = batch_metadata([backend_id for backend_id, _role, _name in items])
+                except Exception:
+                    result = None
+                if isinstance(result, dict):
+                    for backend_id, meta in result.items():
+                        if type(backend_id) is int and isinstance(meta, ElementMetadata):
+                            metadata_by_id[backend_id] = meta
+            for backend_id, ax_role, ax_name in items:
+                meta = metadata_by_id.get(backend_id)
+                if meta is None or meta.backend_id != backend_id:
+                    try:
+                        meta = self.backend.metadata(backend_id)
+                    except Exception:
+                        continue
+                if not meta.connected or meta.backend_id != backend_id:
+                    continue
+                # Full AX discovery is authoritative for observation roles and
+                # click names. DOM metadata remains the only source for fill
+                # labels, so a field's current value cannot enter an observation.
+                observed_meta = replace(meta, ax_role=ax_role, ax_name=ax_name)
+                candidate_role = (ax_role or observed_meta.role).lower()
+                actions = _candidate_actions(observed_meta, candidate_role)
+                if not actions:
+                    continue
+                name = _safe_name(
+                    observed_meta.name if "fill" in actions
+                    else (ax_name or observed_meta.ax_name or observed_meta.name)
+                )
+                candidate_id = f"c-{len(records)}"
+                records.append(CandidateRecord(
+                    candidate_id, backend_id, candidate_role, name, actions, observed_meta
+                ))
+                if len(records) >= MAX_CANDIDATES:
+                    break
+
        for node in self.backend.accessibility_nodes():
            if len(records) >= MAX_CANDIDATES:
                break
@@ -362,25 +407,11 @@ class BrowserWorker:
            # Do not spend several CDP round trips on static text/layout nodes.
            if ax_role not in _FILL_ROLES | _CLICK_ROLES:
                continue
-            try:
-                meta = self.backend.metadata(backend_id)
-            except Exception:
-                continue
-            if not meta.connected or meta.backend_id != backend_id:
-                continue
-            candidate_role = (ax_role or meta.ax_role or meta.role).lower()
-            actions = _candidate_actions(meta, candidate_role)
-            if not actions:
-                continue
-            # For fields prefer an author-supplied label/placeholder returned by
-            # metadata.  AX names for a textbox must not accidentally echo its
-            # current value.  The real backend's metadata.name follows this rule.
-            # A field's AX name can include state/value text on some custom
-            # controls.  Never use it as a fallback for fill candidates.
-            name = _safe_name(meta.name if "fill" in actions else (meta.ax_name or meta.name or ax_name))
-            candidate_id = f"c-{len(records)}"
-            record = CandidateRecord(candidate_id, backend_id, candidate_role, name, actions, meta)
-            records.append(record)
+            pending.append((backend_id, ax_role, ax_name))
+            if len(pending) >= MAX_METADATA_BATCH:
+                consume(pending)
+                pending.clear()
+        consume(pending)
        snapshot = ObservationRecord(
            observation_id, tab["target_id"], frame_id, loader_id, tab["url"], tab["title"], tuple(records)
        )
@@ -426,7 +457,7 @@ class BrowserWorker:
            raise WorkerError("action is not allowed for candidate")
        current = self.backend.metadata(candidate.backend_id)
        current_role = (current.ax_role or current.role).lower()
-        current_name = current.name if action == "fill" else (current.name or current.ax_name)
+        current_name = current.name if action == "fill" else (current.ax_name or current.name)
        current_actions = _candidate_actions(current, current_role)
        if (
            not current.connected
@@ -547,6 +578,39 @@ class BrowserWorker:
        self.backend.close()


+_METADATA_FUNCTION = """function () {
+    const e = this;
+    const style = getComputedStyle(e);
+    const type = (e.getAttribute('type') || '').toLowerCase();
+    const text = node => node ? String(node.innerText || node.textContent || '').trim() : '';
+    const labelledBy = (e.getAttribute('aria-labelledby') || '').split(/\\s+/)
+        .map(id => id ? text(document.getElementById(id)) : '').filter(Boolean).join(' ');
+    const labels = e.labels ? [...e.labels].map(text).filter(Boolean).join(' ') : '';
+    const visibleControlText = ['BUTTON', 'A', 'SUMMARY'].includes(e.tagName) ? text(e) : '';
+    // Never read .value: for fill candidates this is user data.
+    const label = e.getAttribute('aria-label') || labelledBy || labels ||
+        (visibleControlText || e.getAttribute('placeholder') ||
+            e.getAttribute('name') || e.getAttribute('id') || '');
+    return {
+        tag: e.tagName.toLowerCase(), type, role: e.getAttribute('role') || '',
+        name: String(label).slice(0, 240), connected: !!e.isConnected,
+        mainFrame: window.top === window,
+        contenteditable: e.isContentEditable,
+        visible: !!(e.getClientRects().length && style.display !== 'none' &&
+            style.visibility !== 'hidden' && style.opacity !== '0' &&
+            e.getAttribute('aria-hidden') !== 'true'),
+        disabled: !!(e.disabled || e.getAttribute('aria-disabled') === 'true' ||
+            e.matches(':disabled'))
+    };
+}"""
+_METADATA_MANY_FUNCTION = (
+    "function (...nodes) {\n"
+    f"    const read = {_METADATA_FUNCTION};\n"
+    "    return nodes.map(node => node && typeof node === 'object' ? read.call(node) : null);\n"
+    "}"
+)
+
+
 class RealBrowserBackend:
    """Adapter over browser-harness; no CDP implementation is duplicated here."""

@@ -875,34 +939,20 @@ class RealBrowserBackend:
            except Exception:
                pass

-    def metadata(self, backend_id: int) -> ElementMetadata:
-        value = self._call(backend_id, """function () {
-            const e = this;
-            const style = getComputedStyle(e);
-            const type = (e.getAttribute('type') || '').toLowerCase();
-            const text = node => node ? String(node.innerText || node.textContent || '').trim() : '';
-            const labelledBy = (e.getAttribute('aria-labelledby') || '').split(/\\s+/)
-                .map(id => id ? text(document.getElementById(id)) : '').filter(Boolean).join(' ');
-            const labels = e.labels ? [...e.labels].map(text).filter(Boolean).join(' ') : '';
-            const visibleControlText = ['BUTTON', 'A', 'SUMMARY'].includes(e.tagName) ? text(e) : '';
-            // Never read .value: for fill candidates this is user data.
-            const label = e.getAttribute('aria-label') || labelledBy || labels ||
-                (visibleControlText || e.getAttribute('placeholder') ||
-                    e.getAttribute('name') || e.getAttribute('id') || '');
-            return {
-                tag: e.tagName.toLowerCase(), type, role: e.getAttribute('role') || '',
-                name: String(label).slice(0, 240), connected: !!e.isConnected,
-                mainFrame: window.top === window,
-                contenteditable: e.isContentEditable,
-                visible: !!(e.getClientRects().length && style.display !== 'none' &&
-                    style.visibility !== 'hidden' && style.opacity !== '0' &&
-                    e.getAttribute('aria-hidden') !== 'true'),
-                disabled: !!(e.disabled || e.getAttribute('aria-disabled') === 'true' ||
-                    e.matches(':disabled'))
-            };
-        }""")
+    @staticmethod
+    def _metadata_from_dom_value(backend_id: int, value: Any) -> ElementMetadata:
        if not isinstance(value, dict):
            raise WorkerError("element metadata unavailable")
+        return ElementMetadata(
+            backend_id, str(value.get("tag", "")), str(value.get("type", "")),
+            str(value.get("role", "")), " ".join(str(value.get("name", "")).split())[:MAX_NAME],
+            bool(value.get("visible")), bool(value.get("disabled")), bool(value.get("connected")),
+            bool(value.get("mainFrame", True)), bool(value.get("contenteditable", False)),
+        )
+
+    def metadata(self, backend_id: int) -> ElementMetadata:
+        value = self._call(backend_id, _METADATA_FUNCTION)
+        metadata = self._metadata_from_dom_value(backend_id, value)
        ax_role, ax_name = "", ""
        try:
            partial = self._helpers.cdp(
@@ -919,13 +969,66 @@ class RealBrowserBackend:
            # DOM metadata still provides safe labels; accessibility is a
            # refinement and not a reason to expose a broad CDP surface.
            pass
-        return ElementMetadata(
-            backend_id, str(value.get("tag", "")), str(value.get("type", "")),
-            str(value.get("role", "")), " ".join(str(value.get("name", "")).split())[:MAX_NAME],
-            bool(value.get("visible")), bool(value.get("disabled")), bool(value.get("connected")),
-            bool(value.get("mainFrame", True)), bool(value.get("contenteditable", False)),
-            ax_role, ax_name,
-        )
+        return replace(metadata, ax_role=ax_role, ax_name=ax_name)
+
+    def metadata_many(self, backend_ids: Iterable[int]) -> dict[int, ElementMetadata]:
+        """Read value-free DOM metadata for one bounded observation chunk.
+
+        The full AX tree is already the observation's source for role/name. This
+        seam only batches the DOM extraction; action revalidation continues to
+        use metadata(), including its fresh partial AX read.
+        """
+        ids = [backend_id for backend_id in backend_ids if type(backend_id) is int and backend_id > 0][:MAX_METADATA_BATCH]
+        if not ids:
+            return {}
+        object_group = f"fast-browser-metadata-{uuid.uuid4().hex}"
+        object_ids: list[str | None] = []
+        try:
+            for backend_id in ids:
+                try:
+                    resolved = self._helpers.cdp(
+                        "DOM.resolveNode", backendNodeId=backend_id, objectGroup=object_group
+                    )
+                except Exception:
+                    object_ids.append(None)
+                    continue
+                object_id = (resolved.get("object") or {}).get("objectId") if isinstance(resolved, dict) else None
+                object_ids.append(object_id if isinstance(object_id, str) else None)
+
+            target_object = next((object_id for object_id in object_ids if object_id is not None), None)
+            if target_object is None:
+                return {}
+            arguments = [
+                {"objectId": object_id} if object_id is not None else {"value": None}
+                for object_id in object_ids
+            ]
+            try:
+                response = self._helpers.cdp(
+                    "Runtime.callFunctionOn", objectId=target_object,
+                    functionDeclaration=_METADATA_MANY_FUNCTION, arguments=arguments,
+                    returnByValue=True, awaitPromise=False, objectGroup=object_group,
+                )
+            except Exception:
+                return {}
+            details = response.get("exceptionDetails") if isinstance(response, dict) else None
+            remote = response.get("result") if isinstance(response, dict) else None
+            values = remote.get("value") if isinstance(remote, dict) else None
+            if details or not isinstance(remote, dict) or remote.get("subtype") == "error" or not isinstance(values, list):
+                return {}
+            result: dict[int, ElementMetadata] = {}
+            for backend_id, value in zip(ids, values):
+                try:
+                    result[backend_id] = self._metadata_from_dom_value(backend_id, value)
+                except WorkerError:
+                    # Missing or malformed entries use the safe legacy path in
+                    # BrowserWorker.observe rather than being treated as absent.
+                    continue
+            return result
+        finally:
+            try:
+                self._helpers.cdp("Runtime.releaseObjectGroup", objectGroup=object_group)
+            except Exception:
+                pass

    def click(self, backend_id: int) -> None:
        box = self._helpers.cdp("DOM.getBoxModel", backendNodeId=backend_id)
@@ -103,6 +103,25 @@ class FakeBackend:
        pass


+class BatchFakeBackend(FakeBackend):
+    def __init__(self):
+        super().__init__()
+        self.metadata_many_calls = []
+        self.metadata_calls = []
+        self.batch_result = None
+
+    def metadata(self, backend_id):
+        self.metadata_calls.append(backend_id)
+        return super().metadata(backend_id)
+
+    def metadata_many(self, backend_ids):
+        ids = list(backend_ids)
+        self.metadata_many_calls.append(ids)
+        if self.batch_result is not None:
+            return self.batch_result
+        return {backend_id: self.elements[backend_id] for backend_id in ids if backend_id in self.elements}
+
+
 class PopupFakeBackend(FakeBackend):
    def __init__(self):
        super().__init__()
@@ -550,6 +569,82 @@ class WorkerTests(unittest.TestCase):
        self.assertNotIn("values", json.dumps(observation))
        self.assertNotIn("Disabled", json.dumps(observation))

+    def test_observe_batches_dom_metadata_but_keeps_full_ax_roles_and_names(self):
+        backend = BatchFakeBackend()
+        backend.elements[1] = replace(backend.elements[1], name="DOM Email", ax_role="textbox", ax_name="AX Email")
+        backend.elements[3] = replace(backend.elements[3], name="DOM Submit", ax_role="button", ax_name="AX Submit")
+        backend.nodes[0]["name"] = {"value": "AX Email old-test-secret"}
+        backend.nodes[1]["name"] = {"value": "AX Submit"}
+        worker = BrowserWorker(backend, ["http://fixture.test"])
+
+        observation = worker.observe()
+
+        self.assertEqual(backend.metadata_many_calls, [[1, 3, 4, 2]])
+        self.assertEqual(backend.metadata_calls, [])
+        self.assertEqual([candidate["name"] for candidate in observation["candidates"]],
+                         ["DOM Email", "AX Submit", "Password"])
+        self.assertEqual([candidate["role"] for candidate in observation["candidates"]],
+                         ["textbox", "button", "textbox"])
+        self.assertNotIn("old-test-secret", json.dumps(observation))
+
+        result = worker.act({
+            "observationId": observation["observationId"], "candidateId": "c-1", "action": "click",
+            "postcondition": {"kind": "url", "value": "http://fixture.test/done"},
+        })
+        self.assertTrue(result["verified"])
+        self.assertEqual(backend.metadata_calls, [3])
+
+    def test_observe_falls_back_for_missing_batch_metadata(self):
+        backend = BatchFakeBackend()
+        backend.batch_result = {1: backend.elements[1]}
+        worker = BrowserWorker(backend, ["http://fixture.test"])
+
+        observation = worker.observe()
+
+        self.assertEqual([candidate["name"] for candidate in observation["candidates"]],
+                         ["Email", "Submit", "Password"])
+        self.assertEqual(backend.metadata_calls, [3, 4, 2])
+
+    def test_observe_batches_in_bounded_chunks_and_preserves_order_and_deduplication(self):
+        backend = BatchFakeBackend()
+        backend.elements = {
+            index: ElementMetadata(index, "button", "", "button", f"Button {index}", True, False,
+                                   ax_role="button", ax_name=f"Button {index}")
+            for index in range(1, 34)
+        }
+        backend.nodes = [
+            {"backendDOMNodeId": index, "role": {"value": "button"}, "name": {"value": f"AX {index}"}}
+            for index in range(1, 34)
+        ]
+        backend.nodes.insert(10, dict(backend.nodes[9]))
+        worker = BrowserWorker(backend, ["http://fixture.test"])
+
+        observation = worker.observe()
+
+        self.assertEqual([len(chunk) for chunk in backend.metadata_many_calls], [16, 16, 1])
+        self.assertEqual([candidate["name"] for candidate in observation["candidates"]],
+                         [f"AX {index}" for index in range(1, 34)])
+        self.assertEqual(len(observation["candidates"]), 33)
+
+    def test_observe_skips_detached_nodes_and_keeps_candidate_limit(self):
+        backend = BatchFakeBackend()
+        backend.elements = {
+            index: ElementMetadata(index, "button", "", "button", f"Button {index}", True, False,
+                                   connected=index != 10, ax_role="button", ax_name=f"Button {index}")
+            for index in range(1, 63)
+        }
+        backend.nodes = [
+            {"backendDOMNodeId": index, "role": {"value": "button"}, "name": {"value": f"AX {index}"}}
+            for index in range(1, 63)
+        ]
+        worker = BrowserWorker(backend, ["http://fixture.test"])
+
+        observation = worker.observe()
+
+        self.assertEqual(len(observation["candidates"]), 60)
+        self.assertNotIn("AX 10", json.dumps(observation))
+        self.assertTrue(all(len(chunk) <= 16 for chunk in backend.metadata_many_calls))
+
    def test_password_fill_uses_literal_without_exposing_value_in_metadata_or_result(self):
        backend = FakeBackend()
        backend.values[2] = "old-test-secret"
@@ -907,6 +1002,55 @@ class BridgeRegressionTests(unittest.TestCase):
        self.assertEqual(keydowns[0].kwargs["modifiers"], 4)
        backend._helpers.press_key.assert_called_once_with("Backspace")

+    def test_metadata_many_uses_one_call_and_releases_temporary_object_group(self):
+        backend = RealBrowserBackend.__new__(RealBrowserBackend)
+        helper = Mock()
+        backend._helpers = helper
+
+        def cdp(method, **kwargs):
+            if method == "DOM.resolveNode":
+                return {"object": {"objectId": f"object-{kwargs['backendNodeId']}"}}
+            if method == "Runtime.callFunctionOn":
+                return {"result": {"value": [
+                    {"tag": "input", "type": "text", "role": "", "name": "Email",
+                     "connected": True, "mainFrame": True, "contenteditable": False,
+                     "visible": True, "disabled": False},
+                    {"tag": "button", "type": "", "role": "button", "name": "Submit",
+                     "connected": True, "mainFrame": True, "contenteditable": False,
+                     "visible": True, "disabled": False},
+                ]}}
+            return {}
+        helper.cdp.side_effect = cdp
+
+        result = backend.metadata_many([1, 2])
+
+        self.assertEqual([metadata.name for metadata in result.values()], ["Email", "Submit"])
+        calls = [call.args[0] for call in helper.cdp.call_args_list]
+        self.assertEqual(calls.count("DOM.resolveNode"), 2)
+        self.assertEqual(calls.count("Runtime.callFunctionOn"), 1)
+        self.assertEqual(calls.count("Runtime.releaseObjectGroup"), 1)
+        call = next(call for call in helper.cdp.call_args_list if call.args[0] == "Runtime.callFunctionOn")
+        self.assertEqual([argument.get("objectId") for argument in call.kwargs["arguments"]],
+                         ["object-1", "object-2"])
+
+    def test_metadata_many_releases_object_group_when_batch_call_fails(self):
+        backend = RealBrowserBackend.__new__(RealBrowserBackend)
+        helper = Mock()
+        backend._helpers = helper
+
+        def cdp(method, **kwargs):
+            if method == "DOM.resolveNode":
+                return {"object": {"objectId": f"object-{kwargs['backendNodeId']}"}}
+            if method == "Runtime.callFunctionOn":
+                raise RuntimeError("detached")
+            return {}
+        helper.cdp.side_effect = cdp
+
+        self.assertEqual(backend.metadata_many([1, 2]), {})
+        self.assertEqual(
+            [call.args[0] for call in helper.cdp.call_args_list].count("Runtime.releaseObjectGroup"), 1
+        )
+
    def test_call_releases_remote_object_on_browser_exception(self):
        backend = RealBrowserBackend.__new__(RealBrowserBackend)
        backend._helpers = Mock()