Send WillNavigate earlier during navigation startup #37778

uthmaniv · 2025-06-29T22:31:13Z

The will-navigate message tells the devtools client to expect a navigation for a browsing context. This makes the network monitor clear any previous entries and show the requests for the new page that is loaded. In order to support this correctly, we need to send the navigation notification from the constellation instead of the script thread, otherwise we silently ignore navigations triggered by the browser URL bar.

Testing: Ran servo in devtools mode , now the requests appear for new loaded page
Fixes: #37334

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

…for WebViewId argument Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm · 2025-06-30T04:28:37Z

I figured out why the requests are now missing! The change from calling resource_available(..) on the BrowsingContextActor to calling it on the NetworkEventActor means that the from field in the message now refers to an actor that the client has never seen before, so the message gets ignored.

Since the Firefox-only logs show these messages being sent from the Watcher actors, I propose the following fix:
0. move the new ResourceAvailable impl from NetworkEventActor to WatcherActor

enable the network-event resource watching in

servo/components/devtools/actors/watcher.rs

Line 82 in 78cd770

("network-event", false),
add an empty branch for network-event to

servo/components/devtools/actors/watcher.rs

Line 324 in 78cd770

"console-message" | "error-message" => {},
add a new watcher_name field to NetworkEventActor, and take it as an argument in DevtoolsInstance::find_network_event_actor
using the new BrowsingContextId you've added to the devtools HttpRequest and HttpResponse, in handle_network_event, find the BrowsingContextActor for the given id and get the associated watcher name from the actor's public fields
find the watcher actor in network_handler::handle_network_event and use it to invoke the resource_available(..) methods

The end result should move us closer to the behaviour that Firefox expects!

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm

Good start!

jdm · 2025-07-03T02:03:28Z

components/devtools/actors/browsing_context.rs

+        let msg = WillNavigate {
+            browsing_context_id,
+            url: url.clone(),
+        };
+
+        for stream in self.streams.borrow_mut().values_mut() {
+            let _ = stream.write_json_packet(&msg);
+        }


This message is not right. We need a resource-available message that looks like https://gist.github.com/uthmaniv/611b99c3531499dc7170e8b11e19d206#file-gistfile1-txt-L463-L481 . We can use default values for innerWindowId and isFrameSwitching with a TODO about setting them correctly.

I believe we also want this message to be sent from the Watcher actor instead of the BrowsingContext actor, too.

Got it, I was initially contemplating to send it from th WatcherActor. Thanks for the clarification

jdm · 2025-07-03T02:05:15Z

components/shared/devtools/lib.rs

+    WillNavigate {
+        browsing_context_id: BrowsingContextId,
+        url: ServoUrl,
+    },


This message won't be used after my comments are addressed, so let's remove it.

jdm · 2025-07-03T02:05:51Z

components/script/dom/serviceworkerglobalscope.rs

                    .unwrap_or_else(|| global.upcast::<GlobalScope>().get_referrer());

-                let request = RequestBuilder::new(None, script_url, referrer)
+                let request = RequestBuilder::new(Some(TEST_WEBVIEW_ID), script_url, referrer)


We shouldn't use TEST_WEBVIEW_ID in non-test code. Let's keep this as None.

jdm · 2025-07-03T02:07:26Z

components/net/http_loader.rs

        let _ = fetch_terminated_sender.send(false);
    }

+    let browsing_context_id = request.target_webview_id.unwrap().0;


Instead of unwrapping this, we should pass the Option value.

jdm · 2025-07-03T02:08:37Z

components/net/http_loader.rs

                meta_headers.map(Serde::into_inner),
                meta_status,
                pipeline_id,
+                browsing_context_id,


Let's only send the information to the devtools if there is a non-None browsing context id.

jdm · 2025-07-03T02:09:20Z

components/net/http_loader.rs

    is_xhr: bool,
    context: &FetchContext,
    fetch_terminated: UnboundedSender<bool>,
+    browsing_context_id: BrowsingContextId,


Let's make this Option<BrowsingContextId>.

jdm · 2025-07-03T02:10:12Z

components/devtools/network_handler.rs

    mut connections: Vec<TcpStream>,
    network_event: NetworkEvent,
-    browsing_context_actor_name: String,
+    watcher_name: String,


We don't need to pass this here; it's already stored as a field in NetworkEventActor.

jdm · 2025-07-03T02:12:14Z

components/devtools/lib.rs

+            NetworkEvent::HttpRequest(req) => Some(req.browsing_context_id),
+            NetworkEvent::HttpResponse(resp) => Some(resp.browsing_context_id),


Suggested change

NetworkEvent::HttpRequest(req) => Some(req.browsing_context_id),

NetworkEvent::HttpResponse(resp) => Some(resp.browsing_context_id),

NetworkEvent::HttpRequest(req) => req.browsing_context_id,

NetworkEvent::HttpResponse(resp) => resp.browsing_context_id,

jdm · 2025-07-03T02:17:40Z

components/devtools/lib.rs

+        let watcher_name = browsing_context_id
+            .and_then(|id| self.browsing_contexts.get(&id))
+            .map(|actor_name| {
+                self.actors
+                    .lock()
+                    .unwrap()
+                    .find::<BrowsingContextActor>(actor_name)
+                    .watcher
+                    .clone()
+            });
+
+        if watcher_name.is_none() {


Let's do:

let Some(browsing_context_actor_name) = self.browsing_contexts.get(&browsing_context_id) else { return }; let watcher_name = self.actors .lock() .unwrap() .find::<BrowsingContextActor>(browsing_context_actor_name) .watcher .clone();

Then there are no unwraps required and it's clear where any failure needs to be handled.

jdm · 2025-07-03T02:48:09Z

components/devtools/lib.rs

+        let binding = self.actors.lock().unwrap();
+        let actor = binding


Suggested change

let binding = self.actors.lock().unwrap();

let actor = binding

let actors = self.actors.lock().unwrap();

let actor = actors

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm

Really close now! Have you tested the changes? Does navigating work as expected now?

components/devtools/network_handler.rs

jdm · 2025-07-03T18:43:10Z

components/net/http_loader.rs

                            (send_end - send_start).unsigned_abs(),
                            is_xhr,
-                            browsing_context_id,
+                            browsing_context_id.unwrap(),


We need another if let around prepare_devtools_request instead of using unwrap here.

jdm · 2025-07-03T18:44:18Z

components/devtools/actors/watcher.rs

+        url: ServoUrl,
+        connections: &mut Vec<TcpStream>,
+    ) {
+        let msg = serde_json::json!({


Can we create a new struct for this message and its fields instead of using the json macro?

jdm · 2025-07-03T18:44:58Z

components/devtools/actors/watcher.rs

+        connections: &mut Vec<TcpStream>,
+    ) {
+        let msg = serde_json::json!({
+            "browsingContextID": browsing_context_id,


We will need to use browsing_context_id.0 to get the id number instead of the string representation.

uthmaniv · 2025-07-03T18:56:59Z

Really close now! Have you tested the changes? Does navigating work as expected now?

Yeah I did test it but it still doesn't work as expected, there are some changes from the previous commits though.
Previous commit:

On refresh, it shows a single new request for the refreshed url
The single new request is included in the already present requests (does not clear the previous requests)
Current Commit:
No new request is shown for a refresh but clears previous request from the network panel

jdm · 2025-07-03T18:58:42Z

I suspect fixing the browsing context id value in the message will show an improvement.

uthmaniv · 2025-07-03T19:00:25Z

I suspect fixing the browsing context id value in the message will show an improvement.

Alright , we are getting closer :)

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm

I think one final set of changes will bring us success!

jdm · 2025-07-04T03:47:39Z

components/devtools/actors/watcher.rs

+#[derive(Clone, Serialize)]
+#[serde(rename_all = "camelCase")]
+pub struct WillNavigateMessage {
+    browsing_context_id: DevtoolsBrowsingContextId,


Suggested change

browsing_context_id: DevtoolsBrowsingContextId,

#[serde(rename = "browsingContextID")]

browsing_context_id: u32,

jdm · 2025-07-04T03:48:00Z

components/devtools/actors/watcher.rs

-            "newURI": url,
-        });
+            is_frame_switching: false, // TODO: Implement frame switching
+            new_URI: url,


Suggested change

new_URI: url,

new_uri: url,

jdm · 2025-07-04T03:48:27Z

components/devtools/actors/watcher.rs

-            "name": "will-navigate",
-            "time": SystemTime::now()
+        let msg = WillNavigateMessage {
+            browsing_context_id: id_map.browsing_context_id(browsing_context_id),


Suggested change

browsing_context_id: id_map.browsing_context_id(browsing_context_id),

browsing_context_id: id_map.browsing_context_id(browsing_context_id).value(),

jdm · 2025-07-04T03:48:54Z

components/devtools/id.rs

 pub(crate) struct DevtoolsBrowserId(u32);

-#[derive(Clone, Copy, Debug, PartialEq)]
+#[derive(Clone, Copy, Debug, PartialEq, Serialize)]


Suggested change

#[derive(Clone, Copy, Debug, PartialEq, Serialize)]

#[derive(Clone, Copy, Debug, PartialEq)]

jdm · 2025-07-04T03:49:02Z

components/devtools/id.rs

 use std::coll
6D3F
ections::HashMap;

 use base::id::{BrowsingContextId, PipelineId, WebViewId};
+use serde::Serialize;


Suggested change

use serde::Serialize;

jdm · 2025-07-04T05:30:59Z

components/devtools/lib.rs

-        actor.navigate(state, &mut self.id_map.lock().expect("Mutex poisoned"));
+
+        let mut id_map = self.id_map.lock().expect("Mutex poisoned");
+        watcher_actor.emit_will_navigate(


We need to ensure we only send the will-navigate message when the state is NavigationState::Start.

jdm · 2025-07-04T05:58:05Z

Besides the changes from my latest review comments, I experimented with your branch locally and have the following diff:

diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs
index 29d3c656b5..adf7e63181 100644
--- a/components/constellation/constellation.rs
+++ b/components/constellation/constellation.rs
@@ -3612,6 +3612,13 @@ where
                 },
             };

+        if let Some(ref chan) = self.devtools_sender {
+            let state = NavigationState::Start(load_data.url.clone());
+            let _ = chan.send(DevtoolsControlMsg::FromScript(
+                ScriptToDevtoolsControlMsg::Navigate(browsing_context_id, state),
+            ));
+        }
+
         match parent_pipeline_id {
             Some(parent_pipeline_id) => {
                 // Find the script thread for the pipeline containing the iframe
diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs
index d109e95616..f2b53e4cd2 100644
--- a/components/script/script_thread.rs
+++ b/components/script/script_thread.rs
@@ -642,13 +642,6 @@ impl ScriptThread {
                     .dom_manipulation_task_source()
                     .queue(task);
             } else {
-                if let Some(ref sender) = script_thread.senders.devtools_server_sender {
-                    let _ = sender.send(ScriptToDevtoolsControlMsg::Navigate(
-                        browsing_context,
-                        NavigationState::Start(load_data.url.clone()),
-                    ));
-                }
-
                 script_thread
                     .senders
                     .pipeline_to_constellation_sender

This change makes us send the NavigationState::Start message to devtools consistently for all navigations, including typing in the URL bar. Previously, we only sent it when initiating a navigation from within the script thread by clicking links, submitting forms, using window.location = some_url, etc.

This does not address the issue with refreshing a page. I have filed #37869 about that after some investigation. I think with the change applied from the diff (and subsequent warnings fixed), this PR is good enough to merge!

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

…ruction in net/tests/ Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm

Looking good! Can you update this PR's description so it reflects the latest status of these changes? Also, please move the PR out of draft mode :)

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

uthmaniv added 2 commits June 29, 2025 19:36

Send WillNavigate earlier during navigation startup

c69b94a

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

Pass TEST_WEBVIEW_ID for RequestBuilder::new callers with None value …

50972ca

…for WebViewId argument Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

uthmaniv added 2 commits July 1, 2025 14:46

Invoke resource_array on watcher_actor

6d0d15e

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

Handle WillNavigate message synchronously

ce161f9

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm requested changes Jul 3, 2025

View reviewed changes

Refactor emit_will_navigate to call resource_array on watcher_actor

1d0a492

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm requested changes Jul 3, 2025

View reviewed changes

Refactor msg in emit_will_navigate to a WillNavigateMessage struct

8cc6335

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm requested changes Jul 4, 2025

View reviewed changes

uthmaniv added 2 commits July 4, 2025 15:26

Send will-navigate only when state is NavigationState::Start

96bdeba

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

Add a test browsing_context_id for DevtoolsHttpResquest/Response cont…

eaa786a

…ruction in net/tests/ Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm approved these changes Jul 4, 2025

View reviewed changes

uthmaniv marked this pull request as ready for review July 4, 2025 15:22

uthmaniv requested a review from gterzian as a code owner July 4, 2025 15:22

Fix typo and remove unused import in net/tests/

9cd64d2

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

jdm enabled auto-merge July 4, 2025 16:02

Fix failing tests

e530e06

Signed-off-by: Uthman Yahaya Baba <uthmanyahayababa@gmail.com>

auto-merge was automatically disabled July 5, 2025 08:25
Head branch was pushed to by a user without write access

jdm added this pull request to the merge queue Jul 5, 2025

Merged via the queue into servo:main with commit 2ad5b24 Jul 5, 2025
21 checks passed

		NetworkEvent::HttpRequest(req) => Some(req.browsing_context_id),
		NetworkEvent::HttpResponse(resp) => Some(resp.browsing_context_id),

		let binding = self.actors.lock().unwrap();
		let actor = binding

	browsing_context_id: DevtoolsBrowsingContextId,
	#[serde(rename = "browsingContextID")]
	browsing_context_id: u32,

	browsing_context_id: id_map.browsing_context_id(browsing_context_id),
	browsing_context_id: id_map.browsing_context_id(browsing_context_id).value(),

	#[derive(Clone, Copy, Debug, PartialEq, Serialize)]
	#[derive(Clone, Copy, Debug, PartialEq)]

Uh oh!

Send WillNavigate earlier during navigation startup #37778

Send WillNavigate earlier during navigation startup #37778

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!