[cpu-profiler] Don't sample wrong thread's stack in profiler

76217f5 fixed the profiler so it would only sample a thread if
it had the Isolate lock. Unfortunately, this fix missed a timing
window where a thread might have the Isolate lock but might not
have restored the thread-specific data such as thread_local_top_
for the locked thread yet, so the sampler might end up using data
from a different thread.

This doesn't cause any seg faults or the like because the thread
we *meant* to sample has the Isolate lock so the thread we're
accidentally sampling can't mess with any Isolate data but we can
still get incorrect sample data which can be especially obvious if
the accidentally sampled thread is inside code that would never
run on the thread we meant to sample.

Fortunately, we can tell when all thread-specific data has been
restored to the Isolate because thread_state_ in the
PerIsolateThreadData for a thread is set to a non-null value
until everything has been restored, at which point it gets set
to null. So the fix adds a check after the test for the Isolate
lock to check if thread_state_ is null for the thread we mean to
sample. If so, we know all the data in the Isolate is good to go
for sampling.

Bug: v8:11316
Change-Id: I02d6361d8cbd6ec809ad8fb7ef07f5e9c94c7d1e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2628133
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72112}

src/logging/log.cc

@@ -886,7 +886,7 @@ class Ticker : public sampler::Sampler {
       : sampler::Sampler(reinterpret_cast<v8::Isolate*>(isolate)),
         sampling_thread_(
             std::make_unique<SamplingThread>(this, interval_microseconds)),
-        threadId_(ThreadId::Current()) {}
+        perThreadData_(isolate->FindPerThreadDataForThisThread()) {}
 
   ~Ticker() override {
     if (IsActive()) Stop();
@@ -908,8 +908,9 @@ class Ticker : public sampler::Sampler {
   void SampleStack(const v8::RegisterState& state) override {
     if (!profiler_) return;
     Isolate* isolate = reinterpret_cast<Isolate*>(this->isolate());
-    if (v8::Locker::IsActive() &&
+    if (v8::Locker::IsActive() && (!isolate->thread_manager()->IsLockedByThread(
-        !isolate->thread_manager()->IsLockedByThread(threadId_))
+                                       perThreadData_->thread_id()) ||
+                                   perThreadData_->thread_state() != nullptr))
       return;
     TickSample sample;
     sample.Init(isolate, state, TickSample::kIncludeCEntryFrame, true);
@@ -919,7 +920,7 @@ class Ticker : public sampler::Sampler {
  private:
   Profiler* profiler_ = nullptr;
   std::unique_ptr<SamplingThread> sampling_thread_;
-  ThreadId threadId_;
+  Isolate::PerIsolateThreadData* perThreadData_;
 };
 
 //

src/profiler/cpu-profiler.cc

@@ -32,12 +32,13 @@ class CpuSampler : public sampler::Sampler {
   CpuSampler(Isolate* isolate, SamplingEventsProcessor* processor)
       : sampler::Sampler(reinterpret_cast<v8::Isolate*>(isolate)),
         processor_(processor),
-        threadId_(ThreadId::Current()) {}
+        perThreadData_(isolate->FindPerThreadDataForThisThread()) {}
 
   void SampleStack(const v8::RegisterState& regs) override {
     Isolate* isolate = reinterpret_cast<Isolate*>(this->isolate());
-    if (v8::Locker::IsActive() &&
+    if (v8::Locker::IsActive() && (!isolate->thread_manager()->IsLockedByThread(
-        !isolate->thread_manager()->IsLockedByThread(threadId_)) {
+                                       perThreadData_->thread_id()) ||
+                                   perThreadData_->thread_state() != nullptr)) {
       ProfilerStats::Instance()->AddReason(
           ProfilerStats::Reason::kIsolateNotLocked);
       return;
@@ -62,7 +63,7 @@ class CpuSampler : public sampler::Sampler {
 
  private:
   SamplingEventsProcessor* processor_;
-  ThreadId threadId_;
+  Isolate::PerIsolateThreadData* perThreadData_;
 };
 
 ProfilingScope::ProfilingScope(Isolate* isolate, ProfilerListener* listener)

test/cctest/test-cpu-profiler.cc

@@ -589,6 +589,15 @@ static unsigned TotalHitCount(const v8::CpuProfileNode* node) {
   return hit_count;
 }
 
+static unsigned TotalHitCount(const v8::CpuProfileNode* node,
+                              const std::string& name) {
+  if (name.compare(node->GetFunctionNameStr()) == 0) return TotalHitCount(node);
+  unsigned hit_count = 0;
+  for (int i = 0, count = node->GetChildrenCount(); i < count; ++i)
+    hit_count += TotalHitCount(node->GetChild(i), name);
+  return hit_count;
+}
+
 static const v8::CpuProfileNode* FindChild(v8::Local<v8::Context> context,
                                            const v8::CpuProfileNode* node,
                                            const char* name) {
@@ -3269,22 +3278,31 @@ TEST(MultipleIsolates) {
 // wrong if sampling an unlocked frame. We also prevent optimization to prevent
 // inlining so each function call has its own frame.
 const char* varying_frame_size_script = R"(
-    %NeverOptimizeFunction(maybeYield);
+    %NeverOptimizeFunction(maybeYield0);
+    %NeverOptimizeFunction(maybeYield1);
+    %NeverOptimizeFunction(maybeYield2);
     %NeverOptimizeFunction(bar);
     %NeverOptimizeFunction(foo);
-    function maybeYield(n) {
+    function maybeYield0(n) {
       YieldIsolate(Math.random() > yieldLimit);
     }
-    function bar(a, b, c, d) {
+    function maybeYield1(n) {
-      maybeYield(Math.random());
+      YieldIsolate(Math.random() > yieldLimit);
+    }
+    function maybeYield2(n) {
+      YieldIsolate(Math.random() > yieldLimit);
+    }
+    maybeYield = [maybeYield0 ,maybeYield1, maybeYield2];
+    function bar(threadNumber, a, b, c, d) {
+      maybeYield[threadNumber](Math.random());
       return a.length + b.length + c.length + d.length;
     }
-    function foo(timeLimit, yieldProbability) {
+    function foo(timeLimit, yieldProbability, threadNumber) {
       yieldLimit = 1 - yieldProbability;
       const startTime = Date.now();
       for (let i = 0; i < 1e6; i++) {
-        maybeYield(1);
+        maybeYield[threadNumber](1);
-        bar("Hickory", "Dickory", "Doc", "Mouse");
+        bar(threadNumber, "Hickory", "Dickory", "Doc", "Mouse");
         YieldIsolate(Math.random() > 0.999);
         if ((Date.now() - startTime) > timeLimit) break;
       }
@@ -3293,8 +3311,10 @@ const char* varying_frame_size_script = R"(
 
 class UnlockingThread : public v8::base::Thread {
  public:
-  explicit UnlockingThread(v8::Local<v8::Context> env)
+  explicit UnlockingThread(v8::Local<v8::Context> env, int32_t threadNumber)
-      : Thread(Options("UnlockingThread")), env_(CcTest::isolate(), env) {}
+      : Thread(Options("UnlockingThread")),
+        env_(CcTest::isolate(), env),
+        threadNumber_(threadNumber) {}
 
   void Run() override {
     v8::Isolate* isolate = CcTest::isolate();
@@ -3302,10 +3322,11 @@ class UnlockingThread : public v8::base::Thread {
     v8::Isolate::Scope isolate_scope(isolate);
     v8::HandleScope scope(isolate);
     v8::Local<v8::Context> env = v8::Local<v8::Context>::New(isolate, env_);
-    Profile(env);
+    Profile(env, threadNumber_);
   }
 
-  static void Profile(v8::Local<v8::Context> env) {
+  static void Profile(v8::Local<v8::Context> env, int32_t threadNumber) {
+    CHECK_LT(threadNumber, maxThreads_);
     v8::Isolate* isolate = CcTest::isolate();
     v8::Context::Scope context_scope(env);
     v8::CpuProfiler* profiler = v8::CpuProfiler::New(isolate);
@@ -3315,15 +3336,24 @@ class UnlockingThread : public v8::base::Thread {
     int32_t time_limit = 200;
     double yield_probability = 0.001;
     v8::Local<v8::Value> args[] = {v8::Integer::New(isolate, time_limit),
-                                   v8::Number::New(isolate, yield_probability)};
+                                   v8::Number::New(isolate, yield_probability),
+                                   v8::Integer::New(isolate, threadNumber)};
     v8::Local<v8::Function> function = GetFunction(env, "foo");
     function->Call(env, env->Global(), arraysize(args), args).ToLocalChecked();
-    profiler->StopProfiling(profile_name);
+    const v8::CpuProfile* profile = profiler->StopProfiling(profile_name);
+    const CpuProfileNode* root = profile->GetTopDownRoot();
+    for (int32_t number = 0; number < maxThreads_; number++) {
+      std::string maybeYield = "maybeYield" + std::to_string(number);
+      unsigned hit_count = TotalHitCount(root, maybeYield);
+      if (hit_count) CHECK_EQ(number, threadNumber);
+    }
     profiler->Dispose();
   }
 
  private:
   v8::Persistent<v8::Context> env_;
+  int32_t threadNumber_;
+  static const int32_t maxThreads_ = 3;
 };
 
 // Checking for crashes with multiple thread/single Isolate profiling.
@@ -3343,14 +3373,14 @@ TEST(MultipleThreadsSingleIsolate) {
       });
 
   CompileRun(varying_frame_size_script);
-  UnlockingThread thread1(env);
+  UnlockingThread thread1(env, 1);
-  UnlockingThread thread2(env);
+  UnlockingThread thread2(env, 2);
 
   CHECK(thread1.Start());
   CHECK(thread2.Start());
 
   // For good measure, profile on our own thread
-  UnlockingThread::Profile(env);
+  UnlockingThread::Profile(env, 0);
   {
     v8::Unlocker unlocker(isolate);
     thread1.Join();