From 7cd73ef4a7634b3e8f716451681b708d1dcc6acb Mon Sep 17 00:00:00 2001 From: Samson <16504129+sagudev@users.noreply.github.com> Date: Thu, 10 Oct 2024 11:43:51 +0200 Subject: [PATCH] use `ThreadSafeJSContext` instead of `ContextForRequestInterrupt` (#33769) * use `ThreadSafeJSContext` instead of `ContextForRequestInterrupt` Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com> * use servo/mozjs Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com> --------- Signed-off-by: sagudev <16504129+sagudev@users.noreply.github.com> --- Cargo.lock | 6 +- components/script/dom/bindings/trace.rs | 3 +- .../script/dom/dedicatedworkerglobalscope.rs | 16 +++--- components/script/dom/globalscope.rs | 10 ++-- .../script/dom/serviceworkerglobalscope.rs | 14 ++--- components/script/dom/worker.rs | 9 +-- components/script/dom/workerglobalscope.rs | 8 +-- components/script/script_runtime.rs | 56 ++++--------------- components/script/script_thread.rs | 10 ++-- components/script/serviceworker_manager.rs | 10 ++-- 10 files changed, 53 insertions(+), 89 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 06120f512e6..04d2b728000 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4519,7 +4519,7 @@ dependencies = [ [[package]] name = "mozjs" version = "0.14.1" -source = "git+https://github.com/servo/mozjs#446ca9765ac3fe582147fd57a51d9a3bcea676ec" +source = "git+https://github.com/servo/mozjs#a02aaf1e11fd275f2f129d0c7ca80a9d07460036" dependencies = [ "bindgen", "cc", @@ -4531,8 +4531,8 @@ dependencies = [ [[package]] name = "mozjs_sys" -version = "0.128.0-12" -source = "git+https://github.com/servo/mozjs#446ca9765ac3fe582147fd57a51d9a3bcea676ec" +version = "0.128.3-0" +source = "git+https://github.com/servo/mozjs#a02aaf1e11fd275f2f129d0c7ca80a9d07460036" dependencies = [ "bindgen", "cc", diff --git a/components/script/dom/bindings/trace.rs b/components/script/dom/bindings/trace.rs index bab3c51fc8a..544aaa229fc 100644 --- a/components/script/dom/bindings/trace.rs +++ b/components/script/dom/bindings/trace.rs @@ -64,7 +64,7 @@ use crate::dom::bindings::str::{DOMString, USVString}; use crate::dom::htmlimageelement::SourceSet; use crate::dom::htmlmediaelement::HTMLMediaElementFetchContext; use crate::dom::windowproxy::WindowProxyHandler; -use crate::script_runtime::{ContextForRequestInterrupt, StreamConsumer}; +use crate::script_runtime::StreamConsumer; use crate::script_thread::IncompleteParserContexts; use crate::task::TaskBox; @@ -359,7 +359,6 @@ where unsafe_no_jsmanaged_fields!(Error); unsafe_no_jsmanaged_fields!(TrustedPromise); -unsafe_no_jsmanaged_fields!(ContextForRequestInterrupt); unsafe_no_jsmanaged_fields!(WindowProxyHandler); unsafe_no_jsmanaged_fields!(DOMString); unsafe_no_jsmanaged_fields!(USVString); diff --git a/components/script/dom/dedicatedworkerglobalscope.rs b/components/script/dom/dedicatedworkerglobalscope.rs index c200e02abf5..6550671a3d3 100644 --- a/components/script/dom/dedicatedworkerglobalscope.rs +++ b/components/script/dom/dedicatedworkerglobalscope.rs @@ -55,8 +55,8 @@ use crate::fetch::load_whole_resource; use crate::realms::{enter_realm, AlreadyInRealm, InRealm}; use crate::script_runtime::ScriptThreadEventCategory::WorkerEvent; use crate::script_runtime::{ - new_child_runtime, CanGc, CommonScriptMsg, ContextForRequestInterrupt, - JSContext as SafeJSContext, Runtime, ScriptChan, ScriptPort, + new_child_runtime, CanGc, CommonScriptMsg, JSContext as SafeJSContext, Runtime, ScriptChan, + ScriptPort, ThreadSafeJSContext, }; use crate::task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue}; use crate::task_source::networking::NetworkingTaskSource; @@ -332,7 +332,7 @@ impl DedicatedWorkerGlobalScope { browsing_context: Option, gpu_id_hub: Arc, control_receiver: Receiver, - context_sender: Sender, + context_sender: Sender, can_gc: CanGc, ) -> JoinHandle<()> { let serialized_worker_url = worker_url.to_string(); @@ -384,8 +384,8 @@ impl DedicatedWorkerGlobalScope { new_child_runtime(parent, Some(task_source)) }; - let context_for_interrupt = ContextForRequestInterrupt::new(runtime.cx()); - let _ = context_sender.send(context_for_interrupt.clone()); + let context_for_interrupt = runtime.thread_safe_js_context(); + let _ = context_sender.send(context_for_interrupt); let (devtools_mpsc_chan, devtools_mpsc_port) = unbounded(); ROUTER.route_ipc_receiver_to_crossbeam_sender( @@ -442,7 +442,7 @@ impl DedicatedWorkerGlobalScope { TaskSourceName::DOMManipulation, )) .unwrap(); - scope.clear_js_runtime(context_for_interrupt); + scope.clear_js_runtime(); return; }, Ok((metadata, bytes)) => (metadata, bytes), @@ -457,7 +457,7 @@ impl DedicatedWorkerGlobalScope { } if scope.is_closing() { - scope.clear_js_runtime(context_for_interrupt); + scope.clear_js_runtime(); return; } @@ -487,7 +487,7 @@ impl DedicatedWorkerGlobalScope { CommonScriptMsg::CollectReports, ); - scope.clear_js_runtime(context_for_interrupt); + scope.clear_js_runtime(); }) .expect("Thread spawning failed") } diff --git a/components/script/dom/globalscope.rs b/components/script/dom/globalscope.rs index 52afc63ba47..2afaa391310 100644 --- a/components/script/dom/globalscope.rs +++ b/components/script/dom/globalscope.rs @@ -120,8 +120,7 @@ use crate::microtask::{Microtask, MicrotaskQueue, UserMicrotask}; use crate::realms::{enter_realm, AlreadyInRealm, InRealm}; use crate::script_module::{DynamicModuleList, ModuleScript, ModuleTree, ScriptFetchOptions}; use crate::script_runtime::{ - CanGc, CommonScriptMsg, ContextForRequestInterrupt, JSContext as SafeJSContext, ScriptChan, - ScriptPort, + CanGc, CommonScriptMsg, JSContext as SafeJSContext, ScriptChan, ScriptPort, ThreadSafeJSContext, }; use crate::script_thread::{MainThreadScriptChan, ScriptThread}; use crate::security_manager::CSPViolationReporter; @@ -151,7 +150,8 @@ pub struct AutoCloseWorker { #[no_trace] control_sender: Sender, /// The context to request an interrupt on the worker thread. - context: ContextForRequestInterrupt, + #[no_trace] + context: ThreadSafeJSContext, } impl Drop for AutoCloseWorker { @@ -168,7 +168,7 @@ impl Drop for AutoCloseWorker { warn!("Couldn't send an exit message to a dedicated worker."); } - self.context.request_interrupt(); + self.context.request_interrupt_callback(); // TODO: step 2 and 3. // Step 4 is unnecessary since we don't use actual ports for dedicated workers. @@ -2115,7 +2115,7 @@ impl GlobalScope { closing: Arc, join_handle: JoinHandle<()>, control_sender: Sender, - context: ContextForRequestInterrupt, + context: ThreadSafeJSContext, ) { self.list_auto_close_worker .borrow_mut() diff --git a/components/script/dom/serviceworkerglobalscope.rs b/components/script/dom/serviceworkerglobalscope.rs index ae39142e0d6..651d383337a 100644 --- a/components/script/dom/serviceworkerglobalscope.rs +++ b/components/script/dom/serviceworkerglobalscope.rs @@ -45,8 +45,8 @@ use crate::dom::workerglobalscope::WorkerGlobalScope; use crate::fetch::load_whole_resource; use crate::realms::{enter_realm, AlreadyInRealm, InRealm}; use crate::script_runtime::{ - new_rt_and_cx, CanGc, CommonScriptMsg, ContextForRequestInterrupt, JSContext as SafeJSContext, - Runtime, ScriptChan, + new_rt_and_cx, CanGc, CommonScriptMsg, JSContext as SafeJSContext, Runtime, ScriptChan, + ThreadSafeJSContext, }; use crate::task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue}; use crate::task_source::TaskSourceName; @@ -293,7 +293,7 @@ impl ServiceWorkerGlobalScope { swmanager_sender: IpcSender, scope_url: ServoUrl, control_receiver: Receiver, - context_sender: Sender, + context_sender: Sender, closing: Arc, can_gc: CanGc, ) -> JoinHandle<()> { @@ -311,8 +311,8 @@ impl ServiceWorkerGlobalScope { .spawn(move || { thread_state::initialize(ThreadState::SCRIPT | ThreadState::IN_WORKER); let runtime = new_rt_and_cx(None); - let context_for_interrupt = ContextForRequestInterrupt::new(runtime.cx()); - let _ = context_sender.send(context_for_interrupt.clone()); + let context_for_interrupt = runtime.thread_safe_js_context(); + let _ = context_sender.send(context_for_interrupt); let roots = RootCollection::new(); let _stack_roots = ThreadLocalStackRoots::new(&roots); @@ -366,7 +366,7 @@ impl ServiceWorkerGlobalScope { match load_whole_resource(request, &resource_threads_sender, global.upcast()) { Err(_) => { println!("error loading script {}", serialized_worker_url); - scope.clear_js_runtime(context_for_interrupt); + scope.clear_js_runtime(); return; }, Ok((metadata, bytes)) => { @@ -407,7 +407,7 @@ impl ServiceWorkerGlobalScope { CommonScriptMsg::CollectReports, ); - scope.clear_js_runtime(context_for_interrupt); + scope.clear_js_runtime(); }) .expect("Thread spawning failed") } diff --git a/components/script/dom/worker.rs b/components/script/dom/worker.rs index f11e9dd0289..37693dffb28 100644 --- a/components/script/dom/worker.rs +++ b/components/script/dom/worker.rs @@ -37,7 +37,7 @@ use crate::dom::messageevent::MessageEvent; use crate::dom::window::Window; use crate::dom::workerglobalscope::prepare_workerscope_init; use crate::realms::enter_realm; -use crate::script_runtime::{CanGc, ContextForRequestInterrupt, JSContext}; +use crate::script_runtime::{CanGc, JSContext, ThreadSafeJSContext}; use crate::task::TaskOnce; pub type TrustedWorkerAddress = Trusted; @@ -55,7 +55,8 @@ pub struct Worker { closing: Arc, terminated: Cell, #[ignore_malloc_size_of = "Arc"] - context_for_interrupt: DomRefCell>, + #[no_trace] + context_for_interrupt: DomRefCell>, } impl Worker { @@ -88,7 +89,7 @@ impl Worker { self.terminated.get() } - pub fn set_context_for_interrupt(&self, cx: ContextForRequestInterrupt) { + pub fn set_context_for_interrupt(&self, cx: ThreadSafeJSContext) { assert!( self.context_for_interrupt.borrow().is_none(), "Context for interrupt must be set only once" @@ -272,7 +273,7 @@ impl WorkerMethods for Worker { // Step 3 if let Some(cx) = self.context_for_interrupt.borrow().as_ref() { - cx.request_interrupt() + cx.request_interrupt_callback() } } diff --git a/components/script/dom/workerglobalscope.rs b/components/script/dom/workerglobalscope.rs index c31fc9d2ab0..79d52a9ff99 100644 --- a/components/script/dom/workerglobalscope.rs +++ b/components/script/dom/workerglobalscope.rs @@ -55,8 +55,7 @@ use crate::dom::workernavigator::WorkerNavigator; use crate::fetch; use crate::realms::{enter_realm, InRealm}; use crate::script_runtime::{ - get_reports, CommonScriptMsg, ContextForRequestInterrupt, JSContext, Runtime, ScriptChan, - ScriptPort, + get_reports, CommonScriptMsg, JSContext, Runtime, ScriptChan, ScriptPort, }; use crate::task::TaskCanceller; use crate::task_source::dom_manipulation::DOMManipulationTaskSource; @@ -177,10 +176,7 @@ impl WorkerGlobalScope { } /// Clear various items when the worker event-loop shuts-down. - pub fn clear_js_runtime(&self, cx_for_interrupt: ContextForRequestInterrupt) { - // Ensure parent thread can no longer request interrupt - // using our JSContext that will soon be destroyed - cx_for_interrupt.revoke(); + pub fn clear_js_runtime(&self) { self.upcast::() .remove_web_messaging_and_dedicated_workers_infra(); diff --git a/components/script/script_runtime.rs b/components/script/script_runtime.rs index 90379c73da6..aeee9c1553b 100644 --- a/components/script/script_runtime.rs +++ b/components/script/script_runtime.rs @@ -14,7 +14,7 @@ use std::io::{stdout, Write}; use std::ops::Deref; use std::os::raw::c_void; use std::rc::Rc; -use std::sync::{Arc, Mutex}; +use std::sync::Mutex; use std::time::{Duration, Instant}; use std::{fmt, os, ptr, thread}; @@ -33,16 +33,16 @@ use js::jsapi::{ InitConsumeStreamCallback, InitDispatchToEventLoop, JSContext as RawJSContext, JSGCParamKey, JSGCStatus, JSJitCompilerOption, JSObject, JSSecurityCallbacks, JSTracer, JS_AddExtraGCRootsTracer, JS_InitDestroyPrincipalsCallback, JS_InitReadPrincipalsCallback, - JS_RequestInterruptCallback, JS_SetGCCallback, JS_SetGCParameter, - JS_SetGlobalJitCompilerOption, JS_SetOffthreadIonCompilationEnabled, - JS_SetParallelParsingEnabled, JS_SetSecurityCallbacks, JobQueue, MimeType, - PromiseRejectionHandlingState, PromiseUserInputEventHandlingState, RuntimeCode, - SetDOMCallbacks, SetGCSliceCallback, SetJobQueue, SetPreserveWrapperCallbacks, + JS_SetGCCallback, JS_SetGCParameter, JS_SetGlobalJitCompilerOption, + JS_SetOffthreadIonCompilationEnabled, JS_SetParallelParsingEnabled, JS_SetSecurityCallbacks, + JobQueue, MimeType, PromiseRejectionHandlingState, PromiseUserInputEventHandlingState, + RuntimeCode, SetDOMCallbacks, SetGCSliceCallback, SetJobQueue, SetPreserveWrapperCallbacks, SetProcessBuildIdOp, SetPromiseRejectionTrackerCallback, StreamConsumer as JSStreamConsumer, }; use js::jsval::UndefinedValue; use js::panic::wrap_panic; use js::rust::wrappers::{GetPromiseIsHandled, JS_GetPromiseResult}; +pub use js::rust::ThreadSafeJSContext; use js::rust::{ describe_scripted_caller, Handle, HandleObject as RustHandleObject, IntoHandle, JSEngine, JSEngineHandle, ParentRuntime, Runtime as RustRuntime, @@ -447,6 +447,12 @@ pub struct Runtime { networking_task_src: Option>, } +impl Runtime { + pub(crate) fn thread_safe_js_context(&self) -> ThreadSafeJSContext { + self.rt.thread_safe_js_context() + } +} + impl Drop for Runtime { #[allow(unsafe_code)] fn drop(&mut self) { @@ -911,44 +917,6 @@ unsafe fn set_gc_zeal_options(cx: *mut RawJSContext) { #[cfg(not(feature = "debugmozjs"))] unsafe fn set_gc_zeal_options(_: *mut RawJSContext) {} -/// A wrapper around a JSContext that is Send, -/// enabling an interrupt to be requested -/// from a thread other than the one running JS using that context. -#[derive(Clone)] -pub struct ContextForRequestInterrupt(Arc>>); - -impl ContextForRequestInterrupt { - pub fn new(context: *mut RawJSContext) -> ContextForRequestInterrupt { - ContextForRequestInterrupt(Arc::new(Mutex::new(Some(context)))) - } - - pub fn revoke(&self) { - self.0.lock().unwrap().take(); - } - - #[allow(unsafe_code)] - /// Can be called from any thread, to request the callback set by - /// JS_AddInterruptCallback to be called on the thread - /// where that context is running. - /// The lock is held when calling JS_RequestInterruptCallback - /// because it is possible for the JSContext to be destroyed - /// on the other thread in the case of Worker shutdown - pub fn request_interrupt(&self) { - let maybe_cx = self.0.lock().unwrap(); - if let Some(cx) = *maybe_cx { - unsafe { - JS_RequestInterruptCallback(cx); - } - } - } -} - -#[allow(unsafe_code)] -/// It is safe to call `JS_RequestInterruptCallback(cx)` from any thread. -/// See the docs for the corresponding `requestInterrupt` method, -/// at `mozjs/js/src/vm/JSContext.h`. -unsafe impl Send for ContextForRequestInterrupt {} - #[derive(Clone, Copy)] #[repr(transparent)] pub struct JSContext(*mut RawJSContext); diff --git a/components/script/script_thread.rs b/components/script/script_thread.rs index daea4ce4cfd..ce9cf31a8d6 100644 --- a/components/script/script_thread.rs +++ b/components/script/script_thread.rs @@ -145,8 +145,8 @@ use crate::microtask::{Microtask, MicrotaskQueue}; use crate::realms::enter_realm; use crate::script_module::ScriptFetchOptions; use crate::script_runtime::{ - get_reports, new_rt_and_cx, CanGc, CommonScriptMsg, ContextForRequestInterrupt, JSContext, - Runtime, ScriptChan, ScriptPort, ScriptThreadEventCategory, + get_reports, new_rt_and_cx, CanGc, CommonScriptMsg, JSContext, Runtime, ScriptChan, ScriptPort, + ScriptThreadEventCategory, ThreadSafeJSContext, }; use crate::task_manager::TaskManager; use crate::task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue}; @@ -734,13 +734,13 @@ pub struct ScriptThread { struct BHMExitSignal { closing: Arc, - js_context: ContextForRequestInterrupt, + js_context: ThreadSafeJSContext, } impl BackgroundHangMonitorExitSignal for BHMExitSignal { fn signal_to_exit(&self) { self.closing.store(true, Ordering::SeqCst); - self.js_context.request_interrupt(); + self.js_context.request_interrupt_callback(); } } @@ -1314,7 +1314,7 @@ impl ScriptThread { let closing = Arc::new(AtomicBool::new(false)); let background_hang_monitor_exit_signal = BHMExitSignal { closing: closing.clone(), - js_context: ContextForRequestInterrupt::new(cx), + js_context: runtime.thread_safe_js_context(), }; let background_hang_monitor = state.background_hang_monitor_register.register_component( diff --git a/components/script/serviceworker_manager.rs b/components/script/serviceworker_manager.rs index b91fdcf65e3..034da394310 100644 --- a/components/script/serviceworker_manager.rs +++ b/components/script/serviceworker_manager.rs @@ -29,7 +29,7 @@ use crate::dom::serviceworkerglobalscope::{ ServiceWorkerControlMsg, ServiceWorkerGlobalScope, ServiceWorkerScriptMsg, }; use crate::dom::serviceworkerregistration::longest_prefix_match; -use crate::script_runtime::{CanGc, ContextForRequestInterrupt}; +use crate::script_runtime::{CanGc, ThreadSafeJSContext}; enum Message { FromResource(CustomResponseMediator), @@ -103,7 +103,7 @@ impl Drop for ServiceWorkerRegistration { self.context .take() .expect("No context to request interrupt.") - .request_interrupt(); + .request_interrupt_callback(); // TODO: Step 1, 2 and 3. if self @@ -134,7 +134,7 @@ struct ServiceWorkerRegistration { /// A handle to join on the worker thread. join_handle: Option>, /// A context to request an interrupt. - context: Option, + context: Option, /// The closing flag for the worker. closing: Option>, } @@ -157,7 +157,7 @@ impl ServiceWorkerRegistration { &mut self, join_handle: JoinHandle<()>, control_sender: Sender, - context: ContextForRequestInterrupt, + context: ThreadSafeJSContext, closing: Arc, ) { assert!(self.join_handle.is_none()); @@ -454,7 +454,7 @@ fn update_serviceworker( ServiceWorker, JoinHandle<()>, Sender, - ContextForRequestInterrupt, + ThreadSafeJSContext, Arc, ) { let (sender, receiver) = unbounded();