From 6c4b2e0ef264bff9952f7130d1c8fcf61488104c Mon Sep 17 00:00:00 2001 From: Shu-yu Guo Date: Wed, 13 Jul 2022 17:10:21 -0700 Subject: [PATCH] [shared-struct] Fix unlocking in JSAtomicsMutex Errors in the callback were not correctly unlocking the mutex, oops. Bug: v8:12547 Change-Id: If44ebc023b8192605c9f29bfd4099a197110f5c4 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3760986 Commit-Queue: Shu-yu Guo Reviewed-by: Adam Klein Cr-Commit-Position: refs/heads/main@{#81708} --- .../builtins-atomics-synchronization.cc | 8 ++-- src/objects/js-atomics-synchronization-inl.h | 16 ++++++++ src/objects/js-atomics-synchronization.h | 39 ++++++++++++++++++- test/mjsunit/shared-memory/mutex.js | 13 +++++++ 4 files changed, 70 insertions(+), 6 deletions(-) diff --git a/src/builtins/builtins-atomics-synchronization.cc b/src/builtins/builtins-atomics-synchronization.cc index 0db6dcbd8d..0ce55b5240 100644 --- a/src/builtins/builtins-atomics-synchronization.cc +++ b/src/builtins/builtins-atomics-synchronization.cc @@ -44,13 +44,11 @@ BUILTIN(AtomicsMutexLock) { Handle result; { - // TODO(syg): Make base::LockGuard work with Handles. - JSAtomicsMutex::Lock(isolate, js_mutex); + JSAtomicsMutex::LockGuard lock_guard(isolate, js_mutex); ASSIGN_RETURN_FAILURE_ON_EXCEPTION( isolate, result, Execution::Call(isolate, run_under_lock, isolate->factory()->undefined_value(), 0, nullptr)); - js_mutex->Unlock(isolate); } return *result; @@ -75,13 +73,13 @@ BUILTIN(AtomicsMutexTryLock) { NewTypeError(MessageTemplate::kNotCallable)); } - if (js_mutex->TryLock()) { + JSAtomicsMutex::TryLockGuard try_lock_guard(isolate, js_mutex); + if (try_lock_guard.locked()) { Handle result; ASSIGN_RETURN_FAILURE_ON_EXCEPTION( isolate, result, Execution::Call(isolate, run_under_lock, isolate->factory()->undefined_value(), 0, nullptr)); - js_mutex->Unlock(isolate); return ReadOnlyRoots(isolate).true_value(); } diff --git a/src/objects/js-atomics-synchronization-inl.h b/src/objects/js-atomics-synchronization-inl.h index dc3efa776f..accfa49cb5 100644 --- a/src/objects/js-atomics-synchronization-inl.h +++ b/src/objects/js-atomics-synchronization-inl.h @@ -23,6 +23,22 @@ TQ_OBJECT_CONSTRUCTORS_IMPL(JSAtomicsMutex) CAST_ACCESSOR(JSAtomicsMutex) +JSAtomicsMutex::LockGuard::LockGuard(Isolate* isolate, + Handle mutex) + : isolate_(isolate), mutex_(mutex) { + JSAtomicsMutex::Lock(isolate, mutex); +} + +JSAtomicsMutex::LockGuard::~LockGuard() { mutex_->Unlock(isolate_); } + +JSAtomicsMutex::TryLockGuard::TryLockGuard(Isolate* isolate, + Handle mutex) + : isolate_(isolate), mutex_(mutex), locked_(mutex->TryLock()) {} + +JSAtomicsMutex::TryLockGuard::~TryLockGuard() { + if (locked_) mutex_->Unlock(isolate_); +} + // static void JSAtomicsMutex::Lock(Isolate* requester, Handle mutex) { DisallowGarbageCollection no_gc; diff --git a/src/objects/js-atomics-synchronization.h b/src/objects/js-atomics-synchronization.h index 69eaf5bdde..89ad44a01b 100644 --- a/src/objects/js-atomics-synchronization.h +++ b/src/objects/js-atomics-synchronization.h @@ -37,12 +37,49 @@ class WaiterQueueNode; // that the lock can only be used with main thread isolates (including // workers) but not with helper threads that have their own LocalHeap. // -// This mutex manages its own queue of waiting threads under contention, i.e. a +// This mutex manages its own queue of waiting threads under contention, i.e. // it implements a futex in userland. The algorithm is inspired by WebKit's // ParkingLot. class JSAtomicsMutex : public TorqueGeneratedJSAtomicsMutex { public: + // A non-copyable wrapper class that provides an RAII-style mechanism for + // owning the JSAtomicsMutex. + // + // The mutex is locked when a LockGuard object is created, and unlocked when + // the LockGuard object is destructed. + class V8_NODISCARD LockGuard final { + public: + inline LockGuard(Isolate* isolate, Handle mutex); + LockGuard(const LockGuard&) = delete; + LockGuard& operator=(const LockGuard&) = delete; + inline ~LockGuard(); + + private: + Isolate* isolate_; + Handle mutex_; + }; + + // A non-copyable wrapper class that provides an RAII-style mechanism for + // attempting to take ownership of a JSAtomicsMutex via its TryLock method. + // + // The mutex is attempted to be locked via TryLock when a TryLockGuard object + // is created. If the mutex was acquired, then it is released when the + // TryLockGuard object is destructed. + class V8_NODISCARD TryLockGuard final { + public: + inline TryLockGuard(Isolate* isolate, Handle mutex); + TryLockGuard(const TryLockGuard&) = delete; + TryLockGuard& operator=(const TryLockGuard&) = delete; + inline ~TryLockGuard(); + bool locked() const { return locked_; } + + private: + Isolate* isolate_; + Handle mutex_; + bool locked_; + }; + DECL_CAST(JSAtomicsMutex) DECL_PRINTER(JSAtomicsMutex) EXPORT_DECL_VERIFIER(JSAtomicsMutex) diff --git a/test/mjsunit/shared-memory/mutex.js b/test/mjsunit/shared-memory/mutex.js index 49bcb3b5bf..f189535b36 100644 --- a/test/mjsunit/shared-memory/mutex.js +++ b/test/mjsunit/shared-memory/mutex.js @@ -31,3 +31,16 @@ Atomics.Mutex.lock(mutex, () => { assertFalse(Atomics.Mutex.tryLock(mutex, () => { throw "unreachable"; })); }); assertEquals(locked_count, 4); + +// Throwing in the callback should unlock the mutex. +assertThrowsEquals(() => { + Atomics.Mutex.lock(mutex, () => { throw 42; }); +}, 42); +Atomics.Mutex.lock(mutex, () => { locked_count++; }); +assertEquals(locked_count, 5); + +assertThrowsEquals(() => { + Atomics.Mutex.tryLock(mutex, () => { throw 42; }); +}, 42); +Atomics.Mutex.tryLock(mutex, () => { locked_count++; }); +assertEquals(locked_count, 6);