Auto merge of #27016 - gterzian:fix_closing_window, r=jdm,paulrouget

Ensure clean shutdown of JS threads

<!-- Please describe your changes on the following line: -->

FIX https://github.com/servo/servo/issues/26685
FIX https://github.com/servo/servo/issues/26996
FIX https://github.com/servo/servo/issues/9672
FIX #27027

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

<!-- Either: -->
- [ ] 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. -->

components/script/dom/bindings/trace.rs

@@ -42,7 +42,7 @@ use crate::dom::gpucommandencoder::GPUCommandEncoderState;
 use crate::dom::htmlimageelement::SourceSet;
 use crate::dom::htmlmediaelement::{HTMLMediaElementFetchContext, MediaFrameRenderer};
 use crate::dom::identityhub::Identities;
-use crate::script_runtime::StreamConsumer;
+use crate::script_runtime::{ContextForRequestInterrupt, StreamConsumer};
 use crate::script_thread::IncompleteParserContexts;
 use crate::task::TaskBox;
 use app_units::Au;
@@ -501,6 +501,7 @@ unsafe_no_jsmanaged_fields!(TimelineMarkerType);
 unsafe_no_jsmanaged_fields!(WorkerId);
 unsafe_no_jsmanaged_fields!(BufferQueue, QuirksMode, StrTendril);
 unsafe_no_jsmanaged_fields!(Runtime);
+unsafe_no_jsmanaged_fields!(ContextForRequestInterrupt);
 unsafe_no_jsmanaged_fields!(HeaderMap, Method);
 unsafe_no_jsmanaged_fields!(WindowProxyHandler);
 unsafe_no_jsmanaged_fields!(UntrustedNodeAddress, OpaqueNode);

components/script/dom/dedicatedworkerglobalscope.rs

@@ -30,7 +30,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, CommonScriptMsg, JSContext as SafeJSContext, Runtime, ScriptChan, ScriptPort,
+    new_child_runtime, CommonScriptMsg, ContextForRequestInterrupt, JSContext as SafeJSContext,
+    Runtime, ScriptChan, ScriptPort,
 };
 use crate::task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue};
 use crate::task_source::networking::NetworkingTaskSource;
@@ -256,7 +257,7 @@ impl DedicatedWorkerGlobalScope {
                 worker_url,
                 runtime,
                 from_devtools_receiver,
-                Some(closing),
+                closing,
                 gpu_id_hub,
             ),
             task_queue: TaskQueue::new(receiver, own_sender.clone()),
@@ -324,6 +325,7 @@ impl DedicatedWorkerGlobalScope {
         browsing_context: Option<BrowsingContextId>,
         gpu_id_hub: Arc<Mutex<Identities>>,
         control_receiver: Receiver<DedicatedWorkerControlMsg>,
+        context_sender: Sender<ContextForRequestInterrupt>,
     ) -> JoinHandle<()> {
         let serialized_worker_url = worker_url.to_string();
         let name = format!("WebWorker for {}", serialized_worker_url);
@@ -377,6 +379,8 @@ impl DedicatedWorkerGlobalScope {
                     new_child_runtime(parent, Some(task_source))
                 };
 
+                let _ = context_sender.send(ContextForRequestInterrupt::new(runtime.cx()));
+
                 let (devtools_mpsc_chan, devtools_mpsc_port) = unbounded();
                 ROUTER.route_ipc_receiver_to_crossbeam_sender(
                     from_devtools_receiver,

components/script/dom/globalscope.rs

@@ -52,7 +52,9 @@ use crate::dom::workletglobalscope::WorkletGlobalScope;
 use crate::microtask::{Microtask, MicrotaskQueue, UserMicrotask};
 use crate::realms::{enter_realm, AlreadyInRealm, InRealm};
 use crate::script_module::ModuleTree;
-use crate::script_runtime::{CommonScriptMsg, JSContext as SafeJSContext, ScriptChan, ScriptPort};
+use crate::script_runtime::{
+    CommonScriptMsg, ContextForRequestInterrupt, JSContext as SafeJSContext, ScriptChan, ScriptPort,
+};
 use crate::script_thread::{MainThreadScriptChan, ScriptThread};
 use crate::task::TaskCanceller;
 use crate::task_source::dom_manipulation::DOMManipulationTaskSource;
@@ -130,6 +132,8 @@ pub struct AutoCloseWorker {
     /// A sender of control messages,
     /// currently only used to signal shutdown.
     control_sender: Sender<DedicatedWorkerControlMsg>,
+    /// The context to request an interrupt on the worker thread.
+    context: ContextForRequestInterrupt,
 }
 
 impl Drop for AutoCloseWorker {
@@ -146,6 +150,8 @@ impl Drop for AutoCloseWorker {
             warn!("Couldn't send an exit message to a dedicated worker.");
         }
 
+        self.context.request_interrupt();
+
         // TODO: step 2 and 3.
         // Step 4 is unnecessary since we don't use actual ports for dedicated workers.
         if self
@@ -2049,6 +2055,7 @@ impl GlobalScope {
         closing: Arc<AtomicBool>,
         join_handle: JoinHandle<()>,
         control_sender: Sender<DedicatedWorkerControlMsg>,
+        context: ContextForRequestInterrupt,
     ) {
         self.list_auto_close_worker
             .borrow_mut()
@@ -2056,6 +2063,7 @@ impl GlobalScope {
                 closing,
                 join_handle: Some(join_handle),
                 control_sender: control_sender,
+                context,
             });
     }
 
@@ -2713,6 +2721,20 @@ impl GlobalScope {
         unreachable!();
     }
 
+    /// Returns a boolean indicating whether the event-loop
+    /// where this global is running on can continue running JS.
+    pub fn can_continue_running(&self) -> bool {
+        if self.downcast::<Window>().is_some() {
+            return ScriptThread::can_continue_running();
+        }
+        if let Some(worker) = self.downcast::<WorkerGlobalScope>() {
+            return !worker.is_closing();
+        }
+
+        // TODO: plug worklets into this.
+        true
+    }
+
     /// Returns the task canceller of this global to ensure that everything is
     /// properly cancelled when the global scope is destroyed.
     pub fn task_canceller(&self, name: TaskSourceName) -> TaskCanceller {
@@ -2730,11 +2752,14 @@ impl GlobalScope {
 
     /// Perform a microtask checkpoint.
     pub fn perform_a_microtask_checkpoint(&self) {
-        self.microtask_queue.checkpoint(
-            self.get_cx(),
-            |_| Some(DomRoot::from_ref(self)),
-            vec![DomRoot::from_ref(self)],
-        );
+        // Only perform the checkpoint if we're not shutting down.
+        if self.can_continue_running() {
+            self.microtask_queue.checkpoint(
+                self.get_cx(),
+                |_| Some(DomRoot::from_ref(self)),
+                vec![DomRoot::from_ref(self)],
+            );
+        }
     }
 
     /// Enqueue a microtask for subsequent execution.
@@ -2761,8 +2786,9 @@ impl GlobalScope {
     }
 
     /// Process a single event as if it were the next event
-    /// in the thread queue for this global scope.
-    pub fn process_event(&self, msg: CommonScriptMsg) {
+    /// in the queue for the event-loop where this global scope is running on.
+    /// Returns a boolean indicating whether further events should be processed.
+    pub fn process_event(&self, msg: CommonScriptMsg) -> bool {
         if self.is::<Window>() {
             return ScriptThread::process_event(msg);
         }

components/script/dom/serviceworkerglobalscope.rs

@@ -25,7 +25,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, CommonScriptMsg, JSContext as SafeJSContext, Runtime, ScriptChan,
+    new_rt_and_cx, CommonScriptMsg, ContextForRequestInterrupt, JSContext as SafeJSContext,
+    Runtime, ScriptChan,
 };
 use crate::task_queue::{QueuedTask, QueuedTaskConversion, TaskQueue};
 use crate::task_source::TaskSourceName;
@@ -44,6 +45,7 @@ use script_traits::{ScopeThings, ServiceWorkerMsg, WorkerGlobalScopeInit, Worker
 use servo_config::pref;
 use servo_rand::random;
 use servo_url::ServoUrl;
+use std::sync::atomic::AtomicBool;
 use std::sync::Arc;
 use std::thread::{self, JoinHandle};
 use std::time::{Duration, Instant};
@@ -225,6 +227,7 @@ impl ServiceWorkerGlobalScope {
         swmanager_sender: IpcSender<ServiceWorkerMsg>,
         scope_url: ServoUrl,
         control_receiver: Receiver<ServiceWorkerControlMsg>,
+        closing: Arc<AtomicBool>,
     ) -> ServiceWorkerGlobalScope {
         ServiceWorkerGlobalScope {
             workerglobalscope: WorkerGlobalScope::new_inherited(
@@ -234,7 +237,7 @@ impl ServiceWorkerGlobalScope {
                 worker_url,
                 runtime,
                 from_devtools_receiver,
-                None,
+                closing,
                 Arc::new(Mutex::new(Identities::new())),
             ),
             task_queue: TaskQueue::new(receiver, own_sender.clone()),
@@ -258,6 +261,7 @@ impl ServiceWorkerGlobalScope {
         swmanager_sender: IpcSender<ServiceWorkerMsg>,
         scope_url: ServoUrl,
         control_receiver: Receiver<ServiceWorkerControlMsg>,
+        closing: Arc<AtomicBool>,
     ) -> DomRoot<ServiceWorkerGlobalScope> {
         let cx = runtime.cx();
         let scope = Box::new(ServiceWorkerGlobalScope::new_inherited(
@@ -271,6 +275,7 @@ impl ServiceWorkerGlobalScope {
             swmanager_sender,
             scope_url,
             control_receiver,
+            closing,
         ));
         unsafe { ServiceWorkerGlobalScopeBinding::Wrap(SafeJSContext::from_ptr(cx), scope) }
     }
@@ -285,6 +290,8 @@ impl ServiceWorkerGlobalScope {
         swmanager_sender: IpcSender<ServiceWorkerMsg>,
         scope_url: ServoUrl,
         control_receiver: Receiver<ServiceWorkerControlMsg>,
+        context_sender: Sender<ContextForRequestInterrupt>,
+        closing: Arc<AtomicBool>,
     ) -> JoinHandle<()> {
         let ScopeThings {
             script_url,
@@ -300,6 +307,8 @@ impl ServiceWorkerGlobalScope {
             .spawn(move || {
                 thread_state::initialize(ThreadState::SCRIPT | ThreadState::IN_WORKER);
                 let runtime = new_rt_and_cx(None);
+                let _ = context_sender.send(ContextForRequestInterrupt::new(runtime.cx()));
+
                 let roots = RootCollection::new();
                 let _stack_roots = ThreadLocalStackRoots::new(&roots);
 
@@ -330,6 +339,7 @@ impl ServiceWorkerGlobalScope {
                     swmanager_sender,
                     scope_url,
                     control_receiver,
+                    closing,
                 );
 
                 let referrer = referrer_url

components/script/dom/worker.rs

@@ -125,6 +125,7 @@ impl Worker {
         let init = prepare_workerscope_init(global, Some(devtools_sender), Some(worker_id));
 
         let (control_sender, control_receiver) = unbounded();
+        let (context_sender, context_receiver) = unbounded();
 
         let join_handle = DedicatedWorkerGlobalScope::run_worker_scope(
             init,
@@ -142,9 +143,14 @@ impl Worker {
             browsing_context,
             global.wgpu_id_hub(),
             control_receiver,
+            context_sender,
         );
 
-        global.track_worker(closing, join_handle, control_sender);
+        let context = context_receiver
+            .recv()
+            .expect("Couldn't receive a context for worker.");
+
+        global.track_worker(closing, join_handle, control_sender, context);
 
         Ok(worker)
     }

components/script/dom/workerglobalscope.rs

@@ -98,7 +98,7 @@ pub struct WorkerGlobalScope {
     worker_id: WorkerId,
     worker_url: DomRefCell<ServoUrl>,
     #[ignore_malloc_size_of = "Arc"]
-    closing: Option<Arc<AtomicBool>>,
+    closing: Arc<AtomicBool>,
     #[ignore_malloc_size_of = "Defined in js"]
     runtime: DomRefCell<Option<Runtime>>,
     location: MutNullableDom<WorkerLocation>,
@@ -126,7 +126,7 @@ impl WorkerGlobalScope {
         worker_url: ServoUrl,
         runtime: Runtime,
         from_devtools_receiver: Receiver<DevtoolScriptControlMsg>,
-        closing: Option<Arc<AtomicBool>>,
+        closing: Arc<AtomicBool>,
         gpu_id_hub: Arc<Mutex<Identities>>,
     ) -> Self {
         // Install a pipeline-namespace in the current thread.
@@ -193,11 +193,7 @@ impl WorkerGlobalScope {
     }
 
     pub fn is_closing(&self) -> bool {
-        if let Some(ref closing) = self.closing {
-            closing.load(Ordering::SeqCst)
-        } else {
-            false
-        }
+        self.closing.load(Ordering::SeqCst)
     }
 
     pub fn get_url(&self) -> Ref<ServoUrl> {
@@ -494,7 +490,13 @@ impl WorkerGlobalScope {
         }
     }
 
-    pub fn process_event(&self, msg: CommonScriptMsg) {
+    /// Process a single event as if it were the next event
+    /// in the queue for this worker event-loop.
+    /// Returns a boolean indicating whether further events should be processed.
+    pub fn process_event(&self, msg: CommonScriptMsg) -> bool {
+        if self.is_closing() {
+            return false;
+        }
         match msg {
             CommonScriptMsg::Task(_, task, _, _) => task.run_box(),
             CommonScriptMsg::CollectReports(reports_chan) => {
@@ -504,11 +506,10 @@ impl WorkerGlobalScope {
                 reports_chan.send(reports);
             },
         }
+        true
     }
 
     pub fn close(&self) {
-        if let Some(ref closing) = self.closing {
-            closing.store(true, Ordering::SeqCst);
-        }
+        self.closing.store(true, Ordering::SeqCst);
     }
 }

components/script/dom/xmlhttprequest.rs

@@ -1544,7 +1544,10 @@ impl XMLHttpRequest {
 
         if let Some(script_port) = script_port {
             loop {
-                global.process_event(script_port.recv().unwrap());
+                if !global.process_event(script_port.recv().unwrap()) {
+                    // We're exiting.
+                    return Err(Error::Abort);
+                }
                 let context = context.lock().unwrap();
                 let sync_status = context.sync_status.borrow();
                 if let Some(ref status) = *sync_status {