From 1eed2acf6a0e078715faa3c61ecfbb1b3b006a27 Mon Sep 17 00:00:00 2001
From: Sosuke Suzuki <sosuke@bun.com>
Date: Wed, 15 Apr 2026 10:27:43 +0900
Subject: [PATCH] ThreadingWin: force Thread::suspend to be synchronous

On multi-core Windows systems, SuspendThread merely requests a suspension
from the kernel -- the target thread may continue to execute user-mode
instructions briefly after SuspendThread returns, until the kernel
reschedules it away. Callers that read registers (MachineStackMarker)
happen to force a sync by calling GetThreadContext right after suspend,
but callers that only want the target stopped -- notably the libpas
scavenger's pasSuspenderBeginSuspend path -- would otherwise race the
mutator: the scavenger decommits TLC pages while the mutator is still
writing to them, violating pas_thread_suspender.h's contract and
causing intermittent heap corruption or access violations.

Call GetThreadContext once with CONTEXT_INTEGER immediately after a
successful SuspendThread. GetThreadContext synchronizes with the
scheduler and does not return until the target is actually stopped.
On failure (rare -- handle invalidation), undo the SuspendThread and
surface the error code so the caller sees a clean Expected failure
instead of a half-suspended thread.

Add a regression test in TestWebKitAPI: a worker busy-increments a
counter; while suspended, the counter must not advance. The test is
cross-platform (Darwin mach_thread_suspend and Linux signal-based
suspend are already synchronous) and would have flagged this bug had
it existed then.
---
 Source/WTF/wtf/win/ThreadingWin.cpp         | 20 +++++++--
 Tools/TestWebKitAPI/Tests/WTF/Threading.cpp | 50 +++++++++++++++++++++
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/Source/WTF/wtf/win/ThreadingWin.cpp b/Source/WTF/wtf/win/ThreadingWin.cpp
index 54f7746e2839..c41f3469c0f7 100644
--- a/Source/WTF/wtf/win/ThreadingWin.cpp
+++ b/Source/WTF/wtf/win/ThreadingWin.cpp
@@ -217,9 +217,23 @@ auto Thread::suspend(const ThreadSuspendLocker&) -> Expected<void, PlatformSuspe
     // the heap lock, and currentSingleton would lazy-allocate a Thread for it.
     RELEASE_ASSERT_WITH_MESSAGE(this != Thread::currentMayBeNull(), "We do not support suspending the current thread itself.");
     DWORD result = SuspendThread(m_handle);
-    if (result != (DWORD)-1)
-        return { };
-    return makeUnexpected(result);
+    if (result == (DWORD)-1)
+        return makeUnexpected(result);
+    // SuspendThread only requests a suspension; on multi-core Windows the target may
+    // continue to execute in user mode briefly after SuspendThread returns. Callers that
+    // read registers (MachineStackMarker) happen to force a sync via GetThreadContext,
+    // but callers that only need "target is stopped" -- notably the libpas scavenger's
+    // pasSuspenderBeginSuspend -- would otherwise decommit TLC pages while the mutator
+    // is still writing to them. Force the suspension to complete synchronously here so
+    // every Thread::suspend caller can rely on the invariant.
+    CONTEXT ctx;
+    ctx.ContextFlags = CONTEXT_INTEGER;
+    if (!GetThreadContext(m_handle, &ctx)) {
+        DWORD error = GetLastError();
+        ResumeThread(m_handle);
+        return makeUnexpected(error);
+    }
+    return { };
 }
 
 // During resume, suspend or resume should not be executed from the other threads.
diff --git a/Tools/TestWebKitAPI/Tests/WTF/Threading.cpp b/Tools/TestWebKitAPI/Tests/WTF/Threading.cpp
index 50f9604bd0c7..fe7bb3fb5d9c 100644
--- a/Tools/TestWebKitAPI/Tests/WTF/Threading.cpp
+++ b/Tools/TestWebKitAPI/Tests/WTF/Threading.cpp
@@ -25,6 +25,7 @@
 
 #include "config.h"
 
+#include <atomic>
 #include <wtf/Threading.h>
 #include <wtf/Vector.h>
 #include <wtf/threads/BinarySemaphore.h>
@@ -121,4 +122,53 @@ TEST(WTF_Thread, ThreadSafetyAnalysisAssertIsCurrentWorks)
         EXPECT_EQ(2u, holder.result);
 }
 
+// Regression test for the libpas scavenger's thread-suspension contract:
+// Thread::suspend must guarantee the target is actually stopped by the time it
+// returns. On Windows this requires an explicit GetThreadContext sync after
+// SuspendThread (which is otherwise asynchronous on multi-core systems). A
+// non-synchronous Thread::suspend would let the libpas scavenger decommit TLC
+// pages while the mutator is still writing to them. The test observes a
+// volatile counter that a busy-looping target thread increments; while
+// suspended, the counter must not advance.
+TEST(WTF_Thread, SuspendIsSynchronous)
+{
+    std::atomic<uint64_t> counter { 0 };
+    std::atomic<bool> stop { false };
+    BinarySemaphore workerStarted;
+
+    RefPtr<Thread> worker = Thread::create("Thread::suspend sync test"_s, [&] {
+        workerStarted.signal();
+        while (!stop.load(std::memory_order_relaxed))
+            counter.fetch_add(1, std::memory_order_relaxed);
+    });
+    ASSERT_TRUE(worker);
+    workerStarted.wait();
+
+    constexpr int iterations = 50;
+    constexpr int observeSpin = 200'000;
+    int offenses = 0;
+
+    for (int i = 0; i < iterations; ++i) {
+        ThreadSuspendLocker locker;
+        auto suspendResult = worker->suspend(locker);
+        ASSERT_TRUE(suspendResult.has_value());
+
+        uint64_t before = counter.load(std::memory_order_relaxed);
+        // Busy-loop to give a non-synchronous suspend time to leak.
+        for (volatile int spin = 0; spin < observeSpin; ++spin) { }
+        uint64_t after = counter.load(std::memory_order_relaxed);
+
+        worker->resume(locker);
+
+        if (before != after)
+            ++offenses;
+    }
+
+    stop.store(true, std::memory_order_relaxed);
+    worker->waitForCompletion();
+
+    EXPECT_EQ(0, offenses) << "Target thread advanced during " << offenses
+        << " of " << iterations << " suspend windows -- Thread::suspend is not synchronous";
+}
+
 } // namespace TestWebKitAPI