[shared-struct] Make publishing of shared objects safe

Currently there is nothing ensuring the internal VM state of shared
objects are in a coherent state and visible to other threads when the
shared object is published.

This CL adds a store-store memory barrier when returning from Factory methods that allocate shared JSObjects that are exposed to user JS code. For primitives, there is an additional store-store memory barrier in the shared value barrier.

Bug: v8:12547
Change-Id: I4833c7ebf02cc352da9b006d2732669d6d043172
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_isolates_rel_ng,v8_linux64_tsan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3819041
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82596}

src/builtins/builtins-atomics-synchronization.cc

@@ -11,7 +11,7 @@ namespace internal {
 BUILTIN(AtomicsMutexConstructor) {
   DCHECK(FLAG_harmony_struct);
   HandleScope scope(isolate);
-  return *JSAtomicsMutex::Create(isolate);
+  return *isolate->factory()->NewJSAtomicsMutex();
 }
 
 BUILTIN(AtomicsMutexLock) {
@@ -91,7 +91,7 @@ BUILTIN(AtomicsMutexTryLock) {
 BUILTIN(AtomicsConditionConstructor) {
   DCHECK(FLAG_harmony_struct);
   HandleScope scope(isolate);
-  return *JSAtomicsCondition::Create(isolate);
+  return *isolate->factory()->NewJSAtomicsCondition();
 }
 
 BUILTIN(AtomicsConditionWait) {

src/builtins/builtins-shared-array.cc

@@ -19,7 +19,6 @@ BUILTIN(SharedArrayConstructor) {
   DCHECK(FLAG_shared_string_table);
 
   HandleScope scope(isolate);
-  auto* factory = isolate->factory();
 
   Handle<Object> length_arg = args.atOrUndefined(isolate, 1);
   Handle<Object> length_number;
@@ -36,13 +35,7 @@ BUILTIN(SharedArrayConstructor) {
         isolate, NewRangeError(MessageTemplate::kSharedArraySizeOutOfRange));
   }
 
-  Handle<FixedArrayBase> storage =
-      factory->NewFixedArray(length, AllocationType::kSharedOld);
-  Handle<JSSharedArray> instance = Handle<JSSharedArray>::cast(
-      factory->NewJSObject(args.target(), AllocationType::kSharedOld));
-  instance->set_elements(*storage);
-
-  return *instance;
+  return *isolate->factory()->NewJSSharedArray(args.target(), length);
 }
 
 }  // namespace internal

src/builtins/builtins-struct.cc

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 #include "src/builtins/builtins-utils-inl.h"
+#include "src/objects/js-struct-inl.h"
 #include "src/objects/property-details.h"
 
 namespace v8 {
@@ -98,22 +99,7 @@ BUILTIN(SharedStructTypeConstructor) {
 
 BUILTIN(SharedStructConstructor) {
   HandleScope scope(isolate);
-  auto* factory = isolate->factory();
-
-  Handle<JSObject> instance =
-      factory->NewJSObject(args.target(), AllocationType::kSharedOld);
-
-  Handle<Map> instance_map(instance->map(), isolate);
-  if (instance_map->HasOutOfObjectProperties()) {
-    int num_oob_fields =
-        instance_map->NumberOfFields(ConcurrencyMode::kSynchronous) -
-        instance_map->GetInObjectProperties();
-    Handle<PropertyArray> property_array =
-        factory->NewPropertyArray(num_oob_fields, AllocationType::kSharedOld);
-    instance->SetProperties(*property_array);
-  }
-
-  return *instance;
+  return *isolate->factory()->NewJSSharedStruct(args.target());
 }
 
 }  // namespace internal

src/heap/factory.cc

@@ -50,10 +50,13 @@
 #include "src/objects/instance-type-inl.h"
 #include "src/objects/js-array-buffer-inl.h"
 #include "src/objects/js-array-inl.h"
+#include "src/objects/js-atomics-synchronization-inl.h"
 #include "src/objects/js-collection-inl.h"
 #include "src/objects/js-generator-inl.h"
 #include "src/objects/js-objects.h"
 #include "src/objects/js-regexp-inl.h"
+#include "src/objects/js-shared-array-inl.h"
+#include "src/objects/js-struct-inl.h"
 #include "src/objects/js-weak-refs-inl.h"
 #include "src/objects/literal-objects-inl.h"
 #include "src/objects/megadom-handler-inl.h"
@@ -3924,6 +3927,55 @@ Handle<JSFunction> Factory::NewFunctionForTesting(Handle<String> name) {
       .Build();
 }
 
+Handle<JSSharedStruct> Factory::NewJSSharedStruct(
+    Handle<JSFunction> constructor) {
+  SharedObjectSafePublishGuard publish_guard;
+  Handle<JSSharedStruct> instance = Handle<JSSharedStruct>::cast(
+      NewJSObject(constructor, AllocationType::kSharedOld));
+
+  Handle<Map> instance_map(instance->map(), isolate());
+  if (instance_map->HasOutOfObjectProperties()) {
+    int num_oob_fields =
+        instance_map->NumberOfFields(ConcurrencyMode::kSynchronous) -
+        instance_map->GetInObjectProperties();
+    Handle<PropertyArray> property_array =
+        NewPropertyArray(num_oob_fields, AllocationType::kSharedOld);
+    instance->SetProperties(*property_array);
+  }
+
+  return instance;
+}
+
+Handle<JSSharedArray> Factory::NewJSSharedArray(Handle<JSFunction> constructor,
+                                                int length) {
+  SharedObjectSafePublishGuard publish_guard;
+  Handle<FixedArrayBase> storage =
+      NewFixedArray(length, AllocationType::kSharedOld);
+  Handle<JSSharedArray> instance = Handle<JSSharedArray>::cast(
+      NewJSObject(constructor, AllocationType::kSharedOld));
+  instance->set_elements(*storage);
+  return instance;
+}
+
+Handle<JSAtomicsMutex> Factory::NewJSAtomicsMutex() {
+  SharedObjectSafePublishGuard publish_guard;
+  Handle<Map> map = isolate()->js_atomics_mutex_map();
+  Handle<JSAtomicsMutex> mutex = Handle<JSAtomicsMutex>::cast(
+      NewJSObjectFromMap(map, AllocationType::kSharedOld));
+  mutex->set_state(JSAtomicsMutex::kUnlocked);
+  mutex->set_owner_thread_id(ThreadId::Invalid().ToInteger());
+  return mutex;
+}
+
+Handle<JSAtomicsCondition> Factory::NewJSAtomicsCondition() {
+  SharedObjectSafePublishGuard publish_guard;
+  Handle<Map> map = isolate()->js_atomics_condition_map();
+  Handle<JSAtomicsCondition> cond = Handle<JSAtomicsCondition>::cast(
+      NewJSObjectFromMap(map, AllocationType::kSharedOld));
+  cond->set_state(JSAtomicsCondition::kEmptyState);
+  return cond;
+}
+
 Factory::JSFunctionBuilder::JSFunctionBuilder(Isolate* isolate,
                                               Handle<SharedFunctionInfo> sfi,
                                               Handle<Context> context)

src/heap/factory.h

@@ -870,6 +870,15 @@ class V8_EXPORT_PRIVATE Factory : public FactoryBase<Factory> {
     return New(map, allocation);
   }
 
+  Handle<JSSharedStruct> NewJSSharedStruct(Handle<JSFunction> constructor);
+
+  Handle<JSSharedArray> NewJSSharedArray(Handle<JSFunction> constructor,
+                                         int length);
+
+  Handle<JSAtomicsMutex> NewJSAtomicsMutex();
+
+  Handle<JSAtomicsCondition> NewJSAtomicsCondition();
+
   // Helper class for creating JSFunction objects.
   class V8_EXPORT_PRIVATE JSFunctionBuilder final {
    public:

src/objects/js-atomics-synchronization.cc

@@ -214,17 +214,6 @@ class V8_NODISCARD WaiterQueueNode final {
 
 using detail::WaiterQueueNode;
 
-// static
-Handle<JSAtomicsMutex> JSAtomicsMutex::Create(Isolate* isolate) {
-  auto* factory = isolate->factory();
-  Handle<Map> map = isolate->js_atomics_mutex_map();
-  Handle<JSAtomicsMutex> mutex = Handle<JSAtomicsMutex>::cast(
-      factory->NewJSObjectFromMap(map, AllocationType::kSharedOld));
-  mutex->set_state(kUnlocked);
-  mutex->set_owner_thread_id(ThreadId::Invalid().ToInteger());
-  return mutex;
-}
-
 bool JSAtomicsMutex::TryLockExplicit(std::atomic<StateT>* state,
                                      StateT& expected) {
   // Try to lock a possibly contended mutex.
@@ -355,16 +344,6 @@ void JSAtomicsMutex::UnlockSlowPath(Isolate* requester,
   old_head->Notify();
 }
 
-// static
-Handle<JSAtomicsCondition> JSAtomicsCondition::Create(Isolate* isolate) {
-  auto* factory = isolate->factory();
-  Handle<Map> map = isolate->js_atomics_condition_map();
-  Handle<JSAtomicsCondition> cond = Handle<JSAtomicsCondition>::cast(
-      factory->NewJSObjectFromMap(map, AllocationType::kSharedOld));
-  cond->set_state(kEmptyState);
-  return cond;
-}
-
 bool JSAtomicsCondition::TryLockWaiterQueueExplicit(std::atomic<StateT>* state,
                                                     StateT& expected) {
   // Try to acquire the queue lock.

src/objects/js-atomics-synchronization.h

@@ -112,8 +112,6 @@ class JSAtomicsMutex
   DECL_PRINTER(JSAtomicsMutex)
   EXPORT_DECL_VERIFIER(JSAtomicsMutex)
 
-  V8_EXPORT_PRIVATE static Handle<JSAtomicsMutex> Create(Isolate* isolate);
-
   // Lock the mutex, blocking if it's currently owned by another thread.
   static inline void Lock(Isolate* requester, Handle<JSAtomicsMutex> mutex);
 
@@ -127,6 +125,7 @@ class JSAtomicsMutex
   TQ_OBJECT_CONSTRUCTORS(JSAtomicsMutex)
 
  private:
+  friend class Factory;
   friend class detail::WaiterQueueNode;
 
   // There are 2 lock bits: whether the lock itself is locked, and whether the
@@ -181,8 +180,6 @@ class JSAtomicsCondition
   DECL_PRINTER(JSAtomicsCondition)
   EXPORT_DECL_VERIFIER(JSAtomicsCondition)
 
-  V8_EXPORT_PRIVATE static Handle<JSAtomicsCondition> Create(Isolate* isolate);
-
   V8_EXPORT_PRIVATE static bool WaitFor(
       Isolate* requester, Handle<JSAtomicsCondition> cv,
       Handle<JSAtomicsMutex> mutex, base::Optional<base::TimeDelta> timeout);
@@ -197,6 +194,7 @@ class JSAtomicsCondition
   TQ_OBJECT_CONSTRUCTORS(JSAtomicsCondition)
 
  private:
+  friend class Factory;
   friend class detail::WaiterQueueNode;
 
   // There is 1 lock bit: whether the waiter queue is locked.

src/objects/objects.cc

@@ -2967,6 +2967,8 @@ MaybeHandle<Object> Object::ShareSlow(Isolate* isolate,
   // Use Object::Share() if value might already be shared.
   DCHECK(!value->IsShared());
 
+  SharedObjectSafePublishGuard publish_guard;
+
   if (value->IsString()) {
     return String::Share(isolate, Handle<String>::cast(value));
   }

src/objects/objects.h

@@ -965,6 +965,40 @@ class BooleanBit : public AllStatic {
   }
 };
 
+// This is an RAII helper class to emit a store-store memory barrier if this
+// when allocating objects in the shared heap.
+//
+// This helper must be used in every Factory method that allocates a shared
+// JSObject visible user JS code. This is also used in Object::ShareSlow when
+// publishing newly shared JS primitives.
+//
+// While there is no default ordering guarantee for shared JS objects
+// (e.g. without the use of Atomics methods or postMessage, data races on
+// fields are observable), the internal VM state of a JS object must be safe
+// for publishing so that other threads do not crash.
+//
+// This barrier does not provide synchronization for publishing JS shared
+// objects. It only ensures the weaker "do not crash the VM" guarantee.
+//
+// In particular, note that memory barriers are invisible to TSAN. When
+// concurrent marking is active, field accesses are performed with relaxed
+// atomics, and TSAN is unable to detect data races in shared JS objects. When
+// concurrent marking is inactive, unordered publishes of shared JS objects in
+// JS code are reported as data race warnings by TSAN.
+class V8_NODISCARD SharedObjectSafePublishGuard final {
+ public:
+  ~SharedObjectSafePublishGuard() {
+    // A release fence is used to prevent store-store reorderings of stores to
+    // VM-internal state of shared objects past any subsequent stores (i.e. the
+    // publish).
+    //
+    // On the loading side, we rely neither the compiler nor the CPU reordering
+    // loads that are dependent on observing the address of the published shared
+    // object, like fields of the shared object.
+    std::atomic_thread_fence(std::memory_order_release);
+  }
+};
+
 }  // namespace internal
 }  // namespace v8
 

test/mjsunit/shared-memory/shared-struct-workers-optimized-code.js

@@ -32,7 +32,6 @@ if (this.Worker) {
   let struct = new MyStruct();
   struct.payload = 0;
   worker.postMessage(struct);
-  // Spin until we observe the worker's write of string_field.
   assertEquals("done", worker.getMessage());
 
   worker.terminate();

test/unittests/js-atomics/js-atomics-synchronization-primitive-unittest.cc

@@ -102,7 +102,7 @@ TEST_F(JSAtomicsMutexTest, Contention) {
 
   Isolate* isolate = client_isolate_wrapper.isolate();
   Handle<JSAtomicsMutex> contended_mutex =
-      JSAtomicsMutex::Create(client_isolate_wrapper.isolate());
+      isolate->factory()->NewJSAtomicsMutex();
   ParkingSemaphore sema_ready(0);
   ParkingSemaphore sema_execute_start(0);
   ParkingSemaphore sema_execute_complete(0);
@@ -191,9 +191,9 @@ TEST_F(JSAtomicsConditionTest, NotifyAll) {
 
   constexpr uint32_t kThreads = 32;
 
-  Handle<JSAtomicsMutex> mutex = JSAtomicsMutex::Create(client_isolate);
+  Handle<JSAtomicsMutex> mutex = client_isolate->factory()->NewJSAtomicsMutex();
   Handle<JSAtomicsCondition> condition =
-      JSAtomicsCondition::Create(client_isolate);
+      client_isolate->factory()->NewJSAtomicsCondition();
 
   uint32_t waiting_threads_count = 0;
   ParkingSemaphore sema_ready(0);