DevTools: ensure binding is only exposed into the specified context

... when addBinding is called with contextId. Previously, due to
a subtle type, we exposed bidings added with executionContextId to
all contexts created after the binding was added.

Also, do not persist context-specific bindings to agent state,
as context ids don't make sense across the process.

This also adds a test instrastructure to create additional context in
given context group.

Change-Id: I1b3e96cb65b756424bc7872d200bbbf41e4c30b8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2440982Reviewed-by: Simon Zünd <szuend@chromium.org>
Commit-Queue: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/master@{#70261}

src/inspector/v8-runtime-agent-impl.cc

@@ -665,13 +665,7 @@ void V8RuntimeAgentImpl::terminateExecution(
 
 Response V8RuntimeAgentImpl::addBinding(const String16& name,
                                         Maybe<int> executionContextId) {
-  if (!m_state->getObject(V8RuntimeAgentImplState::bindings)) {
-    m_state->setObject(V8RuntimeAgentImplState::bindings,
-                       protocol::DictionaryValue::create());
-  }
-  protocol::DictionaryValue* bindings =
-      m_state->getObject(V8RuntimeAgentImplState::bindings);
-  if (bindings->booleanProperty(name, false)) return Response::Success();
+  if (m_activeBindings.count(name)) return Response::Success();
   if (executionContextId.isJust()) {
     int contextId = executionContextId.fromJust();
     InspectedContext* context =
@@ -681,10 +675,16 @@ Response V8RuntimeAgentImpl::addBinding(const String16& name,
           "Cannot find execution context with given executionContextId");
     }
     addBinding(context, name);
-    // false means that we should not add this binding later.
-    bindings->setBoolean(name, false);
     return Response::Success();
   }
+  // Only persist non context-specific bindings, as contextIds don't make
+  // any sense when state is restored in a different process.
+  if (!m_state->getObject(V8RuntimeAgentImplState::bindings)) {
+    m_state->setObject(V8RuntimeAgentImplState::bindings,
+                       protocol::DictionaryValue::create());
+  }
+  protocol::DictionaryValue* bindings =
+      m_state->getObject(V8RuntimeAgentImplState::bindings);
   bindings->setBoolean(name, true);
   m_inspector->forEachContext(
       m_session->contextGroupId(),
@@ -730,23 +730,22 @@ void V8RuntimeAgentImpl::addBinding(InspectedContext* context,
           .ToLocal(&functionValue)) {
     v8::Maybe<bool> success = global->Set(localContext, v8Name, functionValue);
     USE(success);
+    m_activeBindings.insert(name);
   }
 }
 
 Response V8RuntimeAgentImpl::removeBinding(const String16& name) {
   protocol::DictionaryValue* bindings =
       m_state->getObject(V8RuntimeAgentImplState::bindings);
-  if (!bindings) return Response::Success();
-  bindings->remove(name);
+  if (bindings) bindings->remove(name);
+  m_activeBindings.erase(name);
   return Response::Success();
 }
 
 void V8RuntimeAgentImpl::bindingCalled(const String16& name,
                                        const String16& payload,
                                        int executionContextId) {
-  protocol::DictionaryValue* bindings =
-      m_state->getObject(V8RuntimeAgentImplState::bindings);
-  if (!bindings || !bindings->get(name)) return;
+  if (!m_activeBindings.count(name)) return;
   m_frontend.bindingCalled(name, payload, executionContextId);
 }
 
@@ -755,10 +754,8 @@ void V8RuntimeAgentImpl::addBindings(InspectedContext* context) {
   protocol::DictionaryValue* bindings =
       m_state->getObject(V8RuntimeAgentImplState::bindings);
   if (!bindings) return;
-  for (size_t i = 0; i < bindings->size(); ++i) {
-    if (!bindings->at(i).second) continue;
+  for (size_t i = 0; i < bindings->size(); ++i)
     addBinding(context, bindings->at(i).first);
-  }
 }
 
 void V8RuntimeAgentImpl::restore() {

src/inspector/v8-runtime-agent-impl.h

@@ -32,14 +32,14 @@
 #define V8_INSPECTOR_V8_RUNTIME_AGENT_IMPL_H_
 
 #include <memory>
+#include <set>
 #include <unordered_map>
 
+#include "include/v8.h"
 #include "src/base/macros.h"
 #include "src/inspector/protocol/Forward.h"
 #include "src/inspector/protocol/Runtime.h"
 
-#include "include/v8.h"
-
 namespace v8_inspector {
 
 class InjectedScript;
@@ -145,6 +145,7 @@ class V8RuntimeAgentImpl : public protocol::Runtime::Backend {
   bool m_enabled;
   std::unordered_map<String16, std::unique_ptr<v8::Global<v8::Script>>>
       m_compiledScripts;
+  std::set<String16> m_activeBindings;
 
   DISALLOW_COPY_AND_ASSIGN(V8RuntimeAgentImpl);
 };

test/inspector/inspector-test.cc

@@ -148,7 +148,7 @@ class FrontendChannelImpl : public v8_inspector::V8Inspector::Channel {
                                            v8::MicrotasksScope::kRunMicrotasks);
       v8::HandleScope handle_scope(data->isolate());
       v8::Local<v8::Context> context =
-          data->GetContext(channel_->context_group_id_);
+          data->GetDefaultContext(channel_->context_group_id_);
       v8::Context::Scope context_scope(context);
       v8::Local<v8::Value> message = ToV8String(data->isolate(), message_);
       v8::MaybeLocal<v8::Value> result;
@@ -252,7 +252,7 @@ class ExecuteStringTask : public TaskRunner::Task {
     v8::MicrotasksScope microtasks_scope(data->isolate(),
                                          v8::MicrotasksScope::kRunMicrotasks);
     v8::HandleScope handle_scope(data->isolate());
-    v8::Local<v8::Context> context = data->GetContext(context_group_id_);
+    v8::Local<v8::Context> context = data->GetDefaultContext(context_group_id_);
     v8::Context::Scope context_scope(context);
     v8::ScriptOrigin origin(
         ToV8String(data->isolate(), name_),
@@ -344,6 +344,9 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
     utils->Set(isolate, "createContextGroup",
                v8::FunctionTemplate::New(isolate,
                                          &UtilsExtension::CreateContextGroup));
+    utils->Set(
+        isolate, "createContext",
+        v8::FunctionTemplate::New(isolate, &UtilsExtension::CreateContext));
     utils->Set(
         isolate, "resetContextGroup",
         v8::FunctionTemplate::New(isolate, &UtilsExtension::ResetContextGroup));
@@ -576,6 +579,21 @@ class UtilsExtension : public IsolateData::SetupGlobalTask {
         v8::Int32::New(args.GetIsolate(), context_group_id));
   }
 
+  static void CreateContext(const v8::FunctionCallbackInfo<v8::Value>& args) {
+    if (args.Length() != 2) {
+      fprintf(stderr, "Internal error: createContext(context, name).");
+      Exit();
+    }
+    int context_group_id = args[0].As<v8::Int32>()->Value();
+    std::vector<uint16_t> name =
+        ToVector(args.GetIsolate(), args[1].As<v8::String>());
+
+    RunSyncTask(backend_runner_, [&context_group_id, name](IsolateData* data) {
+      data->CreateContext(context_group_id,
+                          v8_inspector::StringView(name.data(), name.size()));
+    });
+  }
+
   static void ResetContextGroup(
       const v8::FunctionCallbackInfo<v8::Value>& args) {
     if (args.Length() != 1 || !args[0]->IsInt32()) {
@@ -664,7 +682,7 @@ class SetTimeoutTask : public TaskRunner::Task {
     v8::MicrotasksScope microtasks_scope(data->isolate(),
                                          v8::MicrotasksScope::kRunMicrotasks);
     v8::HandleScope handle_scope(data->isolate());
-    v8::Local<v8::Context> context = data->GetContext(context_group_id_);
+    v8::Local<v8::Context> context = data->GetDefaultContext(context_group_id_);
     v8::Context::Scope context_scope(context);
 
     v8::Local<v8::Function> callback = callback_.Get(data->isolate());
@@ -798,7 +816,8 @@ class InspectorExtension : public IsolateData::SetupGlobalTask {
       const v8::FunctionCallbackInfo<v8::Value>& args) {
     v8::Local<v8::Context> context = args.GetIsolate()->GetCurrentContext();
     IsolateData* data = IsolateData::FromContext(context);
-    data->FireContextCreated(context, data->GetContextGroupId(context));
+    data->FireContextCreated(context, data->GetContextGroupId(context),
+                             v8_inspector::StringView());
   }
 
   static void FireContextDestroyed(

test/inspector/isolate-data.cc

@@ -87,6 +87,13 @@ IsolateData* IsolateData::FromContext(v8::Local<v8::Context> context) {
 }
 
 int IsolateData::CreateContextGroup() {
+  int context_group_id = ++last_context_group_id_;
+  CreateContext(context_group_id, v8_inspector::StringView());
+  return context_group_id;
+}
+
+void IsolateData::CreateContext(int context_group_id,
+                                v8_inspector::StringView name) {
   v8::HandleScope handle_scope(isolate_.get());
   v8::Local<v8::ObjectTemplate> global_template =
       v8::ObjectTemplate::New(isolate_.get());
@@ -97,17 +104,15 @@ int IsolateData::CreateContextGroup() {
   v8::Local<v8::Context> context =
       v8::Context::New(isolate_.get(), nullptr, global_template);
   context->SetAlignedPointerInEmbedderData(kIsolateDataIndex, this);
-  int context_group_id = ++last_context_group_id_;
   // Should be 2-byte aligned.
   context->SetAlignedPointerInEmbedderData(
       kContextGroupIdIndex, reinterpret_cast<void*>(context_group_id * 2));
-  contexts_[context_group_id].Reset(isolate_.get(), context);
-  if (inspector_) FireContextCreated(context, context_group_id);
-  return context_group_id;
+  contexts_[context_group_id].emplace_back(isolate_.get(), context);
+  if (inspector_) FireContextCreated(context, context_group_id, name);
 }
 
-v8::Local<v8::Context> IsolateData::GetContext(int context_group_id) {
-  return contexts_[context_group_id].Get(isolate_.get());
+v8::Local<v8::Context> IsolateData::GetDefaultContext(int context_group_id) {
+  return contexts_[context_group_id].begin()->Get(isolate_.get());
 }
 
 void IsolateData::ResetContextGroup(int context_group_id) {
@@ -338,9 +343,9 @@ void IsolateData::PromiseRejectHandler(v8::PromiseRejectMessage data) {
 }
 
 void IsolateData::FireContextCreated(v8::Local<v8::Context> context,
-                                     int context_group_id) {
-  v8_inspector::V8ContextInfo info(context, context_group_id,
-                                   v8_inspector::StringView());
+                                     int context_group_id,
+                                     v8_inspector::StringView name) {
+  v8_inspector::V8ContextInfo info(context, context_group_id, name);
   info.hasMemoryOnConsole = true;
   v8::SealHandleScope seal_handle_scope(isolate());
   inspector_->contextCreated(info);
@@ -388,7 +393,7 @@ bool IsolateData::isInspectableHeapObject(v8::Local<v8::Object> object) {
 
 v8::Local<v8::Context> IsolateData::ensureDefaultContextInGroup(
     int context_group_id) {
-  return GetContext(context_group_id);
+  return GetDefaultContext(context_group_id);
 }
 
 void IsolateData::SetCurrentTimeMS(double time) {

test/inspector/isolate-data.h

@@ -36,8 +36,9 @@ class IsolateData : public v8_inspector::V8InspectorClient {
 
   // Setting things up.
   int CreateContextGroup();
+  void CreateContext(int context_group_id, v8_inspector::StringView name);
   void ResetContextGroup(int context_group_id);
-  v8::Local<v8::Context> GetContext(int context_group_id);
+  v8::Local<v8::Context> GetDefaultContext(int context_group_id);
   int GetContextGroupId(v8::Local<v8::Context> context);
   void RegisterModule(v8::Local<v8::Context> context,
                       v8::internal::Vector<uint16_t> name,
@@ -76,7 +77,8 @@ class IsolateData : public v8_inspector::V8InspectorClient {
   void SetAdditionalConsoleApi(v8_inspector::StringView api_script);
   void SetMaxAsyncTaskStacksForTest(int limit);
   void DumpAsyncTaskStacksStateForTest();
-  void FireContextCreated(v8::Local<v8::Context> context, int context_group_id);
+  void FireContextCreated(v8::Local<v8::Context> context, int context_group_id,
+                          v8_inspector::StringView name);
   void FireContextDestroyed(v8::Local<v8::Context> context);
   void FreeContext(v8::Local<v8::Context> context);
   void SetResourceNamePrefix(v8::Local<v8::String> prefix);
@@ -137,7 +139,7 @@ class IsolateData : public v8_inspector::V8InspectorClient {
   std::unique_ptr<v8::Isolate, IsolateDeleter> isolate_;
   std::unique_ptr<v8_inspector::V8Inspector> inspector_;
   int last_context_group_id_ = 0;
-  std::map<int, v8::Global<v8::Context>> contexts_;
+  std::map<int, std::vector<v8::Global<v8::Context>>> contexts_;
   std::map<v8::internal::Vector<uint16_t>, v8::Global<v8::Module>,
            VectorCompare>
       modules_;

test/inspector/protocol-test.js

@@ -142,6 +142,10 @@ InspectorTest.ContextGroup = class {
     this.id = utils.createContextGroup();
   }
 
+  createContext(name) {
+    utils.createContext(this.id, name || '');
+  }
+
   schedulePauseOnNextStatement(reason, details) {
     utils.schedulePauseOnNextStatement(this.id, reason, details);
   }

test/inspector/runtime/add-binding-expected.txt

@@ -37,6 +37,15 @@ binding called in session2
 
 Disable agent inside session1..
 Call binding..
+binding called in session1
+{
+    method : Runtime.bindingCalled
+    params : {
+        executionContextId : <executionContextId>
+        name : send
+        payload : payload
+    }
+}
 binding called in session2
 {
     method : Runtime.bindingCalled
@@ -49,9 +58,45 @@ binding called in session2
 
 Disable agent inside session2..
 Call binding..
+binding called in session1
+{
+    method : Runtime.bindingCalled
+    params : {
+        executionContextId : <executionContextId>
+        name : send
+        payload : payload
+    }
+}
+binding called in session2
+{
+    method : Runtime.bindingCalled
+    params : {
+        executionContextId : <executionContextId>
+        name : send
+        payload : payload
+    }
+}
 
 Enable agent inside session1..
 Call binding..
+binding called in session1
+{
+    method : Runtime.bindingCalled
+    params : {
+        executionContextId : <executionContextId>
+        name : send
+        payload : payload
+    }
+}
+binding called in session2
+{
+    method : Runtime.bindingCalled
+    params : {
+        executionContextId : <executionContextId>
+        name : send
+        payload : payload
+    }
+}
 
 Running test: testReconnect
 
@@ -97,3 +142,17 @@ binding called in session1
 }
 Remove binding inside session..
 Call binding..
+
+Running test: testAddBindingToContextById
+Call binding in default context (binding should NOT be exposed)
+Call binding in target context (binding should be exposed)
+binding called in session1
+{
+    method : Runtime.bindingCalled
+    params : {
+        executionContextId : <executionContextId>
+        name : frobnicate
+        payload : message
+    }
+}
+Call binding in newly created context (binding should NOT be exposed)

test/inspector/runtime/add-binding.js

@@ -63,6 +63,28 @@ InspectorTest.runAsyncTestSuite([
     session.Protocol.Runtime.removeBinding({name: 'send'});
     InspectorTest.log('Call binding..');
     await session.Protocol.Runtime.evaluate({expression: `send('payload')`});
+  },
+
+  async function testAddBindingToContextById() {
+    const {contextGroup, sessions: [session]} = setupSessions(1);
+    const contextId1 = (await session.Protocol.Runtime.onceExecutionContextCreated()).params.context.id;
+
+    contextGroup.createContext();
+    const contextId2 = (await session.Protocol.Runtime.onceExecutionContextCreated()).params.context.id;
+
+    await session.Protocol.Runtime.addBinding({name: 'frobnicate', executionContextId: contextId2});
+    const expression = `frobnicate('message')`;
+
+    InspectorTest.log('Call binding in default context (binding should NOT be exposed)');
+    await session.Protocol.Runtime.evaluate({expression});
+
+    InspectorTest.log('Call binding in target context (binding should be exposed)');
+    await session.Protocol.Runtime.evaluate({expression, contextId: contextId2});
+
+    InspectorTest.log('Call binding in newly created context (binding should NOT be exposed)');
+    contextGroup.createContext();
+    const contextId3 = (await session.Protocol.Runtime.onceExecutionContextCreated()).params.context.id;
+    await session.Protocol.Runtime.evaluate({expression, contextId: contextId3});
   }
 ]);