Fix race condition and other issues related to Worker destruction. (#30066)

* Fix race condition in Worker destruction

During shutdown, the main script thread calls
JS_RequestInterruptCallback(cx) for each worker thread it owns
where cx is the JSContext* created for that worker.
Although JS_RequestInterruptCallback is safe to call
from threads other than the worker thread, it is possible
the JSContext* has already been destroyed

For example, as noted in #30022, since the main thread sets
the worker's `closing` flag to true to signal termination before it
calls JS_RequestInterruptCallback, we can have a race condition
where the worker exits its event loop when `closing` flags is set
and then it (worker thread) destroys its own JSContext and JSRuntime.
When the main thread resumes, it will call
JS_RequestInterruptCallback for the worker's context, leading to
a use-after-free bug.

This patch solves this issue by improving the existing
`ContextForRequestInterrupt` abstraction used for sharing the Worker's
associated JSContext* with the parent script thread.
Instead of simply wrapping a plain `*mut JSContext`, we now wrap the
`*mut JSContext` in a nullable mutex i.e Mutex<Option<*mut JSContext>>

The mutex lock needs to be held by the parent thread when it
calls JS_RequestInterruptCallback.
Similary, before the worker destroys its JSContext, it locks and
sets the Option to None, signaling that the JSContext can no longer
be used for interrupting the worker.

This patch also fixes the issue in #30052 by enforcing the use
of ContextForRequestInterrupt abstraction which ensures the correct
JSContext is used by the main thread when Worker.terminate is called.

Fixes #30022, #30052

* Fix Worker.importScripts to handle termination

Fixing #30052 uncovered this issue in the implementation
of `importScripts` method. After the fix for #30052,
the WPT test `/workers/Worker-terminate-forever-during-evaluation.html`
started to crash because when evaluation doesn't succeed
`importScripts` always returns Error::JSFailed code to the caller,
which indicates that there is a Dom/JS exception to be thrown. However,
this is not true when the script is terminated, which causes
the generated binding layer for 'importScript` to fail
the assertion that there is a pending exception.

This patch makes `importScripts` work similar to the [logic that
evaluates the top-level script][1] of the Worker - it simply prints
`evaluate_script failed - (terminated)' if the worker is terminating

[1]: 3fea90a231/components/script/dom/workerglobalscope.rs (L434)

components/script/dom/dedicatedworkerglobalscope.rs

@@ -378,7 +378,8 @@ impl DedicatedWorkerGlobalScope {
                     new_child_runtime(parent, Some(task_source))
                 };
 
-                let _ = context_sender.send(ContextForRequestInterrupt::new(runtime.cx()));
+                let context_for_interrupt = ContextForRequestInterrupt::new(runtime.cx());
+                let _ = context_sender.send(context_for_interrupt.clone());
 
                 let (devtools_mpsc_chan, devtools_mpsc_port) = unbounded();
                 ROUTER.route_ipc_receiver_to_crossbeam_sender(
@@ -476,7 +477,8 @@ impl DedicatedWorkerGlobalScope {
                         parent_sender,
                         CommonScriptMsg::CollectReports,
                     );
-                scope.clear_js_runtime();
+
+                scope.clear_js_runtime(context_for_interrupt);
             })
             .expect("Thread spawning failed")
     }

components/script/dom/serviceworkerglobalscope.rs

@@ -306,7 +306,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 context_for_interrupt = ContextForRequestInterrupt::new(runtime.cx());
+                let _ = context_sender.send(context_for_interrupt.clone());
 
                 let roots = RootCollection::new();
                 let _stack_roots = ThreadLocalStackRoots::new(&roots);
@@ -396,7 +397,8 @@ impl ServiceWorkerGlobalScope {
                         scope.script_chan(),
                         CommonScriptMsg::CollectReports,
                     );
-                scope.clear_js_runtime();
+
+                scope.clear_js_runtime(context_for_interrupt);
             })
             .expect("Thread spawning failed")
     }

components/script/dom/worker.rs

@@ -4,6 +4,7 @@
 
 use crate::dom::abstractworker::SimpleWorkerErrorHandler;
 use crate::dom::abstractworker::WorkerScriptMsg;
+use crate::dom::bindings::cell::DomRefCell;
 use crate::dom::bindings::codegen::Bindings::MessagePortBinding::PostMessageOptions;
 use crate::dom::bindings::codegen::Bindings::WorkerBinding::{WorkerMethods, WorkerOptions};
 use crate::dom::bindings::error::{Error, ErrorResult, Fallible};
@@ -23,13 +24,13 @@ 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::JSContext;
+use crate::script_runtime::{ContextForRequestInterrupt, JSContext};
 use crate::task::TaskOnce;
 use crossbeam_channel::{unbounded, Sender};
 use devtools_traits::{DevtoolsPageInfo, ScriptToDevtoolsControlMsg, WorkerId};
 use dom_struct::dom_struct;
 use ipc_channel::ipc;
-use js::jsapi::{Heap, JSObject, JS_RequestInterruptCallback};
+use js::jsapi::{Heap, JSObject};
 use js::jsval::UndefinedValue;
 use js::rust::{CustomAutoRooter, CustomAutoRooterGuard, HandleObject, HandleValue};
 use script_traits::{StructuredSerializedData, WorkerScriptLoadOrigin};
@@ -51,6 +52,8 @@ pub struct Worker {
     #[ignore_malloc_size_of = "Arc"]
     closing: Arc<AtomicBool>,
     terminated: Cell<bool>,
+    #[ignore_malloc_size_of = "Arc"]
+    context_for_interrupt: DomRefCell<Option<ContextForRequestInterrupt>>,
 }
 
 impl Worker {
@@ -60,6 +63,7 @@ impl Worker {
             sender: sender,
             closing: closing,
             terminated: Cell::new(false),
+            context_for_interrupt: Default::default(),
         }
     }
 
@@ -156,6 +160,7 @@ impl Worker {
             .recv()
             .expect("Couldn't receive a context for worker.");
 
+        worker.set_context_for_interrupt(context.clone());
         global.track_worker(closing, join_handle, control_sender, context);
 
         Ok(worker)
@@ -165,6 +170,14 @@ impl Worker {
         self.terminated.get()
     }
 
+    pub fn set_context_for_interrupt(&self, cx: ContextForRequestInterrupt) {
+        assert!(
+            self.context_for_interrupt.borrow().is_none(),
+            "Context for interrupt must be set only once"
+        );
+        *self.context_for_interrupt.borrow_mut() = Some(cx);
+    }
+
     pub fn handle_message(address: TrustedWorkerAddress, data: StructuredSerializedData) {
         let worker = address.root();
 
@@ -241,7 +254,6 @@ impl WorkerMethods for Worker {
         self.post_message_impl(cx, message, guard)
     }
 
-    #[allow(unsafe_code)]
     // https://html.spec.whatwg.org/multipage/#terminate-a-worker
     fn Terminate(&self) {
         // Step 1
@@ -253,8 +265,10 @@ impl WorkerMethods for Worker {
         self.terminated.set(true);
 
         // Step 3
-        let cx = GlobalScope::get_cx();
-        unsafe { JS_RequestInterruptCallback(*cx) };
+        self.context_for_interrupt
+            .borrow()
+            .as_ref()
+            .map(|cx| cx.request_interrupt());
     }
 
     // https://html.spec.whatwg.org/multipage/#handler-worker-onmessage

components/script/dom/workerglobalscope.rs

@@ -30,8 +30,8 @@ use crate::dom::workerlocation::WorkerLocation;
 use crate::dom::workernavigator::WorkerNavigator;
 use crate::fetch;
 use crate::realms::{enter_realm, InRealm};
-use crate::script_runtime::JSContext;
 use crate::script_runtime::{get_reports, CommonScriptMsg, Runtime, ScriptChan, ScriptPort};
+use crate::script_runtime::{ContextForRequestInterrupt, JSContext};
 use crate::task::TaskCanceller;
 use crate::task_source::dom_manipulation::DOMManipulationTaskSource;
 use crate::task_source::file_reading::FileReadingTaskSource;
@@ -166,7 +166,10 @@ impl WorkerGlobalScope {
     }
 
     /// Clear various items when the worker event-loop shuts-down.
-    pub fn clear_js_runtime(&self) {
+    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();
         self.upcast::<GlobalScope>()
             .remove_web_messaging_and_dedicated_workers_infra();
 
@@ -283,8 +286,14 @@ impl WorkerGlobalScopeMethods for WorkerGlobalScope {
             match result {
                 Ok(_) => (),
                 Err(_) => {
-                    println!("evaluate_script failed");
-                    return Err(Error::JSFailed);
+                    if self.is_closing() {
+                        // Don't return JSFailed as we might not have
+                        // any pending exceptions.
+                        println!("evaluate_script failed (terminated)");
+                    } else {
+                        println!("evaluate_script failed");
+                        return Err(Error::JSFailed);
+                    }
                 },
             }
         }

components/script/script_runtime.rs

@@ -87,7 +87,7 @@ use std::os;
 use std::os::raw::c_void;
 use std::ptr;
 use std::rc::Rc;
-use std::sync::Mutex;
+use std::sync::{Arc, Mutex};
 use std::thread;
 use std::time::Duration;
 use style::thread_state::{self, ThreadState};
@@ -858,24 +858,34 @@ unsafe fn set_gc_zeal_options(cx: *mut RawJSContext) {
 #[cfg(not(feature = "debugmozjs"))]
 unsafe fn set_gc_zeal_options(_: *mut RawJSContext) {}
 
-#[repr(transparent)]
 /// 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.
-pub struct ContextForRequestInterrupt(*mut RawJSContext);
+#[derive(Clone)]
+pub struct ContextForRequestInterrupt(Arc<Mutex<Option<*mut RawJSContext>>>);
 
 impl ContextForRequestInterrupt {
     pub fn new(context: *mut RawJSContext) -> ContextForRequestInterrupt {
-        ContextForRequestInterrupt(context)
+        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.
+    /// 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) {
-        unsafe {
-            JS_RequestInterruptCallback(self.0);
+        let maybe_cx = self.0.lock().unwrap();
+        if let Some(cx) = *maybe_cx {
+            unsafe {
+                JS_RequestInterruptCallback(cx);
+            }
         }
     }
 }