[runtime-call-stats] Fix a long standing crash in RuntimeCallStats::Leave

There must be a matching Leave for each Enter. Otherwise it ends up
with a dead stack-allocated object in the timer chain.

Drive-by: There was also a bug in
RuntimeCallTimerScope::RuntimeCallTimerScope(HeapObject* ...) did create a
local object instead of calling an overloaded constructor.

BUG=chromium:669329

Change-Id: I9aa1c574a854af8beab3d8097efab3a726ad1c8d
Reviewed-on: https://chromium-review.googlesource.com/634511
Commit-Queue: Alexei Filippov <alph@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#47613}

src/counters-inl.h

@@ -65,20 +65,25 @@ base::TimeTicks RuntimeCallTimer::Now() {
 
 RuntimeCallTimerScope::RuntimeCallTimerScope(
     Isolate* isolate, RuntimeCallStats::CounterId counter_id) {
-  if (V8_UNLIKELY(FLAG_runtime_stats)) {
-    Initialize(isolate->counters()->runtime_call_stats(), counter_id);
-  }
+  if (V8_LIKELY(!FLAG_runtime_stats)) return;
+  stats_ = isolate->counters()->runtime_call_stats();
+  RuntimeCallStats::Enter(stats_, &timer_, counter_id);
 }
 
 RuntimeCallTimerScope::RuntimeCallTimerScope(
-    HeapObject* heap_object, RuntimeCallStats::CounterId counter_id) {
-  RuntimeCallTimerScope(heap_object->GetIsolate(), counter_id);
-}
+    HeapObject* heap_object, RuntimeCallStats::CounterId counter_id)
+    : RuntimeCallTimerScope(heap_object->GetIsolate(), counter_id) {}
 
 RuntimeCallTimerScope::RuntimeCallTimerScope(
     RuntimeCallStats* stats, RuntimeCallStats::CounterId counter_id) {
-  if (V8_UNLIKELY(FLAG_runtime_stats)) {
-    Initialize(stats, counter_id);
+  if (V8_LIKELY(!FLAG_runtime_stats)) return;
+  stats_ = stats;
+  RuntimeCallStats::Enter(stats_, &timer_, counter_id);
+}
+
+RuntimeCallTimerScope::~RuntimeCallTimerScope() {
+  if (V8_UNLIKELY(stats_ != nullptr)) {
+    RuntimeCallStats::Leave(stats_, &timer_);
   }
 }
 

src/counters.cc

@@ -485,26 +485,10 @@ void RuntimeCallStats::Enter(RuntimeCallStats* stats, RuntimeCallTimer* timer,
 
 // static
 void RuntimeCallStats::Leave(RuntimeCallStats* stats, RuntimeCallTimer* timer) {
-  if (stats->current_timer_.Value() == timer) {
-    stats->current_timer_.SetValue(timer->Stop());
-  } else {
-    // Must be a Threading cctest. Walk the chain of Timers to find the
-    // buried one that's leaving. We don't care about keeping nested timings
-    // accurate, just avoid crashing by keeping the chain intact.
-    RuntimeCallTimer* next = stats->current_timer_.Value();
-    while (next && next->parent() != timer) next = next->parent();
-    if (next == nullptr) return;
-    next->set_parent(timer->Stop());
-  }
-
-  {
-    RuntimeCallTimer* cur_timer = stats->current_timer_.Value();
-    if (cur_timer == nullptr) {
-      stats->current_counter_.SetValue(nullptr);
-    } else {
-      stats->current_counter_.SetValue(cur_timer->counter());
-    }
-  }
+  CHECK(stats->current_timer_.Value() == timer);
+  stats->current_timer_.SetValue(timer->Stop());
+  RuntimeCallTimer* cur_timer = stats->current_timer_.Value();
+  stats->current_counter_.SetValue(cur_timer ? cur_timer->counter() : nullptr);
 }
 
 void RuntimeCallStats::Add(RuntimeCallStats* other) {

src/counters.h

@@ -971,20 +971,9 @@ class RuntimeCallTimerScope {
                                RuntimeCallStats::CounterId counter_id);
   inline RuntimeCallTimerScope(RuntimeCallStats* stats,
                                RuntimeCallStats::CounterId counter_id);
-
-  inline ~RuntimeCallTimerScope() {
-    if (V8_UNLIKELY(stats_ != nullptr)) {
-      RuntimeCallStats::Leave(stats_, &timer_);
-    }
-  }
+  inline ~RuntimeCallTimerScope();
 
  private:
-  V8_INLINE void Initialize(RuntimeCallStats* stats,
-                            RuntimeCallStats::CounterId counter_id) {
-    stats_ = stats;
-    RuntimeCallStats::Enter(stats_, &timer_, counter_id);
-  }
-
   RuntimeCallStats* stats_ = nullptr;
   RuntimeCallTimer timer_;
 };

src/interpreter/interpreter.cc

@@ -36,31 +36,32 @@ class InterpreterCompilationJob final : public CompilationJob {
   class TimerScope final {
    public:
     TimerScope(RuntimeCallStats* stats, RuntimeCallStats::CounterId counter_id)
-        : stats_(stats) {
-      if (V8_UNLIKELY(FLAG_runtime_stats)) {
+        : stats_(stats), runtime_stats_enabled_(FLAG_runtime_stats) {
+      if (V8_UNLIKELY(runtime_stats_enabled_)) {
         RuntimeCallStats::Enter(stats_, &timer_, counter_id);
       }
     }
 
-    explicit TimerScope(RuntimeCallCounter* counter) : stats_(nullptr) {
-      if (V8_UNLIKELY(FLAG_runtime_stats)) {
+    explicit TimerScope(RuntimeCallCounter* counter)
+        : stats_(nullptr), runtime_stats_enabled_(FLAG_runtime_stats) {
+      if (V8_UNLIKELY(runtime_stats_enabled_)) {
         timer_.Start(counter, nullptr);
       }
     }
 
     ~TimerScope() {
-      if (V8_UNLIKELY(FLAG_runtime_stats)) {
-        if (stats_) {
-          RuntimeCallStats::Leave(stats_, &timer_);
-        } else {
-          timer_.Stop();
-        }
+      if (V8_LIKELY(!runtime_stats_enabled_)) return;
+      if (stats_) {
+        RuntimeCallStats::Leave(stats_, &timer_);
+      } else {
+        timer_.Stop();
       }
     }
 
    private:
     RuntimeCallStats* stats_;
     RuntimeCallTimer timer_;
+    bool runtime_stats_enabled_;
   };
 
   BytecodeGenerator* generator() { return &generator_; }