From ed6bf196c96891da0ade3d0662d4c69abfff5c9f Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Wed, 20 Aug 2025 02:43:16 -0400 Subject: [PATCH] constellation: Broadcast preference changes to all content processes (#38716) Building on the preference observer work from #38649, we now automatically install an observer when multiprocess mode is enabled. This observer notifies the constellation of updated preferences, which in turn notifies each content process so the changes will be reflected into script/layout as expected. There's a unit test that verifies this works correctly by checking a preference-gated WebIDL property before and after the preference is toggled. Testing: New unit test added. Fixes: #35966 Depends on #38649. --------- Signed-off-by: Josh Matthews --- Cargo.lock | 2 + components/config/prefs.rs | 12 ++-- components/constellation/constellation.rs | 32 ++++++++++ components/constellation/tracing.rs | 1 + components/script/messaging.rs | 1 + components/script/script_thread.rs | 9 ++- components/servo/Cargo.toml | 4 ++ components/servo/tests/common/mod.rs | 63 +++++++++++++++++--- components/servo/tests/multiprocess.rs | 68 ++++++++++++++++++++++ components/servo/tests/servo.rs | 1 + components/servo/tests/webview.rs | 46 +-------------- components/shared/constellation/Cargo.toml | 1 + components/shared/constellation/lib.rs | 3 + components/shared/script/Cargo.toml | 1 + components/shared/script/lib.rs | 3 + ports/servoshell/desktop/webxr.rs | 4 +- 16 files changed, 191 insertions(+), 60 deletions(-) create mode 100644 components/servo/tests/multiprocess.rs diff --git a/Cargo.lock b/Cargo.lock index 43d728880c0..19504b0e435 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1499,6 +1499,7 @@ dependencies = [ "pixels", "profile_traits", "serde", + "servo_config", "servo_malloc_size_of", "servo_url", "strum", @@ -7309,6 +7310,7 @@ dependencies = [ "pixels", "profile_traits", "serde", + "servo_config", "servo_malloc_size_of", "servo_url", "strum", diff --git a/components/config/prefs.rs b/components/config/prefs.rs index c21679995d7..dd17204698e 100644 --- a/components/config/prefs.rs +++ b/components/config/prefs.rs @@ -12,10 +12,10 @@ pub use crate::pref_util::PrefValue; static PREFERENCES: RwLock = RwLock::new(Preferences::const_default()); pub trait Observer: Send + Sync { - fn prefs_changed(&self, _changes: Vec<(&'static str, PrefValue)>) {} + fn prefs_changed(&self, _changes: &[(&'static str, PrefValue)]) {} } -static OBSERVER: RwLock>> = RwLock::new(None); +static OBSERVERS: RwLock>> = RwLock::new(Vec::new()); #[inline] /// Get the current set of global preferences for Servo. @@ -23,8 +23,8 @@ pub fn get() -> RwLockReadGuard<'static, Preferences> { PREFERENCES.read().unwrap() } -pub fn set_observer(observer: Box) { - *OBSERVER.write().unwrap() = Some(observer); +pub fn add_observer(observer: Box) { + OBSERVERS.write().unwrap().push(observer); } pub fn set(preferences: Preferences) { @@ -57,8 +57,8 @@ pub fn set(preferences: Preferences) { *PREFERENCES.write().unwrap() = preferences; - if let Some(observer) = OBSERVER.read().unwrap().as_deref() { - observer.prefs_changed(changed); + for observer in &*OBSERVERS.read().unwrap() { + observer.prefs_changed(&changed); } } diff --git a/components/constellation/constellation.rs b/components/constellation/constellation.rs index 3d84d95636e..e199efa3e04 100644 --- a/components/constellation/constellation.rs +++ b/components/constellation/constellation.rs @@ -160,6 +160,7 @@ use script_traits::{ ScriptThreadMessage, UpdatePipelineIdReason, }; use serde::{Deserialize, Serialize}; +use servo_config::prefs::{self, PrefValue}; use servo_config::{opts, pref}; use servo_rand::{Rng, ServoRng, SliceRandom, random}; use servo_url::{Host, ImmutableOrigin, ServoUrl}; @@ -254,6 +255,18 @@ struct BrowsingContextGroup { webgpus: HashMap, } +struct PreferenceForwarder(Sender); + +impl prefs::Observer for PreferenceForwarder { + fn prefs_changed(&self, changes: &[(&'static str, PrefValue)]) { + let _ = self + .0 + .send(EmbedderToConstellationMessage::PreferencesUpdated( + changes.to_owned(), + )); + } +} + /// The `Constellation` itself. In the servo browser, there is one /// constellation, which maintains all of the browser global data. /// In embedded applications, there may be more than one constellation, @@ -583,6 +596,7 @@ where hard_fail: bool, ) -> Sender { let (compositor_sender, compositor_receiver) = unbounded(); + let compositor_sender_self = compositor_sender.clone(); // service worker manager to communicate with constellation let (swmanager_ipc_sender, swmanager_ipc_receiver) = @@ -654,6 +668,10 @@ where let rippy_data = resources::read_bytes(Resource::RippyPNG); + if opts::get().multiprocess { + prefs::add_observer(Box::new(PreferenceForwarder(compositor_sender_self))); + } + let mut constellation: Constellation = Constellation { namespace_receiver, namespace_ipc_sender, @@ -1507,6 +1525,20 @@ where EmbedderToConstellationMessage::SetWebDriverResponseSender(sender) => { self.webdriver_input_command_reponse_sender = Some(sender); }, + EmbedderToConstellationMessage::PreferencesUpdated(updates) => { + let event_loops = self + .pipelines + .values() + .map(|pipeline| pipeline.event_loop.clone()); + for event_loop in event_loops { + let _ = event_loop.send(ScriptThreadMessage::PreferencesUpdated( + updates + .iter() + .map(|(name, value)| (String::from(*name), value.clone())) + .collect(), + )); + } + }, } } diff --git a/components/constellation/tracing.rs b/components/constellation/tracing.rs index 4464472e118..e64e58844bf 100644 --- a/components/constellation/tracing.rs +++ b/components/constellation/tracing.rs @@ -78,6 +78,7 @@ mod from_compositor { Self::CreateMemoryReport(..) => target!("CreateMemoryReport"), Self::SendImageKeysForPipeline(..) => target!("SendImageKeysForPipeline"), Self::SetWebDriverResponseSender(..) => target!("SetWebDriverResponseSender"), + Self::PreferencesUpdated(..) => target!("PreferencesUpdated"), } } } diff --git a/components/script/messaging.rs b/components/script/messaging.rs index d93aa2f4649..a0b21cfbe27 100644 --- a/components/script/messaging.rs +++ b/components/script/messaging.rs @@ -97,6 +97,7 @@ impl MixedMessage { ScriptThreadMessage::SetScrollStates(id, ..) => Some(*id), ScriptThreadMessage::EvaluateJavaScript(id, _, _) => Some(*id), ScriptThreadMessage::SendImageKeysBatch(..) => None, + ScriptThreadMessage::PreferencesUpdated(..) => None, }, MixedMessage::FromScript(inner_msg) => match inner_msg { MainThreadScriptMsg::Common(CommonScriptMsg::Task(_, _, pipeline_id, _)) => { diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index f97473d8118..5001b1efe6d 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -88,7 +88,7 @@ use script_traits::{ ConstellationInputEvent, DiscardBrowsingContext, DocumentActivity, InitialScriptState, NewLayoutInfo, Painter, ProgressiveWebMetricType, ScriptThreadMessage, UpdatePipelineIdReason, }; -use servo_config::opts; +use servo_config::{opts, prefs}; use servo_url::{ImmutableOrigin, MutableOrigin, ServoUrl}; use style::thread_state::{self, ThreadState}; use style_traits::CSSPixel; @@ -1887,6 +1887,13 @@ impl ScriptThread { ScriptThreadMessage::RefreshCursor(pipeline_id, cursor_position) => { self.handle_refresh_cursor(pipeline_id, cursor_position); }, + ScriptThreadMessage::PreferencesUpdated(updates) => { + let mut current_preferences = prefs::get().clone(); + for (name, value) in updates { + current_preferences.set_value(&name, value); + } + prefs::set(current_preferences); + }, } } diff --git a/components/servo/Cargo.toml b/components/servo/Cargo.toml index 2c1f75dca13..9c246cb760e 100644 --- a/components/servo/Cargo.toml +++ b/components/servo/Cargo.toml @@ -149,3 +149,7 @@ harness = false [[test]] name = "servo" harness = false + +[[test]] +name = "multiprocess" +harness = false diff --git a/components/servo/tests/common/mod.rs b/components/servo/tests/common/mod.rs index de71361e9be..cd75b52225b 100644 --- a/components/servo/tests/common/mod.rs +++ b/components/servo/tests/common/mod.rs @@ -2,6 +2,7 @@ * 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::cell::{Cell, RefCell}; use std::rc::Rc; use std::sync::Arc; use std::sync::atomic::{AtomicBool, Ordering}; @@ -11,15 +12,20 @@ use anyhow::Error; use compositing_traits::rendering_context::{RenderingContext, SoftwareRenderingContext}; use dpi::PhysicalSize; use embedder_traits::EventLoopWaker; -use servo::{Servo, ServoBuilder}; +use servo::{ + JSValue, JavaScriptEvaluationError, LoadStatus, Servo, ServoBuilder, WebView, WebViewDelegate, +}; macro_rules! run_api_tests { ($($test_function:ident), +) => { + run_api_tests!(setup: |builder| builder, $($test_function),+) + }; + (setup: $builder:expr, $($test_function:ident), +) => { let mut failed = false; // Be sure that `servo_test` is dropped before exiting early. { - let servo_test = ServoTest::new(); + let servo_test = ServoTest::new($builder); $( common::run_test($test_function, stringify!($test_function), &servo_test, &mut failed); )+ @@ -28,7 +34,7 @@ macro_rules! run_api_tests { if failed { std::process::exit(1); } - } + }; } pub(crate) use run_api_tests; @@ -64,7 +70,10 @@ impl Drop for ServoTest { } impl ServoTest { - pub(crate) fn new() -> Self { + pub(crate) fn new(customize: F) -> Self + where + F: FnOnce(ServoBuilder) -> ServoBuilder, + { let rendering_context = Rc::new( SoftwareRenderingContext::new(PhysicalSize { width: 500, @@ -87,9 +96,10 @@ impl ServoTest { } let user_event_triggered = Arc::new(AtomicBool::new(false)); - let servo = ServoBuilder::new(rendering_context.clone()) - .event_loop_waker(Box::new(EventLoopWakerImpl(user_event_triggered))) - .build(); + let builder = ServoBuilder::new(rendering_context.clone()) + .event_loop_waker(Box::new(EventLoopWakerImpl(user_event_triggered))); + let builder = customize(builder); + let servo = builder.build(); Self { servo } } @@ -122,3 +132,42 @@ impl ServoTest { Ok(()) } } + +#[derive(Default)] +pub(crate) struct WebViewDelegateImpl { + pub(crate) url_changed: Cell, +} + +impl WebViewDelegateImpl { + pub(crate) fn reset(&self) { + self.url_changed.set(false); + } +} + +impl WebViewDelegate for WebViewDelegateImpl { + fn notify_url_changed(&self, _webview: servo::WebView, _url: url::Url) { + self.url_changed.set(true); + } +} + +pub(crate) fn evaluate_javascript( + servo_test: &ServoTest, + webview: WebView, + script: impl ToString, +) -> Result { + let load_webview = webview.clone(); + let _ = servo_test.spin(move || Ok(load_webview.load_status() != LoadStatus::Complete)); + + let saved_result = Rc::new(RefCell::new(None)); + let callback_result = saved_result.clone(); + webview.evaluate_javascript(script, move |result| { + *callback_result.borrow_mut() = Some(result) + }); + + let spin_result = saved_result.clone(); + let _ = servo_test.spin(move || Ok(spin_result.borrow().is_none())); + + (*saved_result.borrow()) + .clone() + .expect("Should have waited until value available") +} diff --git a/components/servo/tests/multiprocess.rs b/components/servo/tests/multiprocess.rs new file mode 100644 index 00000000000..cbd1255d110 --- /dev/null +++ b/components/servo/tests/multiprocess.rs @@ -0,0 +1,68 @@ +/* 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/. */ + +//! Unit tests exercising Servo API functionality with multiprocess mode enabled. +//! +//! Since all Servo tests must run serially on the same thread, it is important +//! that tests never panic. In order to ensure this, use `anyhow::ensure!` instead +//! of `assert!` for test assertions. `ensure!` will produce a `Result::Err` in +//! place of panicking. + +#[allow(dead_code)] +mod common; + +use std::rc::Rc; + +use anyhow::ensure; +use common::{ServoTest, WebViewDelegateImpl, evaluate_javascript, run_api_tests}; +use servo::{JSValue, WebViewBuilder, run_content_process}; +use servo_config::{opts, prefs}; + +fn test_multiprocess_preference_observer(servo_test: &ServoTest) -> Result<(), anyhow::Error> { + let delegate = Rc::new(WebViewDelegateImpl::default()); + let webview = WebViewBuilder::new(servo_test.servo()) + .delegate(delegate.clone()) + .build(); + + let result = evaluate_javascript(servo_test, webview.clone(), "window.gc"); + ensure!(matches!(result, Ok(JSValue::Undefined))); + + let mut prefs = prefs::get().clone(); + prefs.dom_servo_helpers_enabled = true; + prefs::set(prefs); + + webview.reload(); + + let result = evaluate_javascript(servo_test, webview.clone(), "window.gc"); + ensure!(matches!(result, Ok(JSValue::Object(..)))); + + Ok(()) +} + +fn main() { + let mut token = None; + let mut take_next = false; + for arg in std::env::args() { + if take_next { + token = Some(arg); + break; + } + if arg == "--content-process" { + take_next = true; + } + } + + if let Some(token) = token { + return run_content_process(token); + } + + run_api_tests!( + setup: |builder| { + let mut opts = opts::Opts::default(); + opts.multiprocess = true; + builder.opts(opts) + }, + test_multiprocess_preference_observer + ); +} diff --git a/components/servo/tests/servo.rs b/components/servo/tests/servo.rs index 6333b0af50a..6143b92f60a 100644 --- a/components/servo/tests/servo.rs +++ b/components/servo/tests/servo.rs @@ -9,6 +9,7 @@ //! of `assert!` for test assertions. `ensure!` will produce a `Result::Err` in //! place of panicking. +#[allow(dead_code)] mod common; use anyhow::ensure; diff --git a/components/servo/tests/webview.rs b/components/servo/tests/webview.rs index ccaa05d3ca3..3bf8b01bf83 100644 --- a/components/servo/tests/webview.rs +++ b/components/servo/tests/webview.rs @@ -11,33 +11,13 @@ mod common; -use std::cell::{Cell, RefCell}; use std::rc::Rc; use anyhow::ensure; -use common::{ServoTest, run_api_tests}; -use servo::{ - JSValue, JavaScriptEvaluationError, LoadStatus, Theme, WebView, WebViewBuilder, WebViewDelegate, -}; +use common::{ServoTest, WebViewDelegateImpl, evaluate_javascript, run_api_tests}; +use servo::{JSValue, JavaScriptEvaluationError, Theme, WebViewBuilder}; use url::Url; -#[derive(Default)] -struct WebViewDelegateImpl { - url_changed: Cell, -} - -impl WebViewDelegateImpl { - pub(crate) fn reset(&self) { - self.url_changed.set(false); - } -} - -impl WebViewDelegate for WebViewDelegateImpl { - fn notify_url_changed(&self, _webview: servo::WebView, _url: url::Url) { - self.url_changed.set(true); - } -} - fn test_create_webview(servo_test: &ServoTest) -> Result<(), anyhow::Error> { let delegate = Rc::new(WebViewDelegateImpl::default()); let webview = WebViewBuilder::new(servo_test.servo()) @@ -53,28 +33,6 @@ fn test_create_webview(servo_test: &ServoTest) -> Result<(), anyhow::Error> { Ok(()) } -fn evaluate_javascript( - servo_test: &ServoTest, - webview: WebView, - script: impl ToString, -) -> Result { - let load_webview = webview.clone(); - let _ = servo_test.spin(move || Ok(load_webview.load_status() != LoadStatus::Complete)); - - let saved_result = Rc::new(RefCell::new(None)); - let callback_result = saved_result.clone(); - webview.evaluate_javascript(script, move |result| { - *callback_result.borrow_mut() = Some(result) - }); - - let spin_result = saved_result.clone(); - let _ = servo_test.spin(move || Ok(spin_result.borrow().is_none())); - - (*saved_result.borrow()) - .clone() - .expect("Should have waited until value available") -} - fn test_evaluate_javascript_basic(servo_test: &ServoTest) -> Result<(), anyhow::Error> { let delegate = Rc::new(WebViewDelegateImpl::default()); let webview = WebViewBuilder::new(servo_test.servo()) diff --git a/components/shared/constellation/Cargo.toml b/components/shared/constellation/Cargo.toml index 7a1132ecd63..d3b1d25882b 100644 --- a/components/shared/constellation/Cargo.toml +++ b/components/shared/constellation/Cargo.toml @@ -31,6 +31,7 @@ net_traits = { workspace = true } pixels = { path = "../../pixels" } profile_traits = { workspace = true } serde = { workspace = true } +servo_config = { path = "../../config" } servo_url = { path = "../../url" } strum = { workspace = true } strum_macros = { workspace = true } diff --git a/components/shared/constellation/lib.rs b/components/shared/constellation/lib.rs index 966d6b88897..37ab161fce6 100644 --- a/components/shared/constellation/lib.rs +++ b/components/shared/constellation/lib.rs @@ -28,6 +28,7 @@ use ipc_channel::ipc::IpcSender; use malloc_size_of_derive::MallocSizeOf; use profile_traits::mem::MemoryReportResult; use serde::{Deserialize, Serialize}; +use servo_config::prefs::PrefValue; use servo_url::{ImmutableOrigin, ServoUrl}; pub use structured_data::*; use strum_macros::IntoStaticStr; @@ -102,6 +103,8 @@ pub enum EmbedderToConstellationMessage { SendImageKeysForPipeline(PipelineId, Vec), /// Set WebDriver input event handled sender. SetWebDriverResponseSender(IpcSender), + /// A set of preferences were updated with the given new values. + PreferencesUpdated(Vec<(&'static str, PrefValue)>), } /// A description of a paint metric that is sent from the Servo renderer to the diff --git a/components/shared/script/Cargo.toml b/components/shared/script/Cargo.toml index 69438867cc0..f0b9d9ac675 100644 --- a/components/shared/script/Cargo.toml +++ b/components/shared/script/Cargo.toml @@ -35,6 +35,7 @@ net_traits = { workspace = true } pixels = { path = "../../pixels" } profile_traits = { workspace = true } serde = { workspace = true } +servo_config = { path = "../../config" } servo_url = { path = "../../url" } strum = { workspace = true, features = ["derive"] } strum_macros = { workspace = true } diff --git a/components/shared/script/lib.rs b/components/shared/script/lib.rs index 8ba181aada3..7028d202aaf 100644 --- a/components/shared/script/lib.rs +++ b/components/shared/script/lib.rs @@ -43,6 +43,7 @@ use net_traits::storage_thread::StorageType; use pixels::PixelFormat; use profile_traits::mem; use serde::{Deserialize, Serialize}; +use servo_config::prefs::PrefValue; use servo_url::{ImmutableOrigin, ServoUrl}; use strum_macros::IntoStaticStr; use style_traits::{CSSPixel, SpeculativePainter}; @@ -257,6 +258,8 @@ pub enum ScriptThreadMessage { EvaluateJavaScript(PipelineId, JavaScriptEvaluationId, String), /// A new batch of keys for the image cache for the specific pipeline. SendImageKeysBatch(PipelineId, Vec), + /// Preferences were updated in the parent process. + PreferencesUpdated(Vec<(String, PrefValue)>), } impl fmt::Debug for ScriptThreadMessage { diff --git a/ports/servoshell/desktop/webxr.rs b/ports/servoshell/desktop/webxr.rs index dd68b1c6fc6..f04547deabc 100644 --- a/ports/servoshell/desktop/webxr.rs +++ b/ports/servoshell/desktop/webxr.rs @@ -63,7 +63,7 @@ impl XrDiscoveryWebXrRegistry { struct XrPrefObserver(Arc); impl prefs::Observer for XrPrefObserver { - fn prefs_changed(&self, changes: Vec<(&'static str, prefs::PrefValue)>) { + fn prefs_changed(&self, changes: &[(&'static str, prefs::PrefValue)]) { if let Some((_, value)) = changes.iter().find(|(name, _)| *name == "dom_webxr_test") { let prefs::PrefValue::Bool(value) = value else { return; @@ -79,7 +79,7 @@ impl WebXrRegistry for XrDiscoveryWebXrRegistry { let mock_enabled = Arc::new(AtomicBool::new(pref!(dom_webxr_test))); xr.register_mock(HeadlessMockDiscovery::new(mock_enabled.clone())); - prefs::set_observer(Box::new(XrPrefObserver(mock_enabled))); + prefs::add_observer(Box::new(XrPrefObserver(mock_enabled))); if let Some(xr_discovery) = self.xr_discovery.take() { match xr_discovery {