Refactoring: de-couple Extensions from "script injection System" [render side] : 1

extensions/renderer/script_injection.cc

 // Copyright 2014 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/script_injection.h"
 
 #include <map>
 
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram.h"
 #include "base/timer/elapsed_timer.h"
 #include "base/values.h"
 #include "content/public/renderer/render_view.h"
 #include "content/public/renderer/v8_value_converter.h"
 #include "extensions/common/extension.h"
+#include "extensions/common/extension_consumer.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/feature_switch.h"
 #include "extensions/common/manifest_handlers/csp_info.h"
 #include "extensions/renderer/dom_activity_logger.h"
 #include "extensions/renderer/extension_groups.h"
 #include "extensions/renderer/extensions_renderer_client.h"
 #include "third_party/WebKit/public/platform/WebString.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebScopedUserGesture.h"
 #include "third_party/WebKit/public/web/WebScriptSource.h"
 #include "third_party/WebKit/public/web/WebSecurityOrigin.h"
 #include "url/gurl.h"
 
 namespace extensions {
 
 namespace {
 
-typedef std::map<std::string, int> IsolatedWorldMap;
+using IsolatedWorldKey = std::pair<HostID, int>;
+using IsolatedWorldMap = std::map<IsolatedWorldKey, int>;
 base::LazyInstance<IsolatedWorldMap> g_isolated_worlds =
     LAZY_INSTANCE_INITIALIZER;
 
 const int64 kInvalidRequestId = -1;
 
 // The id of the next pending injection.
 int64 g_next_pending_id = 0;
 
 bool ShouldNotifyBrowserOfInjections() {
   return !FeatureSwitch::scripts_require_action()->IsEnabled();
 }
 
 // Append all the child frames of |parent_frame| to |frames_vector|.
 void AppendAllChildFrames(blink::WebFrame* parent_frame,
                           std::vector<blink::WebFrame*>* frames_vector) {
   DCHECK(parent_frame);
   for (blink::WebFrame* child_frame = parent_frame->firstChild(); child_frame;
        child_frame = child_frame->nextSibling()) {
     frames_vector->push_back(child_frame);
     AppendAllChildFrames(child_frame, frames_vector);
   }
 }
 
-// Gets the isolated world ID to use for the given |extension| in the given
-// |frame|. If no isolated world has been created for that extension,
-// one will be created and initialized.
-int GetIsolatedWorldIdForExtension(const Extension* extension,
-                                   blink::WebLocalFrame* frame) {
+// Gets the isolated world ID to use for the given |host, instance_id|
+// in the given |frame|. If no isolated world has been created for that
+// |host, instance_id| one will be created and initialized.
+int GetIsolatedWorldIdForInstance(const Host* host,
+                                  int instance_id,
+                                  blink::WebLocalFrame* frame) {
   static int g_next_isolated_world_id =
       ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId();
 
   IsolatedWorldMap& isolated_worlds = g_isolated_worlds.Get();
 
   int id = 0;
-  IsolatedWorldMap::iterator iter = isolated_worlds.find(extension->id());
+  const HostID& host_id = host->id();
+  IsolatedWorldKey key(host_id, instance_id);
+  IsolatedWorldMap::iterator iter = isolated_worlds.find(key);
   if (iter != isolated_worlds.end()) {
     id = iter->second;
   } else {
     id = g_next_isolated_world_id++;
     // This map will tend to pile up over time, but realistically, you're never
-    // going to have enough extensions for it to matter.
-    isolated_worlds[extension->id()] = id;
+    // going to have enough hosts for it to matter.
+    isolated_worlds[key] = id;
   }
 
+  GURL origin;
+  std::string name;
+  std::string name_web_view = "WebView";
+  const GURL* origin_ptr = &origin;
+  const std::string* name_ptr = &name;
+
+  if (host_id.type() == HostID::EXTENSIONS) {
+    const Extension* extension =
+        static_cast<const ExtensionConsumer*>(host)->extension();

[Devlin, 2015/02/04 17:01:14]

Generally, having something like
class AbstractClass {
  Type type() = 0;
}

class DerivedClass : public AbstractClass {
  Type type() { return TYPE_A; }
}

if (instance->type() == TYPE_A) {
  DerivedClass* derived = static_cast<DerivedClass*>(instance);
  derived->...
}

Is an antipattern. (Said differently, having times when you have to add RTTI to
a class and then cast it based on that.)

There are some times when we can't really avoid it, but I don't think this is
one of those times.  Instead, I think we should have methods on Host that handle
these things, e.g. Host::GetContentSecurityPolicy(), etc.

[Xi Han, 2015/02/05 16:06:20]

Adds some of these functions in the Host class.

+    frame->setIsolatedWorldContentSecurityPolicy(
+        id,
+        blink::WebString::fromUTF8(CSPInfo::GetContentSecurityPolicy(
+            extension)));
+    if (instance_id == 0) { // The instance is a TAB.
+      origin_ptr =  &extension->url();
+      name_ptr = &extension->name();
+    }
+  }
+  if (instance_id != 0)  // The instance is a <webview>.
+    name_ptr = &name_web_view;
+
   // We need to set the isolated world origin and CSP even if it's not a new
   // world since these are stored per frame, and we might not have used this
   // isolated world in this frame before.
   frame->setIsolatedWorldSecurityOrigin(
-      id, blink::WebSecurityOrigin::create(extension->url()));
-  frame->setIsolatedWorldContentSecurityPolicy(
-      id,
-      blink::WebString::fromUTF8(CSPInfo::GetContentSecurityPolicy(extension)));
+      id, blink::WebSecurityOrigin::create(*origin_ptr));
   frame->setIsolatedWorldHumanReadableName(
-      id,
-      blink::WebString::fromUTF8(extension->name()));
+      id, blink::WebString::fromUTF8(*name_ptr));
 
   return id;
 }
 
 }  // namespace
 
 // static
-std::string ScriptInjection::GetExtensionIdForIsolatedWorld(
-    int isolated_world_id) {
+HostID ScriptInjection::GetHostIdForIsolatedWorld(int isolated_world_id) {
   IsolatedWorldMap& isolated_worlds = g_isolated_worlds.Get();

[Devlin, 2015/02/04 17:01:14]

const &

[Xi Han, 2015/02/05 16:06:20]

Done.

 
-  for (IsolatedWorldMap::iterator iter = isolated_worlds.begin();
-       iter != isolated_worlds.end();
-       ++iter) {
-    if (iter->second == isolated_world_id)
-      return iter->first;
+  for (auto& kv : isolated_worlds) {

[Devlin, 2015/02/04 17:01:14]

const auto&

[Xi Han, 2015/02/05 16:06:20]

Done.

+    if (kv.second == isolated_world_id)
+      return kv.first.first;
   }
-  return std::string();
+  return HostID();
 }
 
 // static
-void ScriptInjection::RemoveIsolatedWorld(const std::string& extension_id) {
-  g_isolated_worlds.Get().erase(extension_id);
+void ScriptInjection::RemoveIsolatedWorld(const std::string& host_id) {
+  std::set<IsolatedWorldKey> keys_to_delete;
+  IsolatedWorldMap& isolated_worlds = g_isolated_worlds.Get();
+  for (auto& kv: isolated_worlds) {
+    const IsolatedWorldKey& key = kv.first;
+    if (key.first.id() == host_id)
+      keys_to_delete.insert(key);
+  }
+  for (auto& key : keys_to_delete)
+    isolated_worlds.erase(key);
 }
 
 ScriptInjection::ScriptInjection(
     scoped_ptr<ScriptInjector> injector,
     blink::WebLocalFrame* web_frame,
-    const std::string& extension_id,
+    const HostID& host_id,
+    int instance_id,
     UserScript::RunLocation run_location,
     int tab_id)
     : injector_(injector.Pass()),
       web_frame_(web_frame),
-      extension_id_(extension_id),
+      host_id_(host_id),
+      instance_id_(instance_id),
       run_location_(run_location),
       tab_id_(tab_id),
       request_id_(kInvalidRequestId),
       complete_(false) {
 }
 
 ScriptInjection::~ScriptInjection() {
   if (!complete_)
     injector_->OnWillNotInject(ScriptInjector::WONT_INJECT);
 }
 
 bool ScriptInjection::TryToInject(UserScript::RunLocation current_location,
-                                  const Extension* extension,
+                                  const Host* host,
                                   ScriptsRunInfo* scripts_run_info) {
   if (current_location < run_location_)
     return false;  // Wait for the right location.
 
   if (request_id_ != kInvalidRequestId)
     return false;  // We're waiting for permission right now, try again later.
 
-  if (!extension) {
+  if (!host) {
     NotifyWillNotInject(ScriptInjector::EXTENSION_REMOVED);
     return true;  // We're done.
   }
 
+  const Extension* extension =
+      static_cast<const ExtensionConsumer*>(host)->extension();

[Devlin, 2015/02/04 17:01:14]

This feels very unfinished to me.  I think I'd rather you just keep passing in
the extension until you're ready to convert all/most of the usages.  Also, it
makes it easier to lead to nasty breakages (see comment in
script_injection_manager.cc).

[Xi Han, 2015/02/05 16:06:20]

I updated all the if(!host) check, it is a great catch, and I do appreciate it.
For passing in host, if we can resolve these hazards, I still want to keep these
changes since it makes possible that each CL only contains few files changed. 

+
+  // TODO(hanxi): refactor ScriptInjection::CanExecuteOnFrame(...) and pass in
+  // Host.
   switch (injector_->CanExecuteOnFrame(
       extension, web_frame_, tab_id_, web_frame_->top()->document().url())) {
     case PermissionsData::ACCESS_DENIED:
       NotifyWillNotInject(ScriptInjector::NOT_ALLOWED);
       return true;  // We're done.
     case PermissionsData::ACCESS_WITHHELD:
       RequestPermission();
       return false;  // Wait around for permission.
     case PermissionsData::ACCESS_ALLOWED:
-      Inject(extension, scripts_run_info);
+      Inject(host, scripts_run_info);
       return true;  // We're done!
   }
 
   // Some compilers don't realize that we always return from the switch() above.
   // Make them happy.
   return false;
 }
 
-bool ScriptInjection::OnPermissionGranted(const Extension* extension,
+bool ScriptInjection::OnPermissionGranted(const Host* host,
                                           ScriptsRunInfo* scripts_run_info) {
-  if (!extension) {
+  if (!host) {
     NotifyWillNotInject(ScriptInjector::EXTENSION_REMOVED);
     return false;
   }
 
-  Inject(extension, scripts_run_info);
+  Inject(host, scripts_run_info);
   return true;
 }
 
 void ScriptInjection::RequestPermission() {
   content::RenderView* render_view =
       content::RenderView::FromWebView(web_frame()->top()->view());
 
   // If we are just notifying the browser of the injection, then send an
   // invalid request (which is treated like a notification).
   request_id_ = ShouldNotifyBrowserOfInjections() ? kInvalidRequestId
                                                   : g_next_pending_id++;
   render_view->Send(new ExtensionHostMsg_RequestScriptInjectionPermission(
       render_view->GetRoutingID(),
-      extension_id_,
+      host_id_.id(),
       injector_->script_type(),
       request_id_));
 }
 
 void ScriptInjection::NotifyWillNotInject(
     ScriptInjector::InjectFailureReason reason) {
   complete_ = true;
   injector_->OnWillNotInject(reason);
 }
 
-void ScriptInjection::Inject(const Extension* extension,
+void ScriptInjection::Inject(const Host* host,
                              ScriptsRunInfo* scripts_run_info) {
-  DCHECK(extension);
+  DCHECK(host);
   DCHECK(scripts_run_info);
   DCHECK(!complete_);
 
   if (ShouldNotifyBrowserOfInjections())
     RequestPermission();
 
   std::vector<blink::WebFrame*> frame_vector;
   frame_vector.push_back(web_frame_);
   if (injector_->ShouldExecuteInChildFrames())
     AppendAllChildFrames(web_frame_, &frame_vector);
@@ skipping 10 matching lines @@
   GURL top_url = web_frame_->top()->document().url();
   for (std::vector<blink::WebFrame*>::iterator iter = frame_vector.begin();
        iter != frame_vector.end();
        ++iter) {
     // TODO(dcheng): Unfortunately, the code as written won't work in an OOPI
     // world. This is just a temporary hack to make things compile.
     blink::WebLocalFrame* frame = (*iter)->toWebLocalFrame();
 
     // We recheck access here in the renderer for extra safety against races
     // with navigation, but different frames can have different URLs, and the
-    // extension might only have access to a subset of them.
-    // For child frames, we just skip ones the extension doesn't have access
+    // host might only have access to a subset of them.
+    // For child frames, we just skip ones the host doesn't have access
     // to and carry on.
     // Note: we don't consider ACCESS_WITHHELD because there is nowhere to
     // surface a request for a child frame.
     // TODO(rdevlin.cronin): We should ask for permission somehow.
+    const Extension* extension =
+        static_cast<const ExtensionConsumer*>(host)->extension();
     if (injector_->CanExecuteOnFrame(extension, frame, tab_id_, top_url) ==
         PermissionsData::ACCESS_DENIED) {
       DCHECK(frame->parent());
       continue;
     }
     if (inject_js)
-      InjectJs(extension, frame, execution_results.get());
+      InjectJs(host, frame, execution_results.get());
     if (inject_css)
       InjectCss(frame);
   }
 
   complete_ = true;
   injector_->OnInjectionComplete(execution_results.Pass(),
                                  scripts_run_info,
                                  run_location_);
 }
 
-void ScriptInjection::InjectJs(const Extension* extension,
+void ScriptInjection::InjectJs(const Host* host,
                                blink::WebLocalFrame* frame,
                                base::ListValue* execution_results) {
   std::vector<blink::WebScriptSource> sources =
       injector_->GetJsSources(run_location_);
   bool in_main_world = injector_->ShouldExecuteInMainWorld();
   int world_id = in_main_world
                      ? DOMActivityLogger::kMainWorldId
-                     : GetIsolatedWorldIdForExtension(extension, frame);
+                     : GetIsolatedWorldIdForInstance(host, instance_id_, frame);
   bool expects_results = injector_->ExpectsResults();
 
   base::ElapsedTimer exec_timer;
-  DOMActivityLogger::AttachToWorld(world_id, extension->id());
+  DOMActivityLogger::AttachToWorld(world_id, host->id().id());
   v8::HandleScope scope(v8::Isolate::GetCurrent());
   v8::Local<v8::Value> script_value;
   if (in_main_world) {
     // We only inject in the main world for javascript: urls.
     DCHECK_EQ(1u, sources.size());
 
     const blink::WebScriptSource& source = sources.front();
     if (expects_results)
       script_value = frame->executeScriptAndReturnValue(source);
     else
@@ skipping 35 matching lines @@
   std::vector<std::string> css_sources =
       injector_->GetCssSources(run_location_);
   for (std::vector<std::string>::const_iterator iter = css_sources.begin();
        iter != css_sources.end();
        ++iter) {
     frame->document().insertStyleSheet(blink::WebString::fromUTF8(*iter));
   }
 }
 
 }  // namespace extensions