Create a thread safe version of StatsCounters and use.

src/isolate.cc

 // Copyright 2012 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.
 
 #include "src/isolate.h"
 
 #include <stdlib.h>
 
 #include <fstream>  // NOLINT(readability/streams)
 #include <sstream>
@@ skipping 2531 matching lines @@
   code_aging_helper_ = NULL;
   delete stats_table_;
   stats_table_ = NULL;
 
   delete materialized_object_store_;
   materialized_object_store_ = NULL;
 
   delete logger_;
   logger_ = NULL;
 
-  delete counters_;
   counters_ = NULL;
 
   delete handle_scope_implementer_;
   handle_scope_implementer_ = NULL;
 
   delete code_tracer();
   set_code_tracer(NULL);
 
   delete compilation_cache_;
   compilation_cache_ = NULL;
@@ skipping 65 matching lines @@
     handler->has_terminated_ = false;
     handler->exception_ = pending_exception();
     // Propagate to the external try-catch only if we got an actual message.
     if (thread_local_top_.pending_message_obj_->IsTheHole(this)) return true;
 
     handler->message_obj_ = thread_local_top_.pending_message_obj_;
   }
   return true;
 }
 
+static base::LazyMutex initialize_counters_mutex = LAZY_MUTEX_INITIALIZER;
+
+bool Isolate::InitializeCounters() {
+  if (counters_ != nullptr) return false;

[Clemens Hammacher, 2017/06/02 13:08:49]

This pattern is not safe in general. A thread observing that {counters_ !=
nullptr} is not guaranteed to also observe all other updates performed inside
the locked scope below. This would at least require atomic updates of the
{counters_} field, or explicit memory barries.

Overall, I don't see why we need the mutex here in the first place. Is it
allowed to call this function from a background task?

[kschimpf, 2017/06/02 15:01:02]

I was probably too concerned about this, since the way code was previously
written, it could be called on background threads. However, I agree that we
probably could have presumed this. Now that the StatsTable of an Isolate has
been moved to a field in the Counters class, we can make stronger assumptions
(by not making this method public). Will fix as part of CL
https://codereview.chromium.org/2918703002

[kschimpf, 2017/06/02 17:06:27]

Fixed in cl https://codereview.chromium.org/2919953003

+  base::LockGuard<base::Mutex> guard(initialize_counters_mutex.Pointer());
+  if (counters_ != nullptr) return false;
+  counters_shared_ = std::make_shared<Counters>(this);
+  counters_ = counters_shared_.get();
+  return true;
+}
 
 void Isolate::InitializeLoggingAndCounters() {
   if (logger_ == NULL) {
     logger_ = new Logger(this);
   }
-  if (counters_ == NULL) {
-    counters_ = new Counters(this);
-  }
+  InitializeCounters();
 }
 
 
 bool Isolate::Init(Deserializer* des) {
   TRACE_ISOLATE(init);
 
   stress_deopt_count_ = FLAG_deopt_every_n_times;
 
   has_fatal_error_ = false;
 
@@ skipping 1105 matching lines @@
   // Then check whether this scope intercepts.
   if ((flag & intercept_mask_)) {
     intercepted_flags_ |= flag;
     return true;
   }
   return false;
 }
 
 }  // namespace internal
 }  // namespace v8