[Extensions Bindings] Add a thread-friendly ScriptContext getter

Add a common ScriptContext getter to retrieve a ScriptContext from a
given v8::Context that works across threads. This previously existed,
but only in native_extension_bindings_system.cc. Pull it out to a common
file to a) make it reusable from other consumers (custom hooks will need
to use this) and b) remove (direct) threading knowledge from the bindings
system.

Bug: 653596

Change-Id: I09a0e4ea76bb891ab1871ca1deb0d315e6e9789d
Reviewed-on: https://chromium-review.googlesource.com/812173Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#522243}

chrome/renderer/extensions/extension_hooks_delegate.cc

@@ -10,11 +10,11 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
 #include "extensions/renderer/bindings/api_signature.h"
+#include "extensions/renderer/get_script_context.h"
 #include "extensions/renderer/message_target.h"
 #include "extensions/renderer/messaging_util.h"
 #include "extensions/renderer/native_renderer_messaging_service.h"
 #include "extensions/renderer/script_context.h"
-#include "extensions/renderer/script_context_set.h"
 #include "gin/converter.h"
 
 namespace extensions {
@@ -112,9 +112,7 @@ RequestResult ExtensionHooksDelegate::HandleRequest(
       // TODO(devlin): Add getBackgroundPage, getExtensionTabs, getViews.
   };
 
-  ScriptContext* script_context =
-      ScriptContextSet::GetContextByV8Context(context);
-  DCHECK(script_context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
 
   Handler handler = nullptr;
   for (const auto& handler_entry : kHandlers) {
@@ -166,7 +164,7 @@ void ExtensionHooksDelegate::InitializeInstance(
   // isn't terribly efficient, but is only done for certain unpacked extensions
   // and only if they access the chrome.extension module.
   if (messaging_util::IsSendRequestDisabled(
-          ScriptContextSet::GetContextByV8Context(context))) {
+          GetScriptContextFromV8ContextChecked(context))) {
     static constexpr const char* kDeprecatedSendRequestProperties[] = {
         "sendRequest", "onRequest", "onRequestExternal"};
     for (const char* property : kDeprecatedSendRequestProperties) {

chrome/renderer/extensions/tabs_hooks_delegate.cc

@@ -10,11 +10,11 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
 #include "extensions/renderer/bindings/api_signature.h"
+#include "extensions/renderer/get_script_context.h"
 #include "extensions/renderer/message_target.h"
 #include "extensions/renderer/messaging_util.h"
 #include "extensions/renderer/native_renderer_messaging_service.h"
 #include "extensions/renderer/script_context.h"
-#include "extensions/renderer/script_context_set.h"
 #include "gin/converter.h"
 
 namespace extensions {
@@ -53,9 +53,7 @@ RequestResult TabsHooksDelegate::HandleRequest(
       {&TabsHooksDelegate::HandleConnect, kConnect},
   };
 
-  ScriptContext* script_context =
-      ScriptContextSet::GetContextByV8Context(context);
-  DCHECK(script_context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
 
   Handler handler = nullptr;
   for (const auto& handler_entry : kHandlers) {

extensions/renderer/BUILD.gn

@@ -118,6 +118,8 @@ source_set("renderer") {
     "file_system_natives.h",
     "gc_callback.cc",
     "gc_callback.h",
+    "get_script_context.cc",
+    "get_script_context.h",
     "gin_port.cc",
     "gin_port.h",
     "guest_view/extensions_guest_view_container.cc",

extensions/renderer/get_script_context.cc

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/renderer/get_script_context.h"
+
+#include "base/logging.h"
+#include "content/public/renderer/worker_thread.h"
+#include "extensions/renderer/script_context.h"
+#include "extensions/renderer/script_context_set.h"
+#include "extensions/renderer/worker_thread_dispatcher.h"
+
+namespace extensions {
+
+ScriptContext* GetScriptContextFromV8Context(v8::Local<v8::Context> context) {
+  ScriptContext* script_context =
+      content::WorkerThread::GetCurrentId() > 0
+          ? WorkerThreadDispatcher::GetScriptContext()
+          : ScriptContextSet::GetContextByV8Context(context);
+  DCHECK(!script_context || script_context->v8_context() == context);
+  return script_context;
+}
+
+ScriptContext* GetScriptContextFromV8ContextChecked(
+    v8::Local<v8::Context> context) {
+  ScriptContext* script_context = GetScriptContextFromV8Context(context);
+  CHECK(script_context);
+  return script_context;
+}
+
+}  // namespace extensions

extensions/renderer/get_script_context.h

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef EXTENSIONS_RENDERER_GET_SCRIPT_CONTEXT_H_
+#define EXTENSIONS_RENDERER_GET_SCRIPT_CONTEXT_H_
+
+#include "v8/include/v8.h"
+
+namespace extensions {
+class ScriptContext;
+
+// TODO(devlin): This is a very random place for these. But there's not really
+// a better one - ScriptContextSet is (currently) thread-agnostic, and it'd be
+// nice to avoid changing that.
+
+// Returns the ScriptContext associated with the given v8::Context. This is
+// designed to work for both main thread and worker thread contexts.
+ScriptContext* GetScriptContextFromV8Context(v8::Local<v8::Context> context);
+
+// Same as above, but CHECK()s the result before returning.
+ScriptContext* GetScriptContextFromV8ContextChecked(
+    v8::Local<v8::Context> context);
+
+}  // namespace extensions
+
+#endif  // EXTENSIONS_RENDERER_GET_SCRIPT_CONTEXT_H_

extensions/renderer/native_extension_bindings_system.cc

@@ -11,7 +11,6 @@
 #include "base/timer/elapsed_timer.h"
 #include "content/public/common/console_message_level.h"
 #include "content/public/common/content_switches.h"
-#include "content/public/renderer/worker_thread.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/event_filtering_info.h"
 #include "extensions/common/extension_api.h"
@@ -29,13 +28,12 @@
 #include "extensions/renderer/easy_unlock_proximity_required_stub.h"
 #include "extensions/renderer/extension_frame_helper.h"
 #include "extensions/renderer/extension_js_runner.h"
+#include "extensions/renderer/get_script_context.h"
 #include "extensions/renderer/ipc_message_sender.h"
 #include "extensions/renderer/module_system.h"
 #include "extensions/renderer/script_context.h"
-#include "extensions/renderer/script_context_set.h"
 #include "extensions/renderer/storage_area.h"
 #include "extensions/renderer/web_request_hooks.h"
-#include "extensions/renderer/worker_thread_dispatcher.h"
 #include "gin/converter.h"
 #include "gin/handle.h"
 #include "gin/per_context_data.h"
@@ -152,32 +150,13 @@ BindingsSystemPerContextData* GetBindingsDataFromContext(
   return data;
 }
 
-// Returns the ScriptContext associated with the given v8::Context.
-// TODO(devlin): Does this belong here, or should it be curried in as a
-// callback? This is the only place we have knowledge of worker vs. non-worker
-// threads here.
-ScriptContext* GetScriptContext(v8::Local<v8::Context> context) {
-  ScriptContext* script_context =
-      content::WorkerThread::GetCurrentId() > 0
-          ? WorkerThreadDispatcher::GetScriptContext()
-          : ScriptContextSet::GetContextByV8Context(context);
-  DCHECK(!script_context || script_context->v8_context() == context);
-  return script_context;
-}
-
-// Same as above, but CHECKs the result.
-ScriptContext* GetScriptContextChecked(v8::Local<v8::Context> context) {
-  ScriptContext* script_context = GetScriptContext(context);
-  CHECK(script_context);
-  return script_context;
-}
-
 void AddConsoleError(v8::Local<v8::Context> context, const std::string& error) {
-  ScriptContext* script_context = GetScriptContext(context);
+  ScriptContext* script_context = GetScriptContextFromV8Context(context);
   // Note: |script_context| may be null. During context tear down, we remove the
   // script context from the ScriptContextSet, so it's not findable by
-  // GetScriptContext. In theory, we shouldn't be running any bindings code
-  // after this point, but it seems that we are in at least some places.
+  // GetScriptContextFromV8Context. In theory, we shouldn't be running any
+  // bindings code after this point, but it seems that we are in at least some
+  // places.
   // TODO(devlin): Investigate. At least one place this manifests is in
   // messaging binding tear down exhibited by
   // MessagingApiTest.MessagingBackgroundOnly.
@@ -198,7 +177,7 @@ const base::DictionaryValue& GetAPISchema(const std::string& api_name) {
 // |context|.
 bool IsAPIFeatureAvailable(v8::Local<v8::Context> context,
                            const std::string& name) {
-  ScriptContext* script_context = GetScriptContextChecked(context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
   return script_context->GetAvailability(name).is_available();
 }
 
@@ -624,7 +603,7 @@ v8::Local<v8::Object> NativeExtensionBindingsSystem::GetAPIHelper(
     return value.As<v8::Object>();
   }
 
-  ScriptContext* script_context = GetScriptContextChecked(context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
   std::string api_name_string;
   CHECK(
       gin::Converter<std::string>::FromV8(isolate, api_name, &api_name_string));
@@ -697,7 +676,7 @@ void NativeExtensionBindingsSystem::GetInternalAPI(
 
   std::string api_name = gin::V8ToString(info[0]);
   const Feature* feature = FeatureProvider::GetAPIFeature(api_name);
-  ScriptContext* script_context = GetScriptContextChecked(context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
   if (!feature ||
       !script_context->IsAnyFeatureAvailableToContext(
           *feature, CheckAliasStatus::NOT_ALLOWED)) {
@@ -729,7 +708,7 @@ void NativeExtensionBindingsSystem::GetInternalAPI(
 void NativeExtensionBindingsSystem::SendRequest(
     std::unique_ptr<APIRequestHandler::Request> request,
     v8::Local<v8::Context> context) {
-  ScriptContext* script_context = GetScriptContextChecked(context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
 
   GURL url;
   blink::WebLocalFrame* frame = script_context->web_frame();
@@ -760,7 +739,7 @@ void NativeExtensionBindingsSystem::OnEventListenerChanged(
     const base::DictionaryValue* filter,
     bool update_lazy_listeners,
     v8::Local<v8::Context> context) {
-  ScriptContext* script_context = GetScriptContextChecked(context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
   // Note: Check context_type() first to avoid accessing ExtensionFrameHelper on
   // a worker thread.
   bool is_lazy =

extensions/renderer/runtime_hooks_delegate.cc

@@ -12,6 +12,7 @@
 #include "extensions/common/extension.h"
 #include "extensions/common/manifest.h"
 #include "extensions/renderer/bindings/api_signature.h"
+#include "extensions/renderer/get_script_context.h"
 #include "extensions/renderer/message_target.h"
 #include "extensions/renderer/messaging_util.h"
 #include "extensions/renderer/native_renderer_messaging_service.h"
@@ -31,8 +32,7 @@ void GetExtensionId(v8::Local<v8::Name> property_name,
   v8::HandleScope handle_scope(isolate);
   v8::Local<v8::Context> context = info.Holder()->CreationContext();
 
-  ScriptContext* script_context =
-      ScriptContextSet::GetContextByV8Context(context);
+  ScriptContext* script_context = GetScriptContextFromV8Context(context);
   // This could potentially be invoked after the script context is removed
   // (unlike the handler calls, which should only be invoked for valid
   // contexts).
@@ -76,9 +76,7 @@ RequestResult RuntimeHooksDelegate::HandleRequest(
       {&RuntimeHooksDelegate::HandleSendNativeMessage, kSendNativeMessage},
   };
 
-  ScriptContext* script_context =
-      ScriptContextSet::GetContextByV8Context(context);
-  DCHECK(script_context);
+  ScriptContext* script_context = GetScriptContextFromV8ContextChecked(context);
 
   Handler handler = nullptr;
   for (const auto& handler_entry : kHandlers) {