From 726e27938707685d747b1800defffb46d8ae39e0 Mon Sep 17 00:00:00 2001
From: Yuki Shiino <yukishiino@chromium.org>
Date: Fri, 14 Sep 2018 00:50:38 +0900
Subject: [PATCH] Fix Isolate::GetIncumbentContext().

It turned out that the original implementation was broken
from the beginning. This patch fixes the API to return
the correct one.

GetIncumbentContext was implemented at
https://chromium-review.googlesource.com/c/v8/v8/+/536728

Change-Id: Iba29171bac10ed82575a8079396768a9d5af3b13
Bug: chromium:883036
Reviewed-on: https://chromium-review.googlesource.com/1219368
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55874}
---
 src/isolate.cc                         | 13 +++--
 test/unittests/api/isolate-unittest.cc | 69 ++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/src/isolate.cc b/src/isolate.cc
index 970c660efa..f393fc6451 100644
--- a/src/isolate.cc
+++ b/src/isolate.cc
@@ -2213,9 +2213,16 @@ Handle<Context> Isolate::GetIncumbentContext() {
 
   // 1st candidate: most-recently-entered author function's context
   // if it's newer than the last Context::BackupIncumbentScope entry.
-  if (!it.done() &&
-      static_cast<const void*>(it.frame()) >
-          static_cast<const void*>(top_backup_incumbent_scope())) {
+  //
+  // NOTE: This code assumes that the stack grows downward.
+  // This code doesn't work with ASAN because ASAN seems allocating stack
+  // separated for native C++ code and compiled JS code, and the following
+  // comparison doesn't make sense in ASAN.
+  // TODO(yukishiino): Make the implementation of BackupIncumbentScope more
+  // robust.
+  if (!it.done() && (!top_backup_incumbent_scope() ||
+                     it.frame()->sp() < reinterpret_cast<Address>(
+                                            top_backup_incumbent_scope()))) {
     Context* context = Context::cast(it.frame()->context());
     return Handle<Context>(context->native_context(), this);
   }
diff --git a/test/unittests/api/isolate-unittest.cc b/test/unittests/api/isolate-unittest.cc
index 377ad83187..8ddf8a29c8 100644
--- a/test/unittests/api/isolate-unittest.cc
+++ b/test/unittests/api/isolate-unittest.cc
@@ -70,4 +70,73 @@ TEST_F(IsolateTest, MemoryPressureNotificationBackground) {
   v8::platform::PumpMessageLoop(internal::V8::GetCurrentPlatform(), isolate());
 }
 
+using IncumbentContextTest = TestWithIsolate;
+
+// Check that Isolate::GetIncumbentContext() returns the correct one in basic
+// scenarios.
+#if !defined(V8_USE_ADDRESS_SANITIZER)
+TEST_F(IncumbentContextTest, MAYBE_Basic) {
+  auto Str = [&](const char* s) {
+    return String::NewFromUtf8(isolate(), s, NewStringType::kNormal)
+        .ToLocalChecked();
+  };
+  auto Run = [&](Local<Context> context, const char* script) {
+    Context::Scope scope(context);
+    return Script::Compile(context, Str(script))
+        .ToLocalChecked()
+        ->Run(context)
+        .ToLocalChecked();
+  };
+
+  // Set up the test environment; three contexts with getIncumbentGlobal()
+  // function.
+  Local<FunctionTemplate> get_incumbent_global = FunctionTemplate::New(
+      isolate(), [](const FunctionCallbackInfo<Value>& info) {
+        Local<Context> incumbent_context =
+            info.GetIsolate()->GetIncumbentContext();
+        info.GetReturnValue().Set(incumbent_context->Global());
+      });
+  Local<ObjectTemplate> global_template = ObjectTemplate::New(isolate());
+  global_template->Set(Str("getIncumbentGlobal"), get_incumbent_global);
+
+  Local<Context> context_a = Context::New(isolate(), nullptr, global_template);
+  Local<Context> context_b = Context::New(isolate(), nullptr, global_template);
+  Local<Context> context_c = Context::New(isolate(), nullptr, global_template);
+  Local<Object> global_a = context_a->Global();
+  Local<Object> global_b = context_b->Global();
+  Local<Object> global_c = context_c->Global();
+
+  Local<String> security_token = Str("security_token");
+  context_a->SetSecurityToken(security_token);
+  context_b->SetSecurityToken(security_token);
+  context_c->SetSecurityToken(security_token);
+
+  global_a->Set(context_a, Str("b"), global_b).ToChecked();
+  global_b->Set(context_b, Str("c"), global_c).ToChecked();
+
+  // Test scenario 2: A -> B -> C, then the incumbent is C.
+  Run(context_a, "funcA = function() { return b.funcB(); }");
+  Run(context_b, "funcB = function() { return c.getIncumbentGlobal(); }");
+  // Without BackupIncumbentScope.
+  EXPECT_EQ(global_b, Run(context_a, "funcA()"));
+  {
+    // With BackupIncumbentScope.
+    Context::BackupIncumbentScope backup_incumbent(context_a);
+    EXPECT_EQ(global_b, Run(context_a, "funcA()"));
+  }
+
+  // Test scenario 2: A -> B -> C -> C, then the incumbent is C.
+  Run(context_a, "funcA = function() { return b.funcB(); }");
+  Run(context_b, "funcB = function() { return c.funcC(); }");
+  Run(context_c, "funcC = function() { return getIncumbentGlobal(); }");
+  // Without BackupIncumbentScope.
+  EXPECT_EQ(global_c, Run(context_a, "funcA()"));
+  {
+    // With BackupIncumbentScope.
+    Context::BackupIncumbentScope backup_incumbent(context_a);
+    EXPECT_EQ(global_c, Run(context_a, "funcA()"));
+  }
+}
+#endif  // !defined(V8_USE_ADDRESS_SANITIZER)
+
 }  // namespace v8