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]: https://github.com/servo/servo/blob/3fea90a231a94338d67712398fe3d2ba9d402211/components/script/dom/workerglobalscope.rs#L434

4 files changed, 40 insertions, 13 deletions

diff --git a/components/script/dom/dedicatedworkerglobalscope.rs b/components/script/dom/dedicatedworkerglobalscope.rs
index 6d1792f87c2..acd2942b1a8 100644
--- a/components/script/dom/dedicatedworkerglobalscope.rs
+++ b/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")
     }
diff --git a/components/script/dom/serviceworkerglobalscope.rs b/components/script/dom/serviceworkerglobalscope.rs
index 934b9b88b3a..cbd38c0fc42 100644
--- a/components/script/dom/serviceworkerglobalscope.rs
+++ b/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")
     }
diff --git a/components/script/dom/worker.rs b/components/script/dom/worker.rs
index 569f8188802..fbc0c0cd138 100644
--- a/components/script/dom/worker.rs
+++ b/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
diff --git a/components/script/dom/workerglobalscope.rs b/components/script/dom/workerglobalscope.rs
index 6092b3c4c3e..3aaeca56d3b 100644
--- a/components/script/dom/workerglobalscope.rs
+++ b/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);
+                    }
                 },
             }
         }