From ff02fdad6d42dd6c0380c5f321407d33e4353eb3 Mon Sep 17 00:00:00 2001 From: Usman Yahaya Baba <91813795+uthmaniv@users.noreply.github.com> Date: Tue, 15 Jul 2025 06:41:11 +0100 Subject: [PATCH] Send early DevToolsHttpRequest and relocate response reporting to main_fetch (#37906) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This change refactors how we notify DevTools about network activity so that all fetches (even those served from cache) appear correctly in the Network panel, and so that DevTools sees request metadata as soon as possible rather than waiting until the end of a full HTTP cycle. - Before, we only send DevTools events inside http_network_fetch, so cached responses (which skip that path) never show up. By emitting a minimal HttpRequest event at the very start of main_fetch (with URL, method, pipeline and browsing IDs), we guarantee every fetch shows up immediately. - Then, by moving HttpResponse notifications out of http_network_fetch into main_fetch (right after process_response and process_response_eof), we ensure DevTools gets status, header, and completion events for both network and cache hits. Leveraging nullable fields in NetworkEventActor lets us incrementally fill in timing, header, and body data later, improving DevTools’ visibility. Testing: Ran servo with `--devtools=6080` flag, cached responses now appear in the network panel Fixes: https://github.com/servo/servo/issues/37869 --------- Signed-off-by: Uthman Yahaya Baba --- components/devtools/lib.rs | 1 + components/devtools/network_handler.rs | 15 +++ components/net/fetch/methods.rs | 14 ++- components/net/http_loader.rs | 121 +++++++++++++++---------- components/net/tests/fetch.rs | 12 +-- components/net/tests/http_loader.rs | 49 +++++----- components/shared/devtools/lib.rs | 1 + components/shared/net/request.rs | 2 +- 8 files changed, 136 insertions(+), 79 deletions(-) diff --git a/components/devtools/lib.rs b/components/devtools/lib.rs index 02ca7af703e..0e2d4c4a027 100644 --- a/components/devtools/lib.rs +++ b/components/devtools/lib.rs @@ -493,6 +493,7 @@ impl DevtoolsInstance { ) { let browsing_context_id = match &network_event { NetworkEvent::HttpRequest(req) => req.browsing_context_id, + NetworkEvent::HttpRequestUpdate(req) => req.browsing_context_id, NetworkEvent::HttpResponse(resp) => resp.browsing_context_id, }; diff --git a/components/devtools/network_handler.rs b/components/devtools/network_handler.rs index 3aa535a07a6..fd2dace4d9d 100644 --- a/components/devtools/network_handler.rs +++ b/components/devtools/network_handler.rs @@ -55,6 +55,21 @@ pub(crate) fn handle_network_event( ); } }, + + NetworkEvent::HttpRequestUpdate(httprequest) => { + actor.add_request(httprequest); + let resource = actor.resource_updates(); + let watcher_actor = actors.find::(&watcher_name); + + for stream in &mut connections { + watcher_actor.resource_array( + resource.clone(), + "network-event".to_string(), + ResourceArrayType::Updated, + stream, + ); + } + }, NetworkEvent::HttpResponse(httpresponse) => { // Store the response information in the actor actor.add_response(httpresponse); diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index e99776be350..69e2a0d87c7 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -42,7 +42,10 @@ use super::fetch_params::FetchParams; use crate::fetch::cors_cache::CorsCache; use crate::fetch::headers::determine_nosniff; use crate::filemanager_thread::FileManager; -use crate::http_loader::{HttpState, determine_requests_referrer, http_fetch, set_default_accept}; +use crate::http_loader::{ + HttpState, determine_requests_referrer, http_fetch, send_early_httprequest_to_devtools, + send_response_to_devtools, set_default_accept, +}; use crate::protocols::{ProtocolRegistry, is_url_potentially_trustworthy}; use crate::request_interceptor::RequestInterceptor; use crate::subresource_integrity::is_response_integrity_valid; @@ -257,7 +260,8 @@ pub async fn main_fetch( ) -> Response { // Step 1: Let request be fetchParam's request. let request = &mut fetch_params.request; - + // send early HTTP request to DevTools + send_early_httprequest_to_devtools(request, context); // Step 2: Let response be null. let mut response = None; @@ -701,6 +705,8 @@ pub async fn main_fetch( // Step 22. target.process_response(request, &response); + // Send Response to Devtools + send_response_to_devtools(request, context, &response); // Step 23. if !response_loaded { @@ -709,6 +715,10 @@ pub async fn main_fetch( // Step 24. target.process_response_eof(request, &response); + // Send Response to Devtools + // This is done after process_response_eof to ensure that the body is fully + // processed before sending the response to Devtools. + send_response_to_devtools(request, context, &response); if let Ok(http_cache) = context.state.http_cache.write() { http_cache.update_awaiting_consumers(request, &response); diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 906c338c9c9..baef79cacce 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -417,12 +417,12 @@ fn prepare_devtools_request( is_xhr, browsing_context_id, }; - let net_event = NetworkEvent::HttpRequest(request); + let net_event = NetworkEvent::HttpRequestUpdate(request); ChromeToDevtoolsControlMsg::NetworkEvent(request_id, net_event) } -fn send_request_to_devtools( +pub fn send_request_to_devtools( msg: ChromeToDevtoolsControlMsg, devtools_chan: &Sender, ) { @@ -431,25 +431,74 @@ fn send_request_to_devtools( .unwrap(); } -fn send_response_to_devtools( - devtools_chan: &Sender, - request_id: String, - headers: Option, - status: HttpStatus, - pipeline_id: PipelineId, - browsing_context_id: BrowsingContextId, +pub fn send_response_to_devtools( + request: &mut Request, + context: &FetchContext, + response: &Response, ) { - let response = DevtoolsHttpResponse { - headers, - status, - body: None, - pipeline_id, - browsing_context_id, - }; - let net_event_response = NetworkEvent::HttpResponse(response); + if let (Some(devtools_chan), Some(pipeline_id), Some(webview_id)) = ( + context.devtools_chan.as_ref(), + request.pipeline_id, + request.target_webview_id, + ) { + let browsing_context_id = webview_id.0; + let meta = match response + .metadata() + .expect("Response metadata should exist at this stage") + { + FetchMetadata::Unfiltered(m) => m, + FetchMetadata::Filtered { unsafe_, .. } => unsafe_, + }; + let status = meta.status; + let headers = meta.headers.map(Serde::into_inner); - let msg = ChromeToDevtoolsControlMsg::NetworkEvent(request_id, net_event_response); - let _ = devtools_chan.send(DevtoolsControlMsg::FromChrome(msg)); + let devtoolsresponse = DevtoolsHttpResponse { + headers, + status, + body: None, + pipeline_id, + browsing_context_id, + }; + let net_event_response = NetworkEvent::HttpResponse(devtoolsresponse); + + let msg = + ChromeToDevtoolsControlMsg::NetworkEvent(request.id.0.to_string(), net_event_response); + + let _ = devtools_chan + .lock() + .unwrap() + .send(DevtoolsControlMsg::FromChrome(msg)); + } +} + +pub fn send_early_httprequest_to_devtools(request: &mut Request, context: &FetchContext) { + if let (Some(devtools_chan), Some(browsing_context_id), Some(pipeline_id)) = ( + context.devtools_chan.as_ref(), + request.target_webview_id.map(|id| id.0), + request.pipeline_id, + ) { + // Build the partial DevtoolsHttpRequest + let devtools_request = DevtoolsHttpRequest { + url: request.current_url().clone(), + method: request.method.clone(), + headers: request.headers.clone(), + body: None, + pipeline_id, + started_date_time: SystemTime::now(), + time_stamp: 0, + connect_time: Duration::from_millis(0), + send_time: Duration::from_millis(0), + is_xhr: false, + browsing_context_id, + }; + + let msg = ChromeToDevtoolsControlMsg::NetworkEvent( + request.id.0.to_string(), + NetworkEvent::HttpRequest(devtools_request), + ); + + send_request_to_devtools(msg, &devtools_chan.lock().unwrap()); + } } fn auth_from_cache( @@ -885,6 +934,8 @@ pub async fn http_fetch( .try_code() .is_some_and(is_redirect_status) { + // Notify devtools before handling redirect + send_response_to_devtools(request, context, &response); // Substep 1. if response.actual_response().status != StatusCode::SEE_OTHER { // TODO: send RST_STREAM frame @@ -1867,12 +1918,7 @@ async fn http_network_fetch( // Step 5 let url = request.current_url(); - - let request_id = context - .devtools_chan - .as_ref() - .map(|_| uuid::Uuid::new_v4().simple().to_string()); - + let request_id = request.id.0.to_string(); if log_enabled!(log::Level::Info) { info!("{:?} request for {}", request.method, url); for header in request.headers.iter() { @@ -1912,14 +1958,13 @@ async fn http_network_fetch( .map(|body| body.source_is_null()) .unwrap_or(false), &request.pipeline_id, - request_id.as_deref(), + Some(&request_id), is_xhr, context, fetch_terminated_sender, browsing_context_id, ); - let pipeline_id = request.pipeline_id; // This will only get the headers, the body is read later let (res, msg) = match response_future.await { Ok(wrapped_response) => wrapped_response, @@ -1995,17 +2040,8 @@ async fn http_network_fetch( // We're about to spawn a future to be waited on here let (done_sender, done_receiver) = unbounded_channel(); *done_chan = Some((done_sender.clone(), done_receiver)); - let meta = match response - .metadata() - .expect("Response metadata should exist at this stage") - { - FetchMetadata::Unfiltered(m) => m, - FetchMetadata::Filtered { unsafe_, .. } => unsafe_, - }; let devtools_sender = context.devtools_chan.clone(); - let meta_status = meta.status; - let meta_headers = meta.headers; let cancellation_listener = context.cancellation_listener.clone(); if cancellation_listener.cancelled() { return Response::network_error(NetworkError::Internal("Fetch aborted".into())); @@ -2019,19 +2055,6 @@ async fn http_network_fetch( if let Some(m) = msg { send_request_to_devtools(m, &sender); } - - // --- Tell devtools that we got a response - // Send an HttpResponse message to devtools with the corresponding request_id - if let (Some(pipeline_id), Some(browsing_context_id)) = (pipeline_id, browsing_context_id) { - send_response_to_devtools( - &sender, - request_id.unwrap(), - meta_headers.map(Serde::into_inner), - meta_status, - pipeline_id, - browsing_context_id, - ); - } } let done_sender2 = done_sender.clone(); diff --git a/components/net/tests/fetch.rs b/components/net/tests/fetch.rs index a17f1f9d35c..26df87d999b 100644 --- a/components/net/tests/fetch.rs +++ b/components/net/tests/fetch.rs @@ -1295,7 +1295,7 @@ fn test_fetch_with_devtools() { let _ = server.close(); // notification received from devtools - let devhttprequest = expect_devtools_http_request(&devtools_port); + let devhttprequests = expect_devtools_http_request(&devtools_port); let mut devhttpresponse = expect_devtools_http_response(&devtools_port); //Creating default headers for request @@ -1335,10 +1335,10 @@ fn test_fetch_with_devtools() { headers: headers, body: Some(vec![]), pipeline_id: TEST_PIPELINE_ID, - started_date_time: devhttprequest.started_date_time, - time_stamp: devhttprequest.time_stamp, - connect_time: devhttprequest.connect_time, - send_time: devhttprequest.send_time, + started_date_time: devhttprequests.1.started_date_time, + time_stamp: devhttprequests.1.time_stamp, + connect_time: devhttprequests.1.connect_time, + send_time: devhttprequests.1.send_time, is_xhr: true, browsing_context_id: TEST_WEBVIEW_ID.0, }; @@ -1360,7 +1360,7 @@ fn test_fetch_with_devtools() { browsing_context_id: TEST_WEBVIEW_ID.0, }; - assert_eq!(devhttprequest, httprequest); + assert_eq!(devhttprequests.1, httprequest); assert_eq!(devhttpresponse, httpresponse); } diff --git a/components/net/tests/http_loader.rs b/components/net/tests/http_loader.rs index 9c9abd05970..23896823524 100644 --- a/components/net/tests/http_loader.rs +++ b/components/net/tests/http_loader.rs @@ -67,21 +67,28 @@ fn assert_cookie_for_domain( assert_eq!(cookies.as_ref().map(|c| &**c), cookie); } -pub fn expect_devtools_http_request( - devtools_port: &Receiver, -) -> DevtoolsHttpRequest { +fn recv_http_request(devtools_port: &Receiver) -> DevtoolsHttpRequest { match devtools_port.recv().unwrap() { DevtoolsControlMsg::FromChrome(ChromeToDevtoolsControlMsg::NetworkEvent(_, net_event)) => { match net_event { - NetworkEvent::HttpRequest(httprequest) => httprequest, - - _ => panic!("No HttpRequest Received"), + NetworkEvent::HttpRequest(req) => req, + NetworkEvent::HttpRequestUpdate(req) => req, + other => panic!("Expected HttpRequest but got: {:?}", other), } }, - _ => panic!("No HttpRequest Received"), + other => panic!("Expected NetworkEvent but got: {:?}", other), } } +pub fn expect_devtools_http_request( + devtools_port: &Receiver, +) -> (DevtoolsHttpRequest, DevtoolsHttpRequest) { + ( + recv_http_request(devtools_port), + recv_http_request(devtools_port), + ) +} + pub fn expect_devtools_http_response( devtools_port: &Receiver, ) -> DevtoolsHttpResponse { @@ -92,9 +99,9 @@ pub fn expect_devtools_http_response( )) => match net_event_response { NetworkEvent::HttpResponse(httpresponse) => httpresponse, - _ => panic!("No HttpResponse Received"), + other => panic!("Expected HttpResponse but got: {:?}", other), }, - _ => panic!("No HttpResponse Received"), + other => panic!("Expected NetworkEvent but got: {:?}", other), } } @@ -275,7 +282,7 @@ fn test_request_and_response_data_with_network_messages() { let _ = server.close(); // notification received from devtools - let devhttprequest = expect_devtools_http_request(&devtools_port); + let devhttprequests = expect_devtools_http_request(&devtools_port); let devhttpresponse = expect_devtools_http_response(&devtools_port); //Creating default headers for request @@ -322,10 +329,10 @@ fn test_request_and_response_data_with_network_messages() { headers: headers, body: Some(vec![]), pipeline_id: TEST_PIPELINE_ID, - started_date_time: devhttprequest.started_date_time, - time_stamp: devhttprequest.time_stamp, - connect_time: devhttprequest.connect_time, - send_time: devhttprequest.send_time, + started_date_time: devhttprequests.1.started_date_time, + time_stamp: devhttprequests.1.time_stamp, + connect_time: devhttprequests.1.connect_time, + send_time: devhttprequests.1.send_time, is_xhr: false, browsing_context_id: TEST_WEBVIEW_ID.0, }; @@ -352,7 +359,7 @@ fn test_request_and_response_data_with_network_messages() { browsing_context_id: TEST_WEBVIEW_ID.0, }; - assert_eq!(devhttprequest, httprequest); + assert_eq!(devhttprequests.1, httprequest); assert_eq!(devhttpresponse, httpresponse); } @@ -421,21 +428,21 @@ fn test_redirected_request_to_devtools() { let _ = pre_server.close(); let _ = post_server.close(); - let devhttprequest = expect_devtools_http_request(&devtools_port); + let devhttprequests = expect_devtools_http_request(&devtools_port); let devhttpresponse = expect_devtools_http_response(&devtools_port); - assert_eq!(devhttprequest.method, Method::POST); - assert_eq!(devhttprequest.url, pre_url); + assert_eq!(devhttprequests.0.method, Method::POST); + assert_eq!(devhttprequests.0.url, pre_url); assert_eq!( devhttpresponse.status, HttpStatus::from(StatusCode::MOVED_PERMANENTLY) ); - let devhttprequest = expect_devtools_http_request(&devtools_port); + let devhttprequests = expect_devtools_http_request(&devtools_port); let devhttpresponse = expect_devtools_http_response(&devtools_port); - assert_eq!(devhttprequest.method, Method::GET); - assert_eq!(devhttprequest.url, post_url); + assert_eq!(devhttprequests.0.method, Method::GET); + assert_eq!(devhttprequests.0.url, post_url); assert_eq!(devhttpresponse.status, HttpStatus::default()); } diff --git a/components/shared/devtools/lib.rs b/components/shared/devtools/lib.rs index f4c468b1d5d..6d4ab0f8595 100644 --- a/components/shared/devtools/lib.rs +++ b/components/shared/devtools/lib.rs @@ -459,6 +459,7 @@ pub struct HttpResponse { #[derive(Debug)] pub enum NetworkEvent { HttpRequest(HttpRequest), + HttpRequestUpdate(HttpRequest), HttpResponse(HttpResponse), } diff --git a/components/shared/net/request.rs b/components/shared/net/request.rs index 7b39ac7b78d..12d960357b4 100644 --- a/components/shared/net/request.rs +++ b/components/shared/net/request.rs @@ -22,7 +22,7 @@ use crate::{ReferrerPolicy, ResourceTimingType}; #[derive(Clone, Copy, Debug, Deserialize, Eq, Hash, MallocSizeOf, PartialEq, Serialize)] /// An id to differeniate one network request from another. -pub struct RequestId(Uuid); +pub struct RequestId(pub Uuid); impl Default for RequestId { fn default() -> Self {