[d8] Fix data race in counter creation, update, and disposal

This fixes data races when lazily creating counters (and populating the
{counter_map_}, and when concurrently adding samples to the counters.
It also ensures that the Wasm engine is stopped (via {V8::Dispose})
before printing and deleting counters, as background threads might still
try to update the counters otherwise.

R=mlippautz@chromium.org
CC=​nikolaos@chromium.org

Bug: v8:12453, chromium:1275117
Change-Id: Ie6beea6cc74eea52143d12f9921597da4a250f2a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3308710
Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78191}

src/d8/d8.cc

@@ -453,7 +453,9 @@ class ExternalOwningOneByteStringResource
   std::unique_ptr<base::OS::MemoryMappedFile> file_;
 };
 
+// static variables:
 CounterMap* Shell::counter_map_;
+base::SharedMutex Shell::counter_mutex_;
 base::OS::MemoryMappedFile* Shell::counters_file_ = nullptr;
 CounterCollection Shell::local_counters_;
 CounterCollection* Shell::counters_ = &local_counters_;
@@ -2585,7 +2587,10 @@ void Shell::QuitOnce(v8::FunctionCallbackInfo<v8::Value>* args) {
                       .FromMaybe(0);
   WaitForRunningWorkers();
   args->GetIsolate()->Exit();
-  OnExit(args->GetIsolate());
+  // As we exit the process anyway, we do not dispose the platform and other
+  // global data. Other isolates might still be running, so disposing here can
+  // cause them to crash.
+  OnExit(args->GetIsolate(), false);
   base::OS::ExitProcess(exit_code);
 }
 
@@ -2738,8 +2743,9 @@ int32_t* Counter::Bind(const char* name, bool is_histogram) {
 }
 
 void Counter::AddSample(int32_t sample) {
-  count_++;
-  sample_total_ += sample;
+  base::Relaxed_AtomicIncrement(reinterpret_cast<base::Atomic32*>(&count_), 1);
+  base::Relaxed_AtomicIncrement(
+      reinterpret_cast<base::Atomic32*>(&sample_total_), sample);
 }
 
 CounterCollection::CounterCollection() {
@@ -2770,19 +2776,32 @@ void Shell::MapCounters(v8::Isolate* isolate, const char* name) {
 }
 
 Counter* Shell::GetCounter(const char* name, bool is_histogram) {
+  Counter* counter = nullptr;
+  {
+    base::SharedMutexGuard<base::kShared> mutex_guard(&counter_mutex_);
     auto map_entry = counter_map_->find(name);
-  Counter* counter =
-      map_entry != counter_map_->end() ? map_entry->second : nullptr;
+    if (map_entry != counter_map_->end()) {
+      counter = map_entry->second;
+    }
+  }
+
+  if (!counter) {
+    base::SharedMutexGuard<base::kExclusive> mutex_guard(&counter_mutex_);
+
+    counter = (*counter_map_)[name];
 
     if (counter == nullptr) {
       counter = counters_->GetNextCounter();
-    if (counter != nullptr) {
+      if (counter == nullptr) {
+        // Too many counters.
+        return nullptr;
+      }
       (*counter_map_)[name] = counter;
       counter->Bind(name, is_histogram);
     }
-  } else {
-    DCHECK(counter->is_histogram() == is_histogram);
   }
+
+  DCHECK_EQ(is_histogram, counter->is_histogram());
   return counter;
 }
 
@@ -3340,13 +3359,19 @@ void Shell::WriteLcovData(v8::Isolate* isolate, const char* file) {
   }
 }
 
-void Shell::OnExit(v8::Isolate* isolate) {
+void Shell::OnExit(v8::Isolate* isolate, bool dispose) {
   isolate->Dispose();
   if (shared_isolate) {
     i::Isolate::Delete(reinterpret_cast<i::Isolate*>(shared_isolate));
   }
 
+  if (dispose) {
+    V8::Dispose();
+    V8::DisposePlatform();
+  }
+
   if (i::FLAG_dump_counters || i::FLAG_dump_counters_nvp) {
+    base::SharedMutexGuard<base::kShared> mutex_guard(&counter_mutex_);
     std::vector<std::pair<std::string, Counter*>> counters(
         counter_map_->begin(), counter_map_->end());
     std::sort(counters.begin(), counters.end());
@@ -5374,10 +5399,7 @@ int Shell::Main(int argc, char* argv[]) {
 #endif  // V8_FUZZILLI
     } while (fuzzilli_reprl);
   }
-  OnExit(isolate);
-
-  V8::Dispose();
-  V8::DisposePlatform();
+  OnExit(isolate, true);
 
   // Delete the platform explicitly here to write the tracing output to the
   // tracing file.

src/d8/d8.h

@@ -478,7 +478,7 @@ class Shell : public i::AllStatic {
   static int RunMain(Isolate* isolate, bool last_run);
   static int Main(int argc, char* argv[]);
   static void Exit(int exit_code);
-  static void OnExit(Isolate* isolate);
+  static void OnExit(Isolate* isolate, bool dispose);
   static void CollectGarbage(Isolate* isolate);
   static bool EmptyMessageQueues(Isolate* isolate);
   static bool CompleteMessageLoop(Isolate* isolate);
@@ -669,6 +669,7 @@ class Shell : public i::AllStatic {
   static Global<Function> stringify_function_;
   static const char* stringify_source_;
   static CounterMap* counter_map_;
+  static base::SharedMutex counter_mutex_;
   // We statically allocate a set of local counters to be used if we
   // don't want to store the stats in a memory-mapped file
   static CounterCollection local_counters_;

test/mjsunit/dump-counters.js

@@ -0,0 +1,15 @@
+// Copyright 2021 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// Flags: --dump-counters
+
+d8.file.execute("test/mjsunit/wasm/wasm-module-builder.js");
+
+(function testWasm() {
+  // Regression test for https://crbug.com/v8/12453.
+  if (typeof WebAssembly === 'undefined') return;  // Skip on jitless.
+  const builder = new WasmModuleBuilder();
+  builder.addFunction('f', kSig_v_v).addBody([]);
+  builder.instantiate();
+})();