devtools: Fix id collisions by using incrementing counters (#35971)

Devtools clients need a `browserId`, `browsingContextID`, and
`outerWindowID`, which correspond to WebViewId, BrowsingContextId, and
PipelineId in Servo. These u32 values were previously derived from our
sharded (u32,u32) id values by taking only the `index` (second u32) and
ignoring the `namespace_id` (first u32), leading to collisions.

This patch fixes that by mapping those Servo ids to sequential u32
values.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by
`[X]` when the step is complete, and replace `___` with appropriate
data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #35954

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox
is checked, so that we can help you if you get stuck somewhere along the
way.-->

<!-- Pull requests that do not address these steps are welcome, but they
will require additional verification as part of the review process. -->

---------

Signed-off-by: Delan Azabani <dazabani@igalia.com>

components/devtools/actors/browsing_context.rs

@@ -10,7 +10,7 @@ use std::cell::{Cell, RefCell};
 use std::collections::HashMap;
 use std::net::TcpStream;
 
-use base::id::{BrowsingContextId, PipelineId, WebViewId};
+use base::id::PipelineId;
 use devtools_traits::DevtoolScriptControlMsg::{self, GetCssDatabase, WantsLiveNotifications};
 use devtools_traits::{DevtoolsPageInfo, NavigationState};
 use ipc_channel::ipc::{self, IpcSender};
@@ -26,6 +26,7 @@ use crate::actors::stylesheets::StyleSheetsActor;
 use crate::actors::tab::TabDescriptorActor;
 use crate::actors::thread::ThreadActor;
 use crate::actors::watcher::{SessionContext, SessionContextType, WatcherActor};
+use crate::id::{DevtoolsBrowserId, DevtoolsBrowsingContextId, DevtoolsOuterWindowId, IdMap};
 use crate::protocol::JsonPacketStream;
 use crate::{EmptyReplyMsg, StreamId};
 
@@ -124,10 +125,11 @@ pub(crate) struct BrowsingContextActor {
     pub name: String,
     pub title: RefCell<String>,
     pub url: RefCell<String>,
-    /// This correspond to webview_id
-    pub browser_id: WebViewId,
-    pub active_pipeline: Cell<PipelineId>,
-    pub browsing_context_id: BrowsingContextId,
+    /// This corresponds to webview_id
+    pub browser_id: DevtoolsBrowserId,
+    pub active_pipeline_id: Cell<PipelineId>,
+    pub active_outer_window_id: Cell<DevtoolsOuterWindowId>,
+    pub browsing_context_id: DevtoolsBrowsingContextId,
     pub accessibility: String,
     pub console: String,
     pub css_properties: String,
@@ -169,19 +171,21 @@ impl Actor for BrowsingContextActor {
         self.streams.borrow_mut().remove(&id);
         if self.streams.borrow().is_empty() {
             self.script_chan
-                .send(WantsLiveNotifications(self.active_pipeline.get(), false))
+                .send(WantsLiveNotifications(self.active_pipeline_id.get(), false))
                 .unwrap();
         }
     }
 }
 
 impl BrowsingContextActor {
+    #[allow(clippy::too_many_arguments)]
     pub(crate) fn new(
         console: String,
-        browser_id: WebViewId,
-        browsing_context_id: BrowsingContextId,
+        browser_id: DevtoolsBrowserId,
+        browsing_context_id: DevtoolsBrowsingContextId,
         page_info: DevtoolsPageInfo,
         pipeline_id: PipelineId,
+        outer_window_id: DevtoolsOuterWindowId,
         script_sender: IpcSender<DevtoolScriptControlMsg>,
         actors: &mut ActorRegistry,
     ) -> BrowsingContextActor {
@@ -230,7 +234,8 @@ impl BrowsingContextActor {
             script_chan: script_sender,
             title: RefCell::new(title),
             url: RefCell::new(url.into_string()),
-            active_pipeline: Cell::new(pipeline_id),
+            active_pipeline_id: Cell::new(pipeline_id),
+            active_outer_window_id: Cell::new(outer_window_id),
             browser_id,
             browsing_context_id,
             accessibility: accessibility.name(),
@@ -270,11 +275,9 @@ impl BrowsingContextActor {
             },
             title: self.title.borrow().clone(),
             url: self.url.borrow().clone(),
-            browser_id: self.browser_id.0.index.0.get(),
-            //FIXME: shouldn't ignore pipeline namespace field
-            browsing_context_id: self.browsing_context_id.index.0.get(),
-            //FIXME: shouldn't ignore pipeline namespace field
-            outer_window_id: self.active_pipeline.get().index.0.get(),
+            browser_id: self.browser_id.value(),
+            browsing_context_id: self.browsing_context_id.value(),
+            outer_window_id: self.active_outer_window_id.get().value(),
             is_top_level_target: true,
             accessibility_actor: self.accessibility.clone(),
             console_actor: self.console.clone(),
@@ -286,15 +289,17 @@ impl BrowsingContextActor {
         }
     }
 
-    pub(crate) fn navigate(&self, state: NavigationState) {
-        let (pipeline, title, url, state) = match state {
+    pub(crate) fn navigate(&self, state: NavigationState, id_map: &mut IdMap) {
+        let (pipeline_id, title, url, state) = match state {
             NavigationState::Start(url) => (None, None, url, "start"),
             NavigationState::Stop(pipeline, info) => {
                 (Some(pipeline), Some(info.title), info.url, "stop")
             },
         };
-        if let Some(p) = pipeline {
-            self.active_pipeline.set(p);
+        if let Some(pipeline_id) = pipeline_id {
+            let outer_window_id = id_map.outer_window_id(pipeline_id);
+            self.active_outer_window_id.set(outer_window_id);
+            self.active_pipeline_id.set(pipeline_id);
         }
         url.as_str().clone_into(&mut self.url.borrow_mut());
         if let Some(ref t) = title {
@@ -317,7 +322,7 @@ impl BrowsingContextActor {
     }
 
     pub(crate) fn title_changed(&self, pipeline_id: PipelineId, title: String) {
-        if pipeline_id != self.active_pipeline.get() {
+        if pipeline_id != self.active_pipeline_id.get() {
             return;
         }
         *self.title.borrow_mut() = title;
@@ -328,7 +333,7 @@ impl BrowsingContextActor {
             from: self.name(),
             type_: "frameUpdate".into(),
             frames: vec![FrameUpdateMsg {
-                id: self.browsing_context_id.index.0.get(),
+                id: self.browsing_context_id.value(),
                 is_top_level: true,
                 title: self.title.borrow().clone(),
                 url: self.url.borrow().clone(),

components/devtools/actors/console.rs

@@ -152,7 +152,7 @@ impl ConsoleActor {
             Root::BrowsingContext(bc) => UniqueId::Pipeline(
                 registry
                     .find::<BrowsingContextActor>(bc)
-                    .active_pipeline
+                    .active_pipeline_id
                     .get(),
             ),
             Root::DedicatedWorker(w) => UniqueId::Worker(registry.find::<WorkerActor>(w).worker_id),

components/devtools/actors/inspector.rs

@@ -80,7 +80,7 @@ impl Actor for InspectorActor {
         _id: StreamId,
     ) -> Result<ActorMessageStatus, ()> {
         let browsing_context = registry.find::<BrowsingContextActor>(&self.browsing_context);
-        let pipeline = browsing_context.active_pipeline.get();
+        let pipeline = browsing_context.active_pipeline_id.get();
         Ok(match msg_type {
             "getWalker" => {
                 let (tx, rx) = ipc::channel().unwrap();

components/devtools/actors/tab.rs

@@ -146,18 +146,15 @@ impl TabDescriptorActor {
 
     pub fn encodable(&self, registry: &ActorRegistry, selected: bool) -> TabDescriptorActorMsg {
         let ctx_actor = registry.find::<BrowsingContextActor>(&self.browsing_context_actor);
-        let browser_id = ctx_actor.browser_id.0.index.0.get();
-        let browsing_context_id = ctx_actor.browsing_context_id.index.0.get();
-        let outer_window_id = ctx_actor.active_pipeline.get().index.0.get();
         let title = ctx_actor.title.borrow().clone();
         let url = ctx_actor.url.borrow().clone();
 
         TabDescriptorActorMsg {
             actor: self.name(),
-            browser_id,
-            browsing_context_id,
+            browser_id: ctx_actor.browser_id.value(),
+            browsing_context_id: ctx_actor.browsing_context_id.value(),
             is_zombie_tab: false,
-            outer_window_id,
+            outer_window_id: ctx_actor.active_outer_window_id.get().value(),
             selected,
             title,
             traits: DescriptorTraits {

components/devtools/actors/watcher.rs

@@ -259,10 +259,9 @@ impl Actor for WatcherActor {
                 ActorMessageStatus::Processed
             },
             "getParentBrowsingContextID" => {
-                let browsing_context_id = target.browsing_context_id.index.0.get();
                 let msg = GetParentBrowsingContextIDReply {
                     from: self.name(),
-                    browsing_context_id,
+                    browsing_context_id: target.browsing_context_id.value(),
                 };
                 let _ = stream.write_json_packet(&msg);
                 ActorMessageStatus::Processed

components/devtools/id.rs

@@ -0,0 +1,175 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
+
+use std::collections::HashMap;
+
+use base::id::{BrowsingContextId, PipelineId, WebViewId};
+
+#[derive(Debug, Default)]
+pub(crate) struct IdMap {
+    pub(crate) browser_ids: HashMap<WebViewId, u32>,
+    pub(crate) browsing_context_ids: HashMap<BrowsingContextId, u32>,
+    pub(crate) outer_window_ids: HashMap<PipelineId, u32>,
+}
+
+impl IdMap {
+    pub(crate) fn browser_id(&mut self, webview_id: WebViewId) -> DevtoolsBrowserId {
+        let len = self
+            .browser_ids
+            .len()
+            .checked_add(1)
+            .expect("WebViewId count overflow")
+            .try_into()
+            .expect("DevtoolsBrowserId overflow");
+        DevtoolsBrowserId(*self.browser_ids.entry(webview_id).or_insert(len))
+    }
+    pub(crate) fn browsing_context_id(
+        &mut self,
+        browsing_context_id: BrowsingContextId,
+    ) -> DevtoolsBrowsingContextId {
+        let len = self
+            .browsing_context_ids
+            .len()
+            .checked_add(1)
+            .expect("BrowsingContextId count overflow")
+            .try_into()
+            .expect("DevtoolsBrowsingContextId overflow");
+        DevtoolsBrowsingContextId(
+            *self
+                .browsing_context_ids
+                .entry(browsing_context_id)
+                .or_insert(len),
+        )
+    }
+    pub(crate) fn outer_window_id(&mut self, pipeline_id: PipelineId) -> DevtoolsOuterWindowId {
+        let len = self
+            .outer_window_ids
+            .len()
+            .checked_add(1)
+            .expect("PipelineId count overflow")
+            .try_into()
+            .expect("DevtoolsOuterWindowId overflow");
+        DevtoolsOuterWindowId(*self.outer_window_ids.entry(pipeline_id).or_insert(len))
+    }
+}
+
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub(crate) struct DevtoolsBrowserId(u32);
+
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub(crate) struct DevtoolsBrowsingContextId(u32);
+
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub(crate) struct DevtoolsOuterWindowId(u32);
+
+impl DevtoolsBrowserId {
+    pub(crate) fn value(&self) -> u32 {
+        self.0
+    }
+}
+
+impl DevtoolsBrowsingContextId {
+    pub(crate) fn value(&self) -> u32 {
+        self.0
+    }
+}
+
+impl DevtoolsOuterWindowId {
+    pub(crate) fn value(&self) -> u32 {
+        self.0
+    }
+}
+
+#[test]
+pub(crate) fn test_id_map() {
+    use std::thread;
+
+    use base::id::{PipelineNamespace, PipelineNamespaceId};
+    use crossbeam_channel::unbounded;
+
+    macro_rules! test_sequential_id_assignment {
+        ($id_type:ident, $new_id_function:expr, $map_id_function:expr) => {
+            let (sender, receiver) = unbounded();
+            let sender1 = sender.clone();
+            let sender2 = sender.clone();
+            let sender3 = sender.clone();
+            let threads = [
+                thread::spawn(move || {
+                    PipelineNamespace::install(PipelineNamespaceId(1));
+                    sender1.send($new_id_function()).expect("Send failed");
+                    sender1.send($new_id_function()).expect("Send failed");
+                    sender1.send($new_id_function()).expect("Send failed");
+                }),
+                thread::spawn(move || {
+                    PipelineNamespace::install(PipelineNamespaceId(2));
+                    sender2.send($new_id_function()).expect("Send failed");
+                    sender2.send($new_id_function()).expect("Send failed");
+                    sender2.send($new_id_function()).expect("Send failed");
+                }),
+                thread::spawn(move || {
+                    PipelineNamespace::install(PipelineNamespaceId(3));
+                    sender3.send($new_id_function()).expect("Send failed");
+                    sender3.send($new_id_function()).expect("Send failed");
+                    sender3.send($new_id_function()).expect("Send failed");
+                }),
+            ];
+            for thread in threads {
+                thread.join().expect("Thread join failed");
+            }
+            let mut id_map = IdMap::default();
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(1)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(2)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(3)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(4)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(5)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(6)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(7)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(8)
+            );
+            assert_eq!(
+                $map_id_function(&mut id_map, receiver.recv().expect("Recv failed")),
+                $id_type(9)
+            );
+        };
+    }
+
+    test_sequential_id_assignment!(
+        DevtoolsBrowserId,
+        || WebViewId::new(),
+        |id_map: &mut IdMap, id| id_map.browser_id(id)
+    );
+    test_sequential_id_assignment!(
+        DevtoolsBrowsingContextId,
+        || BrowsingContextId::new(),
+        |id_map: &mut IdMap, id| id_map.browsing_context_id(id)
+    );
+    test_sequential_id_assignment!(
+        DevtoolsOuterWindowId,
+        || PipelineId::new(),
+        |id_map: &mut IdMap, id| id_map.outer_window_id(id)
+    );
+}

components/devtools/lib.rs

@@ -44,6 +44,7 @@ use crate::actors::process::ProcessActor;
 use crate::actors::root::RootActor;
 use crate::actors::thread::ThreadActor;
 use crate::actors::worker::{WorkerActor, WorkerType};
+use crate::id::IdMap;
 use crate::network_handler::handle_network_event;
 use crate::protocol::JsonPacketStream;
 
@@ -70,6 +71,7 @@ mod actors {
     pub mod watcher;
     pub mod worker;
 }
+mod id;
 mod network_handler;
 mod protocol;
 
@@ -106,6 +108,7 @@ pub(crate) struct StreamId(u32);
 
 struct DevtoolsInstance {
     actors: Arc<Mutex<ActorRegistry>>,
+    id_map: Arc<Mutex<IdMap>>,
     browsing_contexts: HashMap<BrowsingContextId, String>,
     receiver: Receiver<DevtoolsControlMsg>,
     pipelines: HashMap<PipelineId, BrowsingContextId>,
@@ -167,6 +170,7 @@ impl DevtoolsInstance {
 
         let instance = Self {
             actors,
+            id_map: Arc::new(Mutex::new(IdMap::default())),
             browsing_contexts: HashMap::new(),
             pipelines: HashMap::new(),
             receiver,
@@ -299,7 +303,7 @@ impl DevtoolsInstance {
             .lock()
             .unwrap()
             .find::<BrowsingContextActor>(actor_name)
-            .navigate(state);
+            .navigate(state, &mut self.id_map.lock().expect("Mutex poisoned"));
     }
 
     // We need separate actor representations for each script global that exists;
@@ -314,6 +318,10 @@ impl DevtoolsInstance {
         let mut actors = self.actors.lock().unwrap();
 
         let (browsing_context_id, pipeline_id, worker_id, webview_id) = ids;
+        let id_map = &mut self.id_map.lock().expect("Mutex poisoned");
+        let devtools_browser_id = id_map.browser_id(webview_id);
+        let devtools_browsing_context_id = id_map.browsing_context_id(browsing_context_id);
+        let devtools_outer_window_id = id_map.outer_window_id(pipeline_id);
 
         let console_name = actors.new_name("console");
 
@@ -351,10 +359,11 @@ impl DevtoolsInstance {
                 .or_insert_with(|| {
                     let browsing_context_actor = BrowsingContextActor::new(
                         console_name.clone(),
-                        webview_id,
-                        browsing_context_id,
+                        devtools_browser_id,
+                        devtools_browsing_context_id,
                         page_info,
                         pipeline_id,
+                        devtools_outer_window_id,
                         script_sender,
                         &mut actors,
                     );

python/servo/testing_commands.py

@@ -163,6 +163,7 @@ class MachCommands(CommandBase):
             "background_hang_monitor",
             "base",
             "constellation",
+            "devtools",
             "fonts",
             "hyper_serde",
             "layout_2020",