diff --git a/moli-bounded-buffer/src/buffer.rs b/moli-bounded-buffer/src/buffer.rs index ed0fa4ed3..b89cb4a7f 100644 --- a/moli-bounded-buffer/src/buffer.rs +++ b/moli-bounded-buffer/src/buffer.rs @@ -70,23 +70,7 @@ where /// longer fit. Retained entries keep their original insertion order. pub fn set_limits(&mut self, limits: ByteLimits) -> Vec<(K, V)> { self.limits = limits; - let mut evicted = Vec::new(); - // Filter the order once. Calling remove for every oversized entry - // would rescan the whole queue on each removal when budgets shrink. - self.insertion_order.retain(|key| { - if self - .entries - .get(key) - .is_none_or(|entry| entry.byte_len <= limits.max_entry_bytes) - { - return true; - } - if let Some(entry) = self.entries.remove(key) { - self.used_bytes -= entry.byte_len; - evicted.push((key.clone(), entry.value)); - } - false - }); + let mut evicted = self.retain_entries(|_, entry| entry.byte_len <= limits.max_entry_bytes); while self.used_bytes > limits.max_total_bytes { if let Some(entry) = self.pop_oldest() { evicted.push(entry); @@ -182,12 +166,43 @@ where } /// Removes all entries and releases all byte charges. + /// + /// This does not return removed entries; use [`Self::retain`] to take ownership. pub fn clear(&mut self) { self.entries.clear(); self.insertion_order.clear(); self.used_bytes = 0; } + /// Visits each value once in insertion order, returning removed entries in + /// that order and releasing their byte charges. Mutating a value does not + /// change its recorded charge; use [`Self::insert`] to replace its charge. + pub fn retain(&mut self, mut keep: impl FnMut(&K, &mut V) -> bool) -> Vec<(K, V)> { + self.retain_entries(|key, entry| keep(key, &mut entry.value)) + } + + fn retain_entries( + &mut self, + mut keep: impl FnMut(&K, &mut BufferedValue) -> bool, + ) -> Vec<(K, V)> { + let mut removed = Vec::new(); + self.insertion_order.retain(|key| { + if self + .entries + .get_mut(key) + .is_some_and(|entry| keep(key, entry)) + { + return true; + } + if let Some(entry) = self.entries.remove(key) { + self.used_bytes -= entry.byte_len; + removed.push((key.clone(), entry.value)); + } + false + }); + removed + } + fn pop_oldest(&mut self) -> Option<(K, V)> { while let Some(key) = self.insertion_order.pop_front() { let Some(entry) = self.entries.remove(&key) else { diff --git a/moli-bounded-buffer/src/tests.rs b/moli-bounded-buffer/src/tests.rs index 0f3b57d56..d002720a5 100644 --- a/moli-bounded-buffer/src/tests.rs +++ b/moli-bounded-buffer/src/tests.rs @@ -154,3 +154,37 @@ fn remove_and_clear_return_all_byte_charges() { assert!(buffer.is_empty()); assert_eq!(buffer.used_bytes(), 0); } + +#[test] +fn retain_visits_once_preserves_fifo_and_releases_only_removed_charges() { + let mut buffer = BoundedByteBuffer::new(ByteLimits::new(10, 10)); + for key in 0..4 { + let _ = buffer.insert(key, key * 10, 2); + } + let mut visited = Vec::new(); + let removed = buffer.retain(|key, value| { + visited.push(*key); + *value += 1; + key % 2 == 0 + }); + assert_eq!(visited, vec![0, 1, 2, 3]); + assert_eq!(removed, vec![(1, 11), (3, 31)]); + assert_eq!(buffer.used_bytes(), 4); + assert_eq!(buffer.get(&2), Some(&21)); + assert_eq!( + buffer.insert(4, 40, 8), + InsertOutcome::Stored { + evicted: vec![(0, 1)] + } + ); + assert!(buffer.retain(|_, _| true).is_empty()); + assert_eq!(buffer.used_bytes(), 10); + assert_eq!(buffer.retain(|_, _| false), vec![(2, 21), (4, 40)]); + assert_eq!(buffer.used_bytes(), 0); + assert!(buffer.is_empty()); + assert!( + buffer + .retain(|_, _| panic!("empty buffer must not visit")) + .is_empty() + ); +} diff --git a/moli-fetch/src/lib.rs b/moli-fetch/src/lib.rs index 90c1e6ca8..9484127cd 100644 --- a/moli-fetch/src/lib.rs +++ b/moli-fetch/src/lib.rs @@ -72,7 +72,8 @@ pub use request::{ RequestAuthScheme, RequestAuthTarget, RequestCacheMode, RequestCredentialsMode, RequestHeaderOverride, RequestMode, RequestPriorityHints, RequestRedirectMode, RequestResourceType, ResourceLoadPriority, ScriptFetchRequestMetadata, - ScriptFetchSchedulerPriority, SubresourceRequestMetadata, + ScriptFetchSchedulerPriority, SubresourceRequestMetadata, is_request_body_header_name, + redirect_status_rewrites_to_get, }; pub use request_policy::{is_bad_port, should_request_be_blocked_due_to_bad_port}; pub use response::{ diff --git a/moli-fetch/src/request.rs b/moli-fetch/src/request.rs index 16cc65e9e..b2cf21adc 100644 --- a/moli-fetch/src/request.rs +++ b/moli-fetch/src/request.rs @@ -1122,12 +1122,14 @@ impl Request { } } -fn redirect_status_rewrites_to_get(status: u16, method: &str) -> bool { +/// Whether following this HTTP redirect replaces the request method with GET. +pub fn redirect_status_rewrites_to_get(status: u16, method: &str) -> bool { status == 303 && !method.eq_ignore_ascii_case("GET") && !method.eq_ignore_ascii_case("HEAD") || matches!(status, 301 | 302) && method.eq_ignore_ascii_case("POST") } -fn is_request_body_header_name(name: &str) -> bool { +/// Whether a header must be removed when a redirect discards the request body. +pub fn is_request_body_header_name(name: &str) -> bool { matches!( name.to_ascii_lowercase().as_str(), "content-encoding" diff --git a/moli-protocol-server/src/protocol_server/tests/classic.rs b/moli-protocol-server/src/protocol_server/tests/classic.rs index 8bc4b2ab1..26b4f0743 100644 --- a/moli-protocol-server/src/protocol_server/tests/classic.rs +++ b/moli-protocol-server/src/protocol_server/tests/classic.rs @@ -11559,7 +11559,6 @@ async fn webdriver_classic_click_uses_top_level_pointer_coordinates_inside_offse } #[tokio::test] -#[ignore = "Pending iframe overlay click fix; expectations verified against Chromium"] async fn webdriver_classic_frame_target_click_is_invariant_under_neutral_frame_wrapping() { for depth in 0..=2 { for selector in ["#target", "#container"] { @@ -11646,7 +11645,6 @@ async fn webdriver_classic_frame_target_click_is_invariant_under_neutral_frame_w } #[tokio::test] -#[ignore = "Pending ancestor overlay click fix; expectations verified against Chromium"] async fn webdriver_classic_frame_click_rejects_ancestor_overlays_without_dispatch() { for (depth, blocked_level) in [(1, 0), (2, 0), (2, 1)] { let app = build_router(test_state()); diff --git a/moli-protocol/src/domains/network/agent.rs b/moli-protocol/src/domains/network/agent.rs index eea278c02..aa8aead02 100644 --- a/moli-protocol/src/domains/network/agent.rs +++ b/moli-protocol/src/domains/network/agent.rs @@ -1402,18 +1402,12 @@ impl CapturedResponseBodyStore { fn revoke_retained_visibility(&mut self, session_id: Option<&str>) { // Revocation affects only old-document claims, not ordinary access to // responses from the current document. - for request_id in &self.retained_request_ids { - if let Some(body) = self.bodies.get_mut(request_id) - && !body.remove_session_visibility(session_id) - { - self.bodies.remove(request_id); - } - if let Some(body) = self.buffered_bodies.get_mut(request_id) - && !body.remove_session_visibility(session_id) - { - self.buffered_bodies.remove(request_id); - } - } + let keep = |request_id: &String, body: &mut CapturedResponseBody| { + !self.retained_request_ids.contains(request_id) + || body.remove_session_visibility(session_id) + }; + self.bodies.retain(keep); + self.buffered_bodies.retain(keep); } fn effective_durable_limits(&self) -> ByteLimits { @@ -1462,13 +1456,20 @@ impl CapturedResponseBodyStore { } fn trim_durable_entries(&mut self) { + let mut expired_payloads = HashSet::new(); while self.durable_entry_order.len() > DURABLE_RESPONSE_BODY_MAX_ENTRIES { if let Some(id) = self.durable_entry_order.pop_front() { self.bodies.remove(&id); - self.buffered_bodies.remove(&id); self.retained_request_ids.remove(&id); + if self.buffered_bodies.contains_key(&id) { + expired_payloads.insert(id); + } } } + if !expired_payloads.is_empty() { + self.buffered_bodies + .retain(|id, _| !expired_payloads.contains(id)); + } } fn prepare_navigation(&mut self) { @@ -1485,16 +1486,7 @@ impl CapturedResponseBodyStore { && !matches!(body.state, CapturedResponseBodyState::Pending) }; self.bodies.retain(|_, body| retain(body)); - let ids = self - .buffered_bodies - .iter() - .map(|(id, _)| id.clone()) - .collect::>(); - for id in ids { - if !self.buffered_bodies.get_mut(&id).is_some_and(retain) { - self.buffered_bodies.remove(&id); - } - } + self.buffered_bodies.retain(|_, body| retain(body)); self.retained_request_ids = self .bodies .keys() @@ -1638,20 +1630,8 @@ impl CapturedResponseBodyStore { self.bodies .retain(|_, body| body.remove_session_visibility(session_id)); - let buffered_request_ids = self - .buffered_bodies - .iter() - .map(|(request_id, _)| request_id.clone()) - .collect::>(); - for request_id in buffered_request_ids { - let retain = self - .buffered_bodies - .get_mut(request_id.as_str()) - .is_some_and(|body| body.remove_session_visibility(session_id)); - if !retain { - self.buffered_bodies.remove(request_id.as_str()); - } - } + self.buffered_bodies + .retain(|_, body| body.remove_session_visibility(session_id)); } #[cfg(test)] diff --git a/moli-protocol/src/domains/network/redirect_request.rs b/moli-protocol/src/domains/network/redirect_request.rs index 01dbebf0d..459caf943 100644 --- a/moli-protocol/src/domains/network/redirect_request.rs +++ b/moli-protocol/src/domains/network/redirect_request.rs @@ -1,6 +1,6 @@ /// Project the request metadata for successive followed redirect hops. /// -/// This mirrors FetchRequest::apply_redirect_status: the emitted CDP request +/// This shares FetchRequest::apply_redirect_status rules: the emitted CDP request /// must describe the new hop, not repeat the original POST after it became GET. pub(super) struct RedirectRequest<'a> { pub(super) method: &'a str, @@ -22,23 +22,11 @@ impl<'a> RedirectRequest<'a> { } pub(super) fn follow(&mut self, status: u16) { - let becomes_get = matches!(status, 301 | 302) && self.method.eq_ignore_ascii_case("POST") - || status == 303 - && !self.method.eq_ignore_ascii_case("GET") - && !self.method.eq_ignore_ascii_case("HEAD"); - if becomes_get { + if moli_fetch::redirect_status_rewrites_to_get(status, self.method) { self.method = "GET"; self.body = None; - self.headers.retain(|(name, _)| { - !matches!( - name.to_ascii_lowercase().as_str(), - "content-encoding" - | "content-language" - | "content-length" - | "content-location" - | "content-type" - ) - }); + self.headers + .retain(|(name, _)| !moli_fetch::is_request_body_header_name(name)); } } } diff --git a/moli/src/telemetry.rs b/moli/src/telemetry.rs index 97a30d277..626b89d53 100644 --- a/moli/src/telemetry.rs +++ b/moli/src/telemetry.rs @@ -40,9 +40,6 @@ mod tests { struct SharedWriterGuard(Arc>>); - #[derive(Clone, Copy)] - struct BrokenWriter; - impl Write for SharedWriterGuard { fn write(&mut self, buffer: &[u8]) -> io::Result { self.0.lock().extend_from_slice(buffer); @@ -54,30 +51,6 @@ mod tests { } } - impl Write for BrokenWriter { - fn write(&mut self, _buffer: &[u8]) -> io::Result { - Err(io::Error::new( - io::ErrorKind::BrokenPipe, - "diagnostic sink is gone", - )) - } - - fn flush(&mut self) -> io::Result<()> { - Err(io::Error::new( - io::ErrorKind::BrokenPipe, - "diagnostic sink is gone", - )) - } - } - - impl<'writer> MakeWriter<'writer> for BrokenWriter { - type Writer = Self; - - fn make_writer(&'writer self) -> Self::Writer { - *self - } - } - impl<'writer> MakeWriter<'writer> for SharedWriter { type Writer = SharedWriterGuard; @@ -138,17 +111,28 @@ mod tests { #[test] fn broken_diagnostic_writer_does_not_panic() { - let subscriber = tracing_subscriber::fmt() - .without_time() - .with_ansi(false) - .with_target(false) - .with_writer(BrokenWriter) - .with_env_filter(first_party_filter("error")) - .log_internal_errors(false) - .finish(); - - tracing::subscriber::with_default(subscriber, || { - tracing::error!("broken diagnostic writer regression"); - }); + const CHILD: &str = "MOLI_TEST_BROKEN_DIAGNOSTIC_SINK"; + if std::env::var_os(CHILD).is_some() { + super::init("error"); + tracing::error!("the diagnostic receiver has already exited"); + return; + } + let (reader, writer) = std::io::pipe().unwrap(); + drop(reader); + let status = std::process::Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "telemetry::tests::broken_diagnostic_writer_does_not_panic", + "--nocapture", + ]) + .env(CHILD, "1") + .stdout(std::process::Stdio::null()) + .stderr(writer) + .status() + .unwrap(); + assert!( + status.success(), + "logging to a closed diagnostic pipe terminated the process: {status}" + ); } }