Fix crash in SafeStackFrameIterator related to native frames entry/exit

There might be several ExternalCallbackScope's created
during the native callback. Remove the assert that is not
aligned with that.

Moreover this iterator must work for any kind of
stacks including corrupted ones.

BUG=v8:4705
LOG=N

Review URL: https://codereview.chromium.org/1663193003

Cr-Commit-Position: refs/heads/master@{#33751}

src/frames.cc

@@ -235,16 +235,7 @@ SafeStackFrameIterator::SafeStackFrameIterator(
   if (SingletonFor(type) == NULL) return;
   frame_ = SingletonFor(type, &state);
   DCHECK(frame_);
-
   Advance();
-
-  if (frame_ != NULL && !frame_->is_exit() &&
-      external_callback_scope_ != NULL &&
-      external_callback_scope_->scope_address() < frame_->fp()) {
-    // Skip top ExternalCallbackScope if we already advanced to a JS frame
-    // under it. Sampler will anyways take this top external callback.
-    external_callback_scope_ = external_callback_scope_->previous();
-  }
 }
 
 
@@ -329,22 +320,30 @@ bool SafeStackFrameIterator::IsValidExitFrame(Address fp) const {
 void SafeStackFrameIterator::Advance() {
   while (true) {
     AdvanceOneFrame();
-    if (done()) return;
-    if (frame_->is_java_script()) return;
-    if (frame_->is_exit() && external_callback_scope_) {
+    if (done()) break;
+    ExternalCallbackScope* last_callback_scope = NULL;
+    while (external_callback_scope_ != NULL &&
+           external_callback_scope_->scope_address() < frame_->fp()) {
+      // As long as the setup of a frame is not atomic, we may happen to be
+      // in an interval where an ExternalCallbackScope is already created,
+      // but the frame is not yet entered. So we are actually observing
+      // the previous frame.
+      // Skip all the ExternalCallbackScope's that are below the current fp.
+      last_callback_scope = external_callback_scope_;
+      external_callback_scope_ = external_callback_scope_->previous();
+    }
+    if (frame_->is_java_script()) break;
+    if (frame_->is_exit()) {
       // Some of the EXIT frames may have ExternalCallbackScope allocated on
       // top of them. In that case the scope corresponds to the first EXIT
       // frame beneath it. There may be other EXIT frames on top of the
       // ExternalCallbackScope, just skip them as we cannot collect any useful
       // information about them.
-      if (external_callback_scope_->scope_address() < frame_->fp()) {
+      if (last_callback_scope) {
         frame_->state_.pc_address =
-            external_callback_scope_->callback_entrypoint_address();
-        external_callback_scope_ = external_callback_scope_->previous();
-        DCHECK(external_callback_scope_ == NULL ||
-               external_callback_scope_->scope_address() > frame_->fp());
-        return;
+            last_callback_scope->callback_entrypoint_address();
       }
+      break;
     }
   }
 }

test/cctest/cctest.status

@@ -613,19 +613,18 @@
   # TODO(rmcilroy,4680): Test assert errors.
   'test-cpu-profiler/CodeEvents': [FAIL],
   'test-cpu-profiler/TickEvents': [FAIL],
-  'test-cpu-profiler/CallCollectSample': [FAIL],
+  'test-cpu-profiler/CollectSampleAPI': [FAIL],
   'test-cpu-profiler/FunctionDetails': [FAIL],
+  'test-cpu-profiler/JsNativeJsRuntimeJsSampleMultiple': [FAIL],
   'test-cpu-profiler/NativeMethodUninitializedIC': [FAIL],
   'test-cpu-profiler/NativeMethodMonomorphicIC': [FAIL],
   'test-cpu-profiler/NativeAccessorUninitializedIC': [FAIL],
-  'test-cpu-profiler/ NativeAccessorMonomorphicIC': [FAIL],
+  'test-cpu-profiler/NativeAccessorMonomorphicIC': [FAIL],
   'test-sampler-api/StackFramesConsistent': [FAIL],
   'test-profile-generator/LineNumber': [FAIL],
   'test-profile-generator/ProfileNodeScriptId': [FAIL],
   'test-profile-generator/RecordStackTraceAtStartProfiling': [FAIL],
   'test-feedback-vector/VectorCallICStates': [FAIL],
-  'test-cpu-profiler/NativeAccessorMonomorphicIC': [FAIL],
-  'test-cpu-profiler/CollectSampleAPI': [FAIL],
   'test-compiler/FeedbackVectorPreservedAcrossRecompiles': [FAIL],
   'test-api/DynamicWithSourceURLInStackTraceString': [FAIL],
   'test-api/PromiseRejectCallback': [FAIL],

test/cctest/test-cpu-profiler.cc

@@ -426,6 +426,7 @@ static v8::CpuProfile* RunProfiler(v8::Local<v8::Context> env,
   v8::CpuProfiler* cpu_profiler = env->GetIsolate()->GetCpuProfiler();
   v8::Local<v8::String> profile_name = v8_str("my_profile");
 
+  cpu_profiler->SetSamplingInterval(100);
   cpu_profiler->StartProfiling(profile_name, collect_samples);
 
   i::Sampler* sampler =
@@ -537,6 +538,9 @@ static const ProfileNode* GetSimpleBranch(v8::Local<v8::Context> context,
   return reinterpret_cast<const ProfileNode*>(node);
 }
 
+static void CallCollectSample(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  info.GetIsolate()->GetCpuProfiler()->CollectSample();
+}
 
 static const char* cpu_profiler_test_source = "function loop(timeout) {\n"
 "  this.mmm = 0;\n"
@@ -1608,10 +1612,6 @@ static const char* js_force_collect_sample_source =
     "  CallCollectSample();\n"
     "}";
 
-static void CallCollectSample(const v8::FunctionCallbackInfo<v8::Value>& info) {
-  info.GetIsolate()->GetCpuProfiler()->CollectSample();
-}
-
 TEST(CollectSampleAPI) {
   v8::HandleScope scope(CcTest::isolate());
   v8::Local<v8::Context> env = CcTest::NewContext(PROFILER_EXTENSION);
@@ -1637,6 +1637,70 @@ TEST(CollectSampleAPI) {
   profile->Delete();
 }
 
+static const char* js_native_js_runtime_multiple_test_source =
+    "function foo() {\n"
+    "  CallCollectSample();"
+    "  return Math.sin(Math.random());\n"
+    "}\n"
+    "var bound = foo.bind(this);\n"
+    "function bar() {\n"
+    "  try { return bound(); } catch(e) {}\n"
+    "}\n"
+    "function start() {\n"
+    "  try {\n"
+    "    startProfiling('my_profile');\n"
+    "    var startTime = Date.now();\n"
+    "    do {\n"
+    "      CallJsFunction(bar);\n"
+    "    } while (Date.now() - startTime < 200);\n"
+    "  } catch(e) {}\n"
+    "}";
+
+// The test check multiple entrances/exits between JS and native code.
+//
+// [Top down]:
+//    (root) #0 1
+//      start #16 3
+//        CallJsFunction #0 4
+//          bar #16 5
+//            foo #16 6
+//              CallCollectSample
+//      (program) #0 2
+TEST(JsNativeJsRuntimeJsSampleMultiple) {
+  v8::HandleScope scope(CcTest::isolate());
+  v8::Local<v8::Context> env = CcTest::NewContext(PROFILER_EXTENSION);
+  v8::Context::Scope context_scope(env);
+
+  v8::Local<v8::FunctionTemplate> func_template =
+      v8::FunctionTemplate::New(env->GetIsolate(), CallJsFunction);
+  v8::Local<v8::Function> func =
+      func_template->GetFunction(env).ToLocalChecked();
+  func->SetName(v8_str("CallJsFunction"));
+  env->Global()->Set(env, v8_str("CallJsFunction"), func).FromJust();
+
+  func_template =
+      v8::FunctionTemplate::New(env->GetIsolate(), CallCollectSample);
+  func = func_template->GetFunction(env).ToLocalChecked();
+  func->SetName(v8_str("CallCollectSample"));
+  env->Global()->Set(env, v8_str("CallCollectSample"), func).FromJust();
+
+  CompileRun(js_native_js_runtime_multiple_test_source);
+  v8::Local<v8::Function> function = GetFunction(env, "start");
+
+  v8::CpuProfile* profile = RunProfiler(env, function, NULL, 0, 1000);
+
+  const v8::CpuProfileNode* root = profile->GetTopDownRoot();
+  const v8::CpuProfileNode* startNode = GetChild(env, root, "start");
+  const v8::CpuProfileNode* nativeFunctionNode =
+      GetChild(env, startNode, "CallJsFunction");
+
+  const v8::CpuProfileNode* barNode = GetChild(env, nativeFunctionNode, "bar");
+  const v8::CpuProfileNode* fooNode = GetChild(env, barNode, "foo");
+  GetChild(env, fooNode, "CallCollectSample");
+
+  profile->Delete();
+}
+
 // [Top down]:
 //     0   (root) #0 1
 //     2    (program) #0 2