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

extensions/renderer/script_injection.cc

Index: extensions/renderer/script_injection.cc
diff --git a/extensions/renderer/script_injection.cc b/extensions/renderer/script_injection.cc
index ea588fe90a47b20b2353411dc9ede8a463d6fc84..cdd74e6b9e24bcb37afa0c8276ab0103c0e9f55b 100644
--- a/extensions/renderer/script_injection.cc
+++ b/extensions/renderer/script_injection.cc
@@ -13,6 +13,7 @@
 #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"
@@ -31,7 +32,8 @@ 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;
 
@@ -55,38 +57,58 @@ void AppendAllChildFrames(blink::WebFrame* parent_frame,
   }
 }
 
-// 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;
 }
@@ -94,33 +116,40 @@ int GetIsolatedWorldIdForExtension(const Extension* extension,
 }  // 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),
@@ -133,7 +162,7 @@ ScriptInjection::~ScriptInjection() {
 }
 
 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.
@@ -141,11 +170,16 @@ bool ScriptInjection::TryToInject(UserScript::RunLocation current_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:
@@ -155,7 +189,7 @@ bool ScriptInjection::TryToInject(UserScript::RunLocation current_location,
       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!
   }
 
@@ -164,14 +198,14 @@ bool ScriptInjection::TryToInject(UserScript::RunLocation current_location,
   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;
 }
 
@@ -185,7 +219,7 @@ void ScriptInjection::RequestPermission() {
                                                   : g_next_pending_id++;
   render_view->Send(new ExtensionHostMsg_RequestScriptInjectionPermission(
       render_view->GetRoutingID(),
-      extension_id_,
+      host_id_.id(),
       injector_->script_type(),
       request_id_));
 }
@@ -196,9 +230,9 @@ void ScriptInjection::NotifyWillNotInject(
   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_);
 
@@ -229,19 +263,21 @@ void ScriptInjection::Inject(const Extension* extension,
 
     // 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);
   }
@@ -252,7 +288,7 @@ void ScriptInjection::Inject(const Extension* extension,
                                  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 =
@@ -260,11 +296,11 @@ void ScriptInjection::InjectJs(const Extension* extension,
   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) {