[Extensions Bindings] Update feature initialization

Make two changes to how we initialize features for contexts in extension
native bindings:
- Treat runtime specially. This is necessary because certain runtime
  methods should be exposed only if there exists an extension that can
  connect to the given context (e.g., an extension that cooperates with
  a given website).  Simply checking the fature availability is
  insufficient. Update unittests to match.
- Treat web contexts specially.  Web contexts only ever have a handful
  (3) APIs that are potentially available to them, and are by far the
  most commonly created context. As such, add a fast path for them by
  only checking those possible features. (This matches logic in the
  JS bindings system).

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

extensions/renderer/BUILD.gn

@@ -90,6 +90,7 @@ source_set("renderer") {
     "dom_activity_logger.h",
     "event_bindings.cc",
     "event_bindings.h",
+    "extension_bindings_system.cc",
     "extension_bindings_system.h",
     "extension_frame_helper.cc",
     "extension_frame_helper.h",

extensions/renderer/extension_bindings_system.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/extension_bindings_system.h"
+
+#include "extensions/common/manifest_constants.h"
+#include "extensions/common/manifest_handlers/externally_connectable.h"
+#include "extensions/renderer/renderer_extension_registry.h"
+#include "extensions/renderer/script_context.h"
+
+namespace extensions {
+
+// static
+bool ExtensionBindingsSystem::IsRuntimeAvailableToContext(
+    ScriptContext* context) {
+  for (const auto& extension :
+       *RendererExtensionRegistry::Get()->GetMainThreadExtensionSet()) {
+    ExternallyConnectableInfo* info = static_cast<ExternallyConnectableInfo*>(
+        extension->GetManifestData(manifest_keys::kExternallyConnectable));
+    if (info && info->matches.MatchesURL(context->url()))
+      return true;
+  }
+  return false;
+}
+
+// static
+const char* ExtensionBindingsSystem::kWebAvailableFeatures[] = {
+    "app", "webstore", "dashboardPrivate",
+};
+
+}  // namespace extensions

extensions/renderer/extension_bindings_system.h

@@ -57,6 +57,20 @@ class ExtensionBindingsSystem {
   // Returns the associated RequestSender, if any.
   // TODO(devlin): Factor this out.
   virtual RequestSender* GetRequestSender() = 0;
+
+  // Returns true if any portion of the runtime API is available to the given
+  // |context|. This is different than just checking features because runtime's
+  // availability depends on the installed extensions and the active URL (in the
+  // case of extensions communicating with external websites).
+  static bool IsRuntimeAvailableToContext(ScriptContext* context);
+
+  // The APIs that could potentially be available to webpage-like contexts.
+  // This is the list of possible features; most web pages will not have access
+  // to these APIs.
+  // Note: `runtime` is not included here, since it's handled specially above.
+  // Note: We specify the size of the array to allow for its use in for loops
+  // without needing to expose a separate "kNumWebAvailableFeatures".
+  static const char* kWebAvailableFeatures[3];
 };
 
 }  // namespace extensions

extensions/renderer/js_extension_bindings_system.cc

@@ -116,19 +116,6 @@ v8::Local<v8::Object> GetOrCreateBindObjectIfAvailable(
   return bind_object.IsEmpty() ? GetOrCreateChrome(context) : bind_object;
 }
 
-// Determines if a ScriptContext can connect to any externally_connectable-
-// enabled extension.
-bool IsRuntimeAvailableToContext(ScriptContext* context) {
-  for (const auto& extension :
-       *RendererExtensionRegistry::Get()->GetMainThreadExtensionSet()) {
-    ExternallyConnectableInfo* info = static_cast<ExternallyConnectableInfo*>(
-        extension->GetManifestData(manifest_keys::kExternallyConnectable));
-    if (info && info->matches.MatchesURL(context->url()))
-      return true;
-  }
-  return false;
-}
-
 // Creates the event bindings if necessary for the given |context|.
 void MaybeCreateEventBindings(ScriptContext* context) {
   // chrome.Event is part of the public API (although undocumented). Make it
@@ -177,12 +164,10 @@ void JsExtensionBindingsSystem::UpdateBindingsForContext(
       // Hard-code registration of any APIs that are exposed to webpage-like
       // contexts, because it's too expensive to run the full bindings code.
       // All of the same permission checks will still apply.
-      if (context->GetAvailability("app").is_available())
-        RegisterBinding("app", "app", context);
-      if (context->GetAvailability("webstore").is_available())
-        RegisterBinding("webstore", "webstore", context);
-      if (context->GetAvailability("dashboardPrivate").is_available())
-        RegisterBinding("dashboardPrivate", "dashboardPrivate", context);
+      for (const char* feature_name : kWebAvailableFeatures) {
+        if (context->GetAvailability(feature_name).is_available())
+          RegisterBinding(feature_name, feature_name, context);
+      }
       if (IsRuntimeAvailableToContext(context))
         RegisterBinding("runtime", "runtime", context);
       break;

extensions/renderer/native_extension_bindings_system.cc

@@ -420,14 +420,65 @@ void NativeExtensionBindingsSystem::WillReleaseScriptContext(
 
 void NativeExtensionBindingsSystem::UpdateBindingsForContext(
     ScriptContext* context) {
-  v8::HandleScope handle_scope(context->isolate());
+  v8::Isolate* isolate = context->isolate();
+  v8::HandleScope handle_scope(isolate);
   v8::Local<v8::Context> v8_context = context->v8_context();
   v8::Local<v8::Object> chrome = GetOrCreateChrome(v8_context);
   if (chrome.IsEmpty())
     return;
 
-  BindingsSystemPerContextData* data = GetBindingsDataFromContext(v8_context);
-  DCHECK(data);
+  DCHECK(GetBindingsDataFromContext(v8_context));
+
+  auto set_accessor = [chrome, isolate,
+                       v8_context](base::StringPiece accessor_name) {
+    v8::Local<v8::String> api_name =
+        gin::StringToSymbol(isolate, accessor_name);
+    v8::Maybe<bool> success = chrome->SetAccessor(
+        v8_context, api_name, &BindingAccessor, nullptr, api_name);
+    return success.IsJust() && success.FromJust();
+  };
+
+  bool is_webpage = false;
+  switch (context->context_type()) {
+    case Feature::UNSPECIFIED_CONTEXT:
+    case Feature::WEB_PAGE_CONTEXT:
+    case Feature::BLESSED_WEB_PAGE_CONTEXT:
+      is_webpage = true;
+      break;
+    case Feature::SERVICE_WORKER_CONTEXT:
+      DCHECK(ExtensionsClient::Get()
+                 ->ExtensionAPIEnabledInExtensionServiceWorkers());
+    // Intentional fallthrough.
+    case Feature::BLESSED_EXTENSION_CONTEXT:
+    case Feature::LOCK_SCREEN_EXTENSION_CONTEXT:
+    case Feature::UNBLESSED_EXTENSION_CONTEXT:
+    case Feature::CONTENT_SCRIPT_CONTEXT:
+    case Feature::WEBUI_CONTEXT:
+      is_webpage = false;
+  }
+
+  if (is_webpage) {
+    // Hard-code registration of any APIs that are exposed to webpage-like
+    // contexts, because it's more expensive to iterate over all the existing
+    // features when only a handful could ever be available.
+    // All of the same permission checks will still apply.
+    // TODO(devlin): It could be interesting to apply this same logic to all
+    // context types, especially on a given platform. Something to think about
+    // for when we generate features.
+    for (const char* feature_name : kWebAvailableFeatures) {
+      if (context->GetAvailability(feature_name).is_available() &&
+          !set_accessor(feature_name)) {
+        LOG(ERROR) << "Failed to create API on Chrome object.";
+        return;
+      }
+    }
+
+    // Runtime is special (see IsRuntimeAvailableToContext()).
+    if (IsRuntimeAvailableToContext(context) && !set_accessor("runtime"))
+      LOG(ERROR) << "Failed to create API on Chrome object.";
+
+    return;
+  }
 
   const FeatureProvider* api_feature_provider =
       FeatureProvider::GetAPIFeatures();
@@ -471,11 +522,7 @@ void NativeExtensionBindingsSystem::UpdateBindingsForContext(
     base::StringPiece accessor_name =
         GetFirstDifferentAPIName(map_entry.first, base::StringPiece());
     last_accessor = accessor_name;
-    v8::Local<v8::String> api_name =
-        gin::StringToSymbol(v8_context->GetIsolate(), accessor_name);
-    v8::Maybe<bool> success = chrome->SetAccessor(
-        v8_context, api_name, &BindingAccessor, nullptr, api_name);
-    if (!success.IsJust() || !success.FromJust()) {
+    if (!set_accessor(accessor_name)) {
       LOG(ERROR) << "Failed to create API on Chrome object.";
       return;
     }

extensions/renderer/native_extension_bindings_system_unittest.cc

@@ -8,6 +8,7 @@
 #include "base/run_loop.h"
 #include "base/strings/stringprintf.h"
 #include "components/crx_file/id_util.h"
+#include "content/public/test/mock_render_thread.h"
 #include "extensions/common/extension.h"
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/extension_messages.h"
@@ -102,6 +103,7 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
   }
 
   void SetUp() override {
+    render_thread_ = base::MakeUnique<content::MockRenderThread>();
     script_context_set_ = base::MakeUnique<ScriptContextSet>(&extension_ids_);
     bindings_system_ = base::MakeUnique<NativeExtensionBindingsSystem>(
         base::Bind(&NativeExtensionBindingsSystemUnittest::MockSendRequestIPC,
@@ -125,6 +127,7 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
     ASSERT_TRUE(raw_script_contexts_.empty());
     script_context_set_.reset();
     bindings_system_.reset();
+    render_thread_.reset();
     APIBindingTest::TearDown();
   }
 
@@ -179,7 +182,10 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
     raw_script_contexts_.erase(iter);
   }
 
-  void RegisterExtension(const ExtensionId& id) { extension_ids_.insert(id); }
+  void RegisterExtension(scoped_refptr<const Extension> extension) {
+    extension_ids_.insert(extension->id());
+    RendererExtensionRegistry::Get()->Insert(extension);
+  }
 
   void InitEventChangeHandler() {
     event_change_handler_ = base::MakeUnique<MockEventChangeHandler>();
@@ -196,6 +202,7 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
 
  private:
   ExtensionIdSet extension_ids_;
+  std::unique_ptr<content::MockRenderThread> render_thread_;
   std::unique_ptr<ScriptContextSet> script_context_set_;
   std::vector<ScriptContext*> raw_script_contexts_;
   std::unique_ptr<NativeExtensionBindingsSystem> bindings_system_;
@@ -212,7 +219,7 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
 TEST_F(NativeExtensionBindingsSystemUnittest, Basic) {
   scoped_refptr<Extension> extension = CreateExtension(
       "foo", ItemType::EXTENSION, {"idle", "power", "webRequest"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -322,7 +329,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, Basic) {
 TEST_F(NativeExtensionBindingsSystemUnittest, Events) {
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle", "power"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -364,7 +371,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, Events) {
 TEST_F(NativeExtensionBindingsSystemUnittest, APIObjectsAreEqual) {
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -392,7 +399,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle", "power"});
 
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -448,7 +455,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, TestBridgingToJSCustomBindings) {
 
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -538,7 +545,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, TestSendRequestHook) {
 
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -574,7 +581,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, TestEventRegistration) {
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle", "power"});
 
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -628,7 +635,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
   scoped_refptr<Extension> app = CreateExtension("foo", ItemType::PLATFORM_APP,
                                                  std::vector<std::string>());
   EXPECT_TRUE(app->is_platform_app());
-  RegisterExtension(app->id());
+  RegisterExtension(app);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -667,7 +674,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
        TestPrefixedApiMethodsAndSystemBinding) {
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"system.cpu"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -707,7 +714,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
 TEST_F(NativeExtensionBindingsSystemUnittest, TestLastError) {
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, {"idle", "power"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -757,7 +764,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, TestLastError) {
 TEST_F(NativeExtensionBindingsSystemUnittest, TestCustomProperties) {
   scoped_refptr<Extension> extension =
       CreateExtension("storage extension", ItemType::EXTENSION, {"storage"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -793,7 +800,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
        CheckDifferentContextsHaveDifferentAPIObjects) {
   scoped_refptr<Extension> extension =
       CreateExtension("extension", ItemType::EXTENSION, {"idle"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context_a = MainContext();
@@ -828,45 +835,78 @@ TEST_F(NativeExtensionBindingsSystemUnittest,
 // context are properly present or absent from the API object.
 TEST_F(NativeExtensionBindingsSystemUnittest,
        CheckRestrictedFeaturesBasedOnContext) {
-  scoped_refptr<Extension> extension =
-      CreateExtension("extension", ItemType::EXTENSION, {"idle"});
-  RegisterExtension(extension->id());
+  scoped_refptr<Extension> connectable_extension;
+  {
+    DictionaryBuilder manifest;
+    manifest.Set("name", "connectable")
+        .Set("manifest_version", 2)
+        .Set("version", "0.1")
+        .Set("description", "test extension");
+    DictionaryBuilder connectable;
+    connectable.Set("matches",
+                    ListBuilder().Append("*://example.com/*").Build());
+    manifest.Set("externally_connectable", connectable.Build());
+    connectable_extension =
+        ExtensionBuilder()
+            .SetManifest(manifest.Build())
+            .SetLocation(Manifest::INTERNAL)
+            .SetID(crx_file::id_util::GenerateId("connectable"))
+            .Build();
+  }
+
+  RegisterExtension(connectable_extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> blessed_context = MainContext();
-  v8::Local<v8::Context> webpage_context = AddContext();
+  v8::Local<v8::Context> connectable_webpage_context = AddContext();
+  v8::Local<v8::Context> nonconnectable_webpage_context = AddContext();
 
   // Create two contexts - a blessed extension context and a normal web page
   // context.
-  ScriptContext* blessed_script_context = CreateScriptContext(
-      blessed_context, extension.get(), Feature::BLESSED_EXTENSION_CONTEXT);
-  blessed_script_context->set_url(extension->url());
+  ScriptContext* blessed_script_context =
+      CreateScriptContext(blessed_context, connectable_extension.get(),
+                          Feature::BLESSED_EXTENSION_CONTEXT);
+  blessed_script_context->set_url(connectable_extension->url());
   bindings_system()->UpdateBindingsForContext(blessed_script_context);
 
-  ScriptContext* webpage_script_context =
-      CreateScriptContext(webpage_context, nullptr, Feature::WEB_PAGE_CONTEXT);
-  webpage_script_context->set_url(GURL("http://example.com"));
-  bindings_system()->UpdateBindingsForContext(webpage_script_context);
+  ScriptContext* connectable_webpage_script_context = CreateScriptContext(
+      connectable_webpage_context, nullptr, Feature::WEB_PAGE_CONTEXT);
+  connectable_webpage_script_context->set_url(GURL("http://example.com"));
+  bindings_system()->UpdateBindingsForContext(
+      connectable_webpage_script_context);
+
+  ScriptContext* nonconnectable_webpage_script_context = CreateScriptContext(
+      nonconnectable_webpage_context, nullptr, Feature::WEB_PAGE_CONTEXT);
+  nonconnectable_webpage_script_context->set_url(GURL("http://notexample.com"));
+  bindings_system()->UpdateBindingsForContext(
+      nonconnectable_webpage_script_context);
 
   // Check that properties are correctly restricted. The blessed context should
-  // have access to the whole runtime API, but the webpage should only have
-  // access to sendMessage.
+  // have access to the whole runtime API, the connectable webpage should only
+  // have access to sendMessage, and the nonconnectable webpage should not have
+  // access to any of the API.
+  const char kRuntime[] = "chrome.runtime";
   const char kSendMessage[] = "chrome.runtime.sendMessage";
   const char kGetUrl[] = "chrome.runtime.getURL";
   const char kOnMessage[] = "chrome.runtime.onMessage";
+  ASSERT_TRUE(PropertyExists(blessed_context, kRuntime));
   EXPECT_TRUE(PropertyExists(blessed_context, kSendMessage));
   EXPECT_TRUE(PropertyExists(blessed_context, kGetUrl));
   EXPECT_TRUE(PropertyExists(blessed_context, kOnMessage));
-  EXPECT_TRUE(PropertyExists(webpage_context, kSendMessage));
-  EXPECT_FALSE(PropertyExists(webpage_context, kGetUrl));
-  EXPECT_FALSE(PropertyExists(webpage_context, kOnMessage));
+
+  ASSERT_TRUE(PropertyExists(connectable_webpage_context, kRuntime));
+  EXPECT_TRUE(PropertyExists(connectable_webpage_context, kSendMessage));
+  EXPECT_FALSE(PropertyExists(connectable_webpage_context, kGetUrl));
+  EXPECT_FALSE(PropertyExists(connectable_webpage_context, kOnMessage));
+
+  EXPECT_FALSE(PropertyExists(nonconnectable_webpage_context, kRuntime));
 }
 
 // Tests behavior when script sets window.chrome to be various things.
 TEST_F(NativeExtensionBindingsSystemUnittest, TestUsingOtherChromeObjects) {
   scoped_refptr<Extension> extension = CreateExtension(
       "extension", ItemType::EXTENSION, std::vector<std::string>());
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context_a = MainContext();
@@ -935,7 +975,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, TestUsingOtherChromeObjects) {
 TEST_F(NativeExtensionBindingsSystemUnittest, TestUpdatingPermissions) {
   scoped_refptr<Extension> extension =
       CreateExtension("extension", ItemType::EXTENSION, {"idle"});
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();
@@ -1030,7 +1070,7 @@ TEST_F(NativeExtensionBindingsSystemUnittest, UnmanagedEvents) {
 
   scoped_refptr<Extension> extension =
       CreateExtension("foo", ItemType::EXTENSION, std::vector<std::string>());
-  RegisterExtension(extension->id());
+  RegisterExtension(extension);
 
   v8::HandleScope handle_scope(isolate());
   v8::Local<v8::Context> context = MainContext();