Worklet can access ScriptThread TLS after it's no longer valid

[jdm]

This can cause panics if a GC occurs after the ScriptThread code has cleaned up the TLS:

servo/components/script/dom/worklet.rs

Lines 159 to 164 in dae4943

	impl Drop for Worklet {
	fn drop(&mut self) {
	let script_thread = ScriptThread::worklet_thread_pool();
	script_thread.exit_worklet(self.worklet_id);
	}
	}

[jdm]

diff --git a/components/script/dom/worklet.rs b/components/script/dom/worklet.rs
index 227d9d97c8..e823db830c 100644
--- a/components/script/dom/worklet.rs
+++ b/components/script/dom/worklet.rs
@@ -78,6 +78,8 @@ pub struct Worklet {
     window: Dom<Window>,
     worklet_id: WorkletId,
     global_type: WorkletGlobalScopeType,
+    #[ignore_malloc_size_of = "rc is hard"]
+    thread_pool: Rc<WorkletThreadPool>,
 }

 impl Worklet {
@@ -87,6 +89,7 @@ impl Worklet {
             window: Dom::from_ref(window),
             worklet_id: WorkletId::new(),
             global_type: global_type,
+            thread_pool: ScriptThread::worklet_thread_pool(),
         }
     }

@@ -136,9 +139,8 @@ impl WorkletMethods for Worklet {
         // Steps 6-12 in parallel.
         let pending_tasks_struct = PendingTasksStruct::new();
         let global = self.window.upcast::<GlobalScope>();
-        let pool = ScriptThread::worklet_thread_pool();

-        pool.fetch_and_invoke_a_worklet_script(
+        self.thread_pool.fetch_and_invoke_a_worklet_script(
             global.pipeline_id(),
             self.worklet_id,
             self.global_type,
@@ -158,8 +160,7 @@ impl WorkletMethods for Worklet {

 impl Drop for Worklet {
     fn drop(&mut self) {
-        let script_thread = ScriptThread::worklet_thread_pool();
-        script_thread.exit_worklet(self.worklet_id);
+        self.thread_pool.exit_worklet(self.worklet_id);
     }
 }

@@ -504,9 +505,13 @@ impl WorkletThread {
                 //       this total ordering on thread roles is what guarantees deadlock-freedom.
                 WorkletData::StartSwapRoles(sender) => {
                     let (our_swapper, their_swapper) = swapper();
-                    sender
+                    if sender
                         .send(WorkletData::FinishSwapRoles(their_swapper))
-                        .unwrap();
+                        .is_err()
+                    {
+                        warn!("Couldn't complete swap; exiting worklet event loop.");
+                        return;
+                    }
                     let _ = our_swapper.swap(&mut self.role);
                 },
                 // To finish swapping roles, perform the atomic swap.

This was an attempt to correct it in #25345, but a lot of css-paint-api tests started timing out for reasons that were not clear to me, so this patch was not merged.

[gterzian]

The only thing I can think of, is that the patch was doing ScriptThread::worklet_thread_pool() in the Worklet constructor, wherease previously it was done only "lazily" when AddModule was called.

It could be worth a shot having the worklet own an Option<Rc<WorkletThreadPool>>, and in AddModule initializing it to something if it hasn't been initialized already, while setting it to none in the constructor.