feat(cdp): reject unknown Cb.startNativeSession params instead of ignoring them (C5) #58
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "c5/reject-unknown-params"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
C5 / Track 2. ⚠ Unverified C++ — t7 build running.
I did not build the schema generator, and I'd argue you don't want one
The plan called for generating the
Cb.*dispatcher from a checked-in schema, to kill the silently-ignored-param class structurally. Reading the code first, that's the wrong size of answer:One method takes params. A generator, a schema file, and a build step to maintain them is more machinery than that surface justifies — and it would not have caught either bug earlier than four lines of key-checking do.
What BUG-1 and BUG-2 actually share
Not the spellings — the behaviour: an unknown key is silently ignored.
signalingUseTlsunread →use_tlsdefaulted truewss://at a plaintext broker → opaque TLS error, minutes later, far from the causeiceServersunread → fell back toBuildDefaultIceServers()stun.l.google.comwith a relay-only policy that could never be satisfiedNeither appeared in any response or log. The aliases already in the tree fix the two spellings we know about; rejecting unknown keys fixes the next one — at the point of the mistake, with the misspelling quoted and the accepted set listed beside it.
Cb.*is hand-dispatched: no PDL, no/json/protocolentry, nothing validating params.kKnownParamsis the schema. One line to add a param.Breakage audit
This rejects input that previously succeeded, so I checked every caller rather than assuming:
browser_session.rs:1555-1610signalingHost,signalingSessionId,signalingToken,useTls,iceServers,iceTransportPolicytests/scratch/cv2-fcdirect.mjs:110signalingHost,signalingSessionId,signalingUseTls,iceServersA caller that would break was, by definition, already having its param silently dropped.
Stacking
Branched on
verify/c4-with-tests(= #57 + #56 + #54), because a branch offmaincannot buildcb_wire_envelope_unittests— I made exactly that mistake on the first attempt and the build failed on thepartition_allocerror, not on anything in this change.The build I fired to prove cb_wire_envelope_unittests passes did not get that far — it failed to COMPILE the test, with: FAILED: cb_wire_envelope_test.o ../../base/memory/raw_ptr_exclusion.h:11:10: fatal error: 'partition_alloc/pointers/raw_ptr_exclusion.h' file not found Root cause: :cb_wire_envelope listed //base in private `deps`. But cb_wire_envelope.h includes "base/values.h" in its PUBLIC interface (ProbeResultPayload holds a base::DictValue), so any target that includes that header needs //base's include dirs transitively — and //base is what pulls in partition_alloc's generated headers. The source_set itself compiled fine (it sees //base directly); only DEPENDENTS broke. With no dependent building, nothing could observe it. That is gn's deps-vs-public_deps distinction exactly: a dep whose headers appear in your own public headers is part of your interface, not an implementation detail. Then the obvious question — is this one target or a class? Scanned every source_set in capture/ for "public header includes base/ AND //base is a private dep". NINETEEN targets, including: pointer_state, input_dispatch_ime, input_dispatch_touch, input_dispatch_drag, input_dispatch_clipboard, cursor_client, cursor_xy_join, embedder, framesink_capture, stats_relay, clipboard_relay, file_upload_relay, active_webcontents_resolver, cb_audio_lifecycle, cb_signaling_ws_client, cb_offerer_driver, cb_dc_host, cb_signaling_reconnect, cb_wire_envelope Note which names appear there: pointer_state and input_dispatch. Their test binaries — cloud_browser_pointer_state_unittests, cloud_browser_input_dispatch_unittests — are two of the five that have never been built in any lane. This is very likely why: whoever last tried to add them hit this same wall and dropped the target rather than the dep. I cannot prove that from here (no record of the attempt), but the shape matches. All nineteen moved to public_deps. The worker binary links them all transitively either way, which is why nothing was broken in production — the defect is only visible to a target that depends on ONE of them in isolation, i.e. a unit test. That is the whole point of unit tests having narrow deps, and it is why this stayed hidden while the tests stayed unbuilt. Verified locally: brace/bracket balance on both BUILD.gn files, no target left with //base in both deps and public_deps, re-scan reports zero remaining affected targets, make verify + cxx-include-lint clean. NOT verified: that it compiles. A t7 build follows — that is the point. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>Found while verifying C4's t7 build: my new wire-envelope tests COMPILED and never executed. Neither did the ones that were already there — including the negative test whose own comment says it exists "so a maintainer has to argue the contract change explicitly". I cited that test as a guarantee earlier today. It has never run in this lane. Two independent gaps, same shape: 1. CHROMELESS_BUILD_TARGETS named two unittest targets. The tree declares SEVEN: cb_wire_envelope_unittests <- never built cloud_browser_adm_unittests <- never built cloud_browser_encoder_unittests built cloud_browser_framesink_capturer_unittests built cloud_browser_input_dispatch_unittests <- never built cloud_browser_pcf_unittests (t2-only, deliberate) cloud_browser_pointer_state_unittests <- never built Earlier today I restored encoder+framesink to close a 10-week silent-skip and did not notice I was restoring a two-item list into a seven-item tree. 2. STEP 7 invoked those two binaries BY NAME. So even adding a target to the lane would have changed nothing here — it would build and sit there. The mismatch is invisible: the build goes green either way. That is the same failure shape as the silent-skip STEP 7's fatal-on-missing check was added to close, one level up — there the binary was absent, here it exists and nobody invokes it. STEP 7 now DERIVES the list from CHROMELESS_BUILD_TARGETS (any label whose target name ends in _unittests), so a target added to the lane is automatically executed and cannot drift out of the runner again. The existing fatal-on-missing behaviour is preserved and now applies to every derived binary; a lane with no test targets at all logs a loud WARN rather than passing silently. Adds cb_wire_envelope_unittests to the x264-t7 lane. That is the contract this repo publishes to third parties, so it is the one that most needs to run. The other three unbuilt targets (adm, input_dispatch, pointer_state) are NOT added here: nothing has compiled them in ~10 weeks, they may have bitrotted, and finding that out is its own change rather than something to discover while landing a codec fix. Verified locally (this part is shell, so it is verifiable without Chromium): bash -n clean, and the derivation exercised against a realistic target string — picks exactly the two _unittests labels, correctly ignores cloud_browser_worker and headless:resource_pack_data. NOT verified: that cb_wire_envelope_unittests passes. A t7 build is running to find out; that is the entire point of the change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>localoutside a function killed STEP 7 silently — plus a lintMy own bug, from the previous commit on this branch. The STEP 7 rewrite used local -a test_binaries=() inside an `if` block at SCRIPT TOP LEVEL. bash rejects that outright: bash: local: can only be used in a function Under `set -e` the step aborts instantly and — the expensive part — prints NOTHING of its own. The log reads: === STEP 6/10 autoninja OK (745s) === === STEP 7/10 unit tests START === <end of file> which looks exactly like an infrastructure kill. I spent the first pass chasing reapers, node pressure and hostPath contention before reading my own diff. The compile had SUCCEEDED; only my four extra characters failed. `bash -n` does not catch this. It is a runtime error, not a syntax error, so the script passed every check I ran locally and the real feedback took 27 minutes of t7 build to arrive. Two changes: 1. Drop the three `local` keywords. Top-level variables are already global; the keyword bought nothing. Verified every remaining `local` in the file is genuinely inside a function. 2. tools/lint/shell_toplevel_local_lint.py — because "a four-character mistake costs a quarter hour of Chromium" is exactly the feedback-loop gap this repo's lint directory exists to close, and it is the same argument that produced lint-cxx (a missing #include surfacing hours later in an unwatched lane). Checked against the whole tree before landing, per CONTRIBUTING: clean on all 35 shell scripts, and confirmed to FAIL on the real bug by reintroducing it on a copy. It tracks brace depth, handles both function syntaxes, and skips heredoc bodies so `local` appearing as text in an embedded script is not flagged. Wired into CI's Lint job next to the other host-runnable checks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>⚠ UNVERIFIED C++ — capture/ cannot be compiled here (no local Chromium; a build is 4-8h cold). A t7 build is running against this branch; do not merge until it is green. The research first, because the original plan was wrong about two things: 1. Physics is NOT a second dialect. webrtc_signaling.rs's SignalingEnvelope is #[serde(tag="type", rename_all="snake_case")] over variants Offer / Answer / Ice / …, so it emits exactly "offer", "answer", "ice" — byte identical to this codec's accept-list. The `sdp_offer` strings in that file are LOG LABELS (lines 1072, 1096), not wire tags. The plan's "delete physics's serde aliases" step has nothing to delete. 2. The PORTAL is the real second dialect, and it is genuinely incompatible. portal/src/canvas/browser_screencast_webrtc.rs:48-53: {"type":"sdp_offer", "sdp":"v=0..."} {"type":"sdp_answer", "sdp":"v=0..."} {"type":"ice_candidate", "candidate":"...", "sdpMid":"0", …} Flat: no `from`, payload inlined as siblings of `type`. Decode() rejected it on all three counts. It works today only because physics translates in the middle (pattern_c_envelope_to_portal_msg, screencast_ws.rs:6909). This change teaches Decode() the flat dialect and normalizes it to the same Envelope struct. Encode is untouched: we still always EMIT canonical, because an emitter that picked a dialect per-peer would need to know which peer it is addressing, which this codec deliberately does not. What this does NOT do, stated because the plan implied otherwise: it does not make pattern_c_envelope_to_portal_msg deletable. That translator is LOSSY in the other direction — it returns None for bye, request_renegotiate, probe_result and session_unhealthy, so those four tags never reach the portal at all. Removing it needs the portal to learn those tags first. "Liberalize the decoder" and "delete the translator" are two changes, not one. Design notes: * PortalTagFromString is a SEPARATE function from TagFromString, and the test asserts the two tag spaces stay disjoint. Decode() consults both; that is where the widening lives. Keeping it out of TagFromString is what stops "canonical" quietly becoming "anything we accept". * The accept-list stays CLOSED — three tags wider, not open. A new negative test uses `restart_ice`, well-formed in every respect except its tag. * `from` is absent in this dialect and is inferred as kClient. That is a statement (the portal is the browser-facing UI and the only producer of these frames), not a default — a future flat-but-not-client producer must send canonical instead. Callers do switch on `from`, so leaving it indeterminate was not an option. * A flat `ice_candidate` with no `candidate` field is the end-of-candidates marker: canonical says that with `data: null`, which has no flat equivalent because the payload IS the frame. The old RejectsHistoricSdpOfferTag test called sdp_offer "the historic v0 tag" and existed so that flipping the contract required an explicit argument rather than a quiet edit. The framing was wrong on the facts — it is neither historic nor v0, it is live — and this commit is that argument, made in the place the tripwire pointed at. conformance/suites/signaling-wire.mjs moves in lockstep: sdp_offer leaves REJECTED_TAGS for a new PORTAL_TAGS export. Without that the kit would assert the opposite of the code it ships beside. Verified locally: make verify passes, cxx-include-lint clean (121 files), conformance 23/23. NOT verified: that any of this compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>⚠ UNVERIFIED C++ — a t7 build is running against this branch. The plan called for generating the Cb.* dispatcher from a checked-in schema, to kill "the silently-ignored-param class (BUG-1/BUG-2's root cause) structurally". Reading the code first, the generator is the wrong size of answer. The Cb.* surface is FOUR methods and exactly ONE of them takes params: Cb.startNativeSession 7 accepted keys Cb.startFrameSinkCapture, Cb.getCaptureStats, Cb.shutdown — no params A code generator, a schema file, and a build step to maintain all that is more machinery than the surface justifies, and it would not have caught either bug any earlier than the four lines below do. What both bugs actually share is one behaviour: an unknown key is silently ignored. CLAUDE.md already names them: BUG-1 `signalingUseTls` read as `useTls` → use_tls defaulted true → the guest dialled wss:// at a plaintext broker → opaque TLS error minutes later, far from the cause. BUG-2 array-form `iceServers` read only as a string → fell through to BuildDefaultIceServers() → a deployment with provisioned TURN silently ran on stun.l.google.com with a relay-only policy that could never be satisfied. Neither appeared in any response or log. The aliases already in the tree fix the two spellings we know about. This rejects unknown keys, which fixes the NEXT one too — at the point of the mistake, with the misspelling quoted in the error and the accepted set listed beside it. `Cb.*` is hand-dispatched: no PDL, no /json/protocol entry, nothing validating params. The kKnownParams array IS the schema. One line to add a param. BREAKAGE AUDIT — this rejects input that previously succeeded, so I checked every caller rather than assuming: isolator (PRODUCTION, mtest-tw5/isolator/src/browser_session.rs:1555-1610) sends signalingHost, signalingSessionId, signalingToken, useTls, iceServers, iceTransportPolicy — all six known. tests/scratch/cv2-fcdirect.mjs:110 sends signalingHost, signalingSessionId, signalingUseTls, iceServers — all four known. physics: no call site (the three grep hits are comments). So no current caller is affected. A caller that WOULD have been affected was by definition already having its param silently dropped. Verified locally: make verify, cxx-include-lint clean (121 files). NOT verified: that it compiles. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>✅ Build green on t7
Three binaries invoked, three passed — checked the invocation lines rather than the summary, since a green STEP 7 that ran nothing is the exact failure #56 exists to remove.
The dict-iteration idiom compiled clean, which was the drift risk worth naming:
base's dict type spelling has moved across rolls (this repo carries three pure-drift-fix commits), so thekv.firstform rather than a structured binding was the conservative choice.Still not verified: that the rejection fires — there is no test asserting an unknown param produces an error.
cb_devtools_agent.cchas no test target at all, so adding one is a larger change than this (it needs a target, and #56's derived STEP 7 to run it). The breakage audit in the description is what backs the behaviour for now: production sends six keys, all known.