iter-124: navigate probe actor refresh + cookies contract test - #163
Merged
Conversation
ractive
added a commit
that referenced
this pull request
Jul 19, 2026
…round-trips Local PR review (#163) found three issues in the console-actor refresh fix: 1. HIGH: refresh_probe_console_actor set probe_refreshed = true even when TabActor::get_target returned Err. A single transient failure (the new docshell not yet queryable server-side) permanently stranded the probe on its stale actor for the rest of the wait, intermittently reintroducing the exact noSuchActor bug this PR fixes. The function now returns bool; both call sites in wait_for_doc_complete only latch probe_refreshed on Ok, so a failed attempt retries at the next probe-timer tick instead of giving up permanently. 2. MED: added unit_navigate_probe_refresh_retries_after_transient_error (first getTarget errors, second succeeds, probe recovers and short-circuits) and unit_navigate_probe_refresh_persistent_error_falls_back_to_timeout (every getTarget attempt errors, wait_for_doc_complete falls through cleanly to AppError::Timeout with no panic and no hang past the budget). 3. LOW: the dom-loading refresh fired for every wait_level, including Loading/Interactive with a non-empty URL, where the refreshed actor is never consumed (those cases resolve straight from the event's own url). Gated on wait_level == WaitLevel::Complete || url.is_empty() so the round-trip is only paid when something will actually use it.
Owner
Author
Local review round (local-only mode)
Post-fix: gates clean (fmt/clippy/workspace 33 blocks 0 fail), serial live |
The iter-122 Theme A interleaved readystate probe captured its console actor from the pre-navigation target and never refreshed it. Firefox tears down the old docshell (and, cross-process, the child process) once the new document commits, invalidating that actor ID — every subsequent probe eval failed with noSuchActor for the rest of the wait, silently defeating the fast path and falling through to the full events-budget timeout (~5.6-5.9s instead of the sub-4s the iter-122 AC requires). Refresh ReadyStateProbe::console_actor via TabActor::get_target once dom-loading commits (or lazily before the first probe attempt if the events stream is quiet), matching the existing refresh_target pattern used elsewhere in the connect_tab module. live_navigate_default_fast now completes in ~9.7s total across 5 tests (was 33.85s with the target test alone timing out at 5.9s).
live_cookies_surfaces_js_readable_cookie asserted the probe cookie's source was always "document.cookie" — true only because iter-121's predecessor state had StorageActor cookie enumeration dead on FF152, making the document.cookie fallback the sole path that ever surfaced a JS-set cookie without a Domain= attribute. iter-121 fixed StorageActor enumeration, and it turns out FF152's StorageActor does enumerate this cookie shape. The cookies command's merge logic (crates/ff-rdp-cli/src/commands/cookies.rs) then correctly drops the now-redundant document.cookie duplicate, so the surviving entry has no `source` field (StorageActor-derived CookieInfo doesn't serialize one) instead of "document.cookie" — strictly better data (carries isHttpOnly/isSecure/sameSite), not a regression. Rewrite the assertion to accept either valid shape: StorageActor- sourced (no source field, isHttpOnly present) or document.cookie- sourced (source: "document.cookie"), so the test pins the actual contract instead of an implementation detail that iter-121 changed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…round-trips Local PR review (#163) found three issues in the console-actor refresh fix: 1. HIGH: refresh_probe_console_actor set probe_refreshed = true even when TabActor::get_target returned Err. A single transient failure (the new docshell not yet queryable server-side) permanently stranded the probe on its stale actor for the rest of the wait, intermittently reintroducing the exact noSuchActor bug this PR fixes. The function now returns bool; both call sites in wait_for_doc_complete only latch probe_refreshed on Ok, so a failed attempt retries at the next probe-timer tick instead of giving up permanently. 2. MED: added unit_navigate_probe_refresh_retries_after_transient_error (first getTarget errors, second succeeds, probe recovers and short-circuits) and unit_navigate_probe_refresh_persistent_error_falls_back_to_timeout (every getTarget attempt errors, wait_for_doc_complete falls through cleanly to AppError::Timeout with no panic and no hang past the budget). 3. LOW: the dom-loading refresh fired for every wait_level, including Loading/Interactive with a non-empty URL, where the refreshed actor is never consumed (those cases resolve straight from the event's own url). Gated on wait_level == WaitLevel::Complete || url.is_empty() so the round-trip is only paid when something will actually use it.
ractive
force-pushed
the
iter-124/live-sweep-followups
branch
from
July 19, 2026 14:43
9d0466a to
1740642
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ReadyStateProbe.console_actorwas captured beforenavigateTo; Firefox invalidates that actor when the new document commits, so every probe attempt failed withnoSuchActor(18/18 instrumented) and the iter-122 fast path silently fell back to the ~5.6–5.9s events-budget burn. Fix:refresh_probe_console_actor()re-resolves viaTabActor::get_targetwhendom-loadingcommits (lazy fallback before the first probe on quiet streams), mirroringConnectedTab::refresh_target.wait_for_doc_completenow takesOption<&mut ReadyStateProbe>.live_cookies_surfaces_js_readable_cookieassertedsource == "document.cookie"— only ever true while StorageActor enumeration was dead (pre-iter-121). The merge incookies.rscorrectly prefers the StorageActor entry (real flags, nosourcefield). Test now pins the StorageActor-or-fallback contract instead of the implementation detail.kb/iterations/iteration-124-live-sweep-followups.md; remaining dogfood-61 moderate bugs filed as iters 125–127.Test plan
live_navigate_default_fastserial post-fix: 2 passed in 4.07s (was 5.6–5.9s, red)live_cookies_surfaces_js_readable_cookieserial post-fix: PASS (storage_actor_sourced=true)live_cookies/live_navigate_default_fast/live_123filters: all green (5+5+4)cargo fmt/cargo clippy --workspace --all-targets -- -D warnings/cargo test --workspace -q: clean (33 blocks, 1687 passed, 0 failed)FF_RDP_LIVE_TESTS=1 cargo xtask check-iteration-ready: 10/10 PASS🤖 Generated with Claude Code