Add frameId to chrome.tabs.executeScript/insertCSS

- Re-implemented https://codereview.chromium.org/952473002/, with all
  checks at the browser side instead of just the renderer.

- Use the last committed URL for checking permissions instead of the
  visible URL. As a result, executeScript/insertCSS will now succeed in
  frames where the currently committed page is scriptable by extensions,
  but the target of the pending navigation is is not.

- ExecuteScript: Do not send IPC to non-live frames (they're not going
  to reply anyway).

- Include URL in the error if the extension has the tabs permission
  (follow-up to TODO from https://codereview.chromium.org/1414223005).

BUG=63979,551626
TEST=./browser_tests --gtest_filter=ExecuteScriptApiTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review URL: https://codereview.chromium.org/1628423002

Cr-Commit-Position: refs/heads/master@{#374057}

8 files changed, 123 insertions, 52 deletions

diff --git a/extensions/browser/api/execute_code_function.cc b/extensions/browser/api/execute_code_function.cc
index 0f98df3..53fcb44 100644
--- a/extensions/browser/api/execute_code_function.cc
+++ b/extensions/browser/api/execute_code_function.cc
@@ -8,6 +8,7 @@
 #include "extensions/browser/api/execute_code_function.h"
 
 #include "extensions/browser/component_extension_resource_manager.h"
+#include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/file_reader.h"
 #include "extensions/common/error_utils.h"
@@ -139,8 +140,11 @@ bool ExecuteCodeFunction::Execute(const std::string& code_string) {
 
   ScriptExecutor::FrameScope frame_scope =
       details_->all_frames.get() && *details_->all_frames
-          ? ScriptExecutor::ALL_FRAMES
-          : ScriptExecutor::TOP_FRAME;
+          ? ScriptExecutor::INCLUDE_SUB_FRAMES
+          : ScriptExecutor::SINGLE_FRAME;
+
+  int frame_id = details_->frame_id.get() ? *details_->frame_id
+                                          : ExtensionApiFrameIdMap::kTopFrameId;
 
   ScriptExecutor::MatchAboutBlank match_about_blank =
       details_->match_about_blank.get() && *details_->match_about_blank
@@ -163,18 +167,11 @@ bool ExecuteCodeFunction::Execute(const std::string& code_string) {
   CHECK_NE(UserScript::UNDEFINED, run_at);
 
   executor->ExecuteScript(
-      host_id_,
-      script_type,
-      code_string,
-      frame_scope,
-      match_about_blank,
-      run_at,
-      ScriptExecutor::ISOLATED_WORLD,
+      host_id_, script_type, code_string, frame_scope, frame_id,
+      match_about_blank, run_at, ScriptExecutor::ISOLATED_WORLD,
       IsWebView() ? ScriptExecutor::WEB_VIEW_PROCESS
                   : ScriptExecutor::DEFAULT_PROCESS,
-      GetWebViewSrc(),
-      file_url_,
-      user_gesture_,
+      GetWebViewSrc(), file_url_, user_gesture_,
       has_callback() ? ScriptExecutor::JSON_SERIALIZED_RESULT
                      : ScriptExecutor::NO_RESULT,
       base::Bind(&ExecuteCodeFunction::OnExecuteCodeFinished, this));
diff --git a/extensions/browser/extension_api_frame_id_map.cc b/extensions/browser/extension_api_frame_id_map.cc
index 72949be..e844fac 100644
--- a/extensions/browser/extension_api_frame_id_map.cc
+++ b/extensions/browser/extension_api_frame_id_map.cc
@@ -24,6 +24,7 @@ base::LazyInstance<ExtensionApiFrameIdMap>::Leaky g_map_instance =
 }  // namespace
 
 const int ExtensionApiFrameIdMap::kInvalidFrameId = -1;
+const int ExtensionApiFrameIdMap::kTopFrameId = 0;
 
 ExtensionApiFrameIdMap::CachedFrameIdPair::CachedFrameIdPair()
     : frame_id(kInvalidFrameId), parent_frame_id(kInvalidFrameId) {}
@@ -75,7 +76,7 @@ int ExtensionApiFrameIdMap::GetFrameId(content::RenderFrameHost* rfh) {
     return kInvalidFrameId;
   if (rfh->GetParent())
     return rfh->GetFrameTreeNodeId();
-  return 0;  // Main frame.
+  return kTopFrameId;
 }
 
 int ExtensionApiFrameIdMap::GetParentFrameId(content::RenderFrameHost* rfh) {
@@ -96,7 +97,7 @@ content::RenderFrameHost* ExtensionApiFrameIdMap::GetRenderFrameHostById(
   if (frame_id == kInvalidFrameId)
     return nullptr;
 
-  if (frame_id == 0)
+  if (frame_id == kTopFrameId)
     return web_contents->GetMainFrame();
 
   DCHECK_GE(frame_id, 1);
diff --git a/extensions/browser/extension_api_frame_id_map.h b/extensions/browser/extension_api_frame_id_map.h
index c3af8b3..8bb1422 100644
--- a/extensions/browser/extension_api_frame_id_map.h
+++ b/extensions/browser/extension_api_frame_id_map.h
@@ -48,6 +48,9 @@ class ExtensionApiFrameIdMap {
   // An invalid extension API frame ID.
   static const int kInvalidFrameId;
 
+  // Extension API frame ID of the top-level frame.
+  static const int kTopFrameId;
+
   static ExtensionApiFrameIdMap* Get();
 
   // Get the extension API frame ID for |rfh|.
diff --git a/extensions/browser/script_executor.cc b/extensions/browser/script_executor.cc
index 687be48..230bca3 100644
--- a/extensions/browser/script_executor.cc
+++ b/extensions/browser/script_executor.cc
@@ -13,6 +13,7 @@
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/script_execution_observer.h"
 #include "extensions/common/extension_messages.h"
@@ -28,6 +29,7 @@ namespace extensions {
 namespace {
 
 const char* kRendererDestroyed = "The tab was closed.";
+const char* kFrameRemoved = "The frame was removed.";
 
 // A handler for a single injection request. On creation this will send the
 // injection request to the renderer, and it will be destroyed after either the
@@ -38,18 +40,28 @@ class Handler : public content::WebContentsObserver {
           content::WebContents* web_contents,
           const ExtensionMsg_ExecuteCode_Params& params,
           ScriptExecutor::FrameScope scope,
+          int frame_id,
           const ScriptExecutor::ExecuteScriptCallback& callback)
       : content::WebContentsObserver(web_contents),
         script_observers_(AsWeakPtr(script_observers)),
         host_id_(params.host_id),
         request_id_(params.request_id),
+        include_sub_frames_(scope == ScriptExecutor::INCLUDE_SUB_FRAMES),
+        root_rfh_(ExtensionApiFrameIdMap::GetRenderFrameHostById(web_contents,
+                                                                 frame_id)),
+        root_is_main_frame_(root_rfh_ ? !root_rfh_->GetParent() : false),
         callback_(callback) {
-    if (scope == ScriptExecutor::ALL_FRAMES) {
-      web_contents->ForEachFrame(base::Bind(&Handler::SendExecuteCode,
-                                            base::Unretained(this), params));
-    } else {
-      SendExecuteCode(params, web_contents->GetMainFrame());
+    if (root_rfh_) {
+      if (include_sub_frames_) {
+        web_contents->ForEachFrame(base::Bind(&Handler::SendExecuteCode,
+                                              base::Unretained(this), params));
+      } else {
+        SendExecuteCode(params, root_rfh_);
+      }
     }
+
+    if (pending_render_frames_.empty())
+      Finish();
   }
 
  private:
@@ -92,10 +104,25 @@ class Handler : public content::WebContentsObserver {
   // the number of pending messages.
   void SendExecuteCode(const ExtensionMsg_ExecuteCode_Params& params,
                        content::RenderFrameHost* frame) {
+    if (!frame->IsRenderFrameLive())
+      return;
+    DCHECK(!root_is_main_frame_ || ShouldIncludeFrame(frame));
+    if (!root_is_main_frame_ && !ShouldIncludeFrame(frame))
+      return;
     pending_render_frames_.insert(frame);
     frame->Send(new ExtensionMsg_ExecuteCode(frame->GetRoutingID(), params));
   }
 
+  // Returns whether a frame is the root frame or a descendant of it.
+  bool ShouldIncludeFrame(content::RenderFrameHost* frame) {
+    while (frame) {
+      if (frame == root_rfh_)
+        return true;
+      frame = frame->GetParent();
+    }
+    return false;
+  }
+
   // Handles the ExecuteCodeFinished message.
   void OnExecuteCodeFinished(content::RenderFrameHost* render_frame_host,
                              int request_id,
@@ -106,22 +133,22 @@ class Handler : public content::WebContentsObserver {
     DCHECK(!pending_render_frames_.empty());
     bool erased = pending_render_frames_.erase(render_frame_host) == 1;
     DCHECK(erased);
-    bool is_main_frame = web_contents()->GetMainFrame() == render_frame_host;
+    bool is_root_frame = root_rfh_ == render_frame_host;
 
     // Set the result, if there is one.
     const base::Value* script_value = nullptr;
     if (result_list.Get(0u, &script_value)) {
       // If this is the main result, we put it at index 0. Otherwise, we just
       // append it at the end.
-      if (is_main_frame && !results_.empty())
+      if (is_root_frame && !results_.empty())
         CHECK(results_.Insert(0u, script_value->DeepCopy()));
       else
         results_.Append(script_value->DeepCopy());
     }
 
-    if (is_main_frame) {  // Only use the main frame's error and url.
-      main_frame_error_ = error;
-      main_frame_url_ = on_url;
+    if (is_root_frame) {  // Only use the root frame's error and url.
+      root_frame_error_ = error;
+      root_frame_url_ = on_url;
     }
 
     // Wait until the final request finishes before reporting back.
@@ -130,23 +157,24 @@ class Handler : public content::WebContentsObserver {
   }
 
   void Finish() {
-    if (main_frame_url_.is_empty()) {
-      // We never finished the main frame injection.
-      main_frame_error_ = kRendererDestroyed;
+    if (root_frame_url_.is_empty()) {
+      // We never finished the root frame injection.
+      root_frame_error_ =
+          root_is_main_frame_ ? kRendererDestroyed : kFrameRemoved;
       results_.Clear();
     }
 
-    if (script_observers_.get() && main_frame_error_.empty() &&
+    if (script_observers_.get() && root_frame_error_.empty() &&
         host_id_.type() == HostID::EXTENSIONS) {
       ScriptExecutionObserver::ExecutingScriptsMap id_map;
       id_map[host_id_.id()] = std::set<std::string>();
       FOR_EACH_OBSERVER(
           ScriptExecutionObserver, *script_observers_,
-          OnScriptsExecuted(web_contents(), id_map, main_frame_url_));
+          OnScriptsExecuted(web_contents(), id_map, root_frame_url_));
     }
 
     if (!callback_.is_null())
-      callback_.Run(main_frame_error_, main_frame_url_, results_);
+      callback_.Run(root_frame_error_, root_frame_url_, results_);
     delete this;
   }
 
@@ -158,17 +186,27 @@ class Handler : public content::WebContentsObserver {
   // The request id of the injection.
   int request_id_;
 
+  // Whether to inject in |root_rfh_| and all of its descendant frames.
+  bool include_sub_frames_;
+
+  // The frame (and optionally its descendant frames) where the injection will
+  // occur.
+  content::RenderFrameHost* root_rfh_;
+
+  // Whether |root_rfh_| is the main frame of a tab.
+  bool root_is_main_frame_;
+
   // The hosts of the still-running injections.
   std::set<content::RenderFrameHost*> pending_render_frames_;
 
   // The results of the injection.
   base::ListValue results_;
 
-  // The error from injecting into the main frame.
-  std::string main_frame_error_;
+  // The error from injecting into the root frame.
+  std::string root_frame_error_;
 
-  // The url of the main frame.
-  GURL main_frame_url_;
+  // The url of the root frame.
+  GURL root_frame_url_;
 
   // The callback to run after all injections complete.
   ScriptExecutor::ExecuteScriptCallback callback_;
@@ -197,6 +235,7 @@ void ScriptExecutor::ExecuteScript(const HostID& host_id,
                                    ScriptExecutor::ScriptType script_type,
                                    const std::string& code,
                                    ScriptExecutor::FrameScope frame_scope,
+                                   int frame_id,
                                    ScriptExecutor::MatchAboutBlank about_blank,
                                    UserScript::RunLocation run_at,
                                    ScriptExecutor::WorldType world_type,
@@ -232,7 +271,8 @@ void ScriptExecutor::ExecuteScript(const HostID& host_id,
   params.user_gesture = user_gesture;
 
   // Handler handles IPCs and deletes itself on completion.
-  new Handler(script_observers_, web_contents_, params, frame_scope, callback);
+  new Handler(script_observers_, web_contents_, params, frame_scope, frame_id,
+              callback);
 }
 
 }  // namespace extensions
diff --git a/extensions/browser/script_executor.h b/extensions/browser/script_executor.h
index 4f81b78..25e6c54 100644
--- a/extensions/browser/script_executor.h
+++ b/extensions/browser/script_executor.h
@@ -44,8 +44,8 @@ class ScriptExecutor {
 
   // The scope of the script injection across the frames.
   enum FrameScope {
-    TOP_FRAME,
-    ALL_FRAMES,
+    SINGLE_FRAME,
+    INCLUDE_SUB_FRAMES,
   };
 
   // Whether to insert the script in about: frames when its origin matches
@@ -82,6 +82,10 @@ class ScriptExecutor {
   // Executes a script. The arguments match ExtensionMsg_ExecuteCode_Params in
   // extension_messages.h (request_id is populated automatically).
   //
+  // The script will be executed in the frame identified by |frame_id| (which is
+  // an extension API frame ID). If |frame_scope| is INCLUDE_SUB_FRAMES, then
+  // the script will also be executed in all descendants of the frame.
+  //
   // |callback| will always be called even if the IPC'd renderer is destroyed
   // before a response is received (in this case the callback will be with a
   // failure and appropriate error message).
@@ -89,6 +93,7 @@ class ScriptExecutor {
                      ScriptType script_type,
                      const std::string& code,
                      FrameScope frame_scope,
+                     int frame_id,
                      MatchAboutBlank match_about_blank,
                      UserScript::RunLocation run_at,
                      WorldType world_type,
diff --git a/extensions/common/api/extension_types.json b/extensions/common/api/extension_types.json
index 29562ee..9f27590 100644
--- a/extensions/common/api/extension_types.json
+++ b/extensions/common/api/extension_types.json
@@ -45,7 +45,17 @@
         "properties": {
           "code": {"type": "string", "optional": true, "description": "JavaScript or CSS code to inject.<br><br><b>Warning:</b><br>Be careful using the <code>code</code> parameter. Incorrect use of it may open your extension to <a href=\"https://en.wikipedia.org/wiki/Cross-site_scripting\">cross site scripting</a> attacks."},
           "file": {"type": "string", "optional": true, "description": "JavaScript or CSS file to inject."},
-          "allFrames": {"type": "boolean", "optional": true, "description": "If allFrames is <code>true</code>, implies that the JavaScript or CSS should be injected into all frames of current page. By default, it's <code>false</code> and is only injected into the top frame."},
+          "allFrames": {
+            "type": "boolean",
+            "optional": true,
+            "description": "If allFrames is <code>true</code>, implies that the JavaScript or CSS should be injected into all frames of current page. By default, it's <code>false</code> and is only injected into the top frame. If <code>true</code> and <code>frameId</code> is set, then the code is inserted in the selected frame and all of its child frames."
+          },
+          "frameId": {
+            "type": "integer",
+            "optional": true,
+            "minimum": 0,
+            "description": "The <a href='webNavigation#frame_ids'>frame</a> where the script or CSS should be injected. Defaults to 0 (the top-level frame)."
+          },
           "matchAboutBlank": {"type": "boolean", "optional": true, "description": "If matchAboutBlank is true, then the code is also injected in about:blank and about:srcdoc frames if your extension has access to its parent document. Code cannot be inserted in top-level about:-frames. By default it is <code>false</code>."},
           "runAt": {
             "$ref": "RunAt",
diff --git a/extensions/renderer/programmatic_script_injector.cc b/extensions/renderer/programmatic_script_injector.cc
index 13f0a20..e19a4c9 100644
--- a/extensions/renderer/programmatic_script_injector.cc
+++ b/extensions/renderer/programmatic_script_injector.cc
@@ -13,14 +13,15 @@
 #include "extensions/common/error_utils.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/manifest_constants.h"
+#include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permissions_data.h"
 #include "extensions/renderer/injection_host.h"
+#include "extensions/renderer/renderer_extension_registry.h"
 #include "extensions/renderer/script_context.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/WebScriptSource.h"
-#include "url/origin.h"
 
 namespace extensions {
 
@@ -31,8 +32,10 @@ ProgrammaticScriptInjector::ProgrammaticScriptInjector(
       url_(
           ScriptContext::GetDataSourceURLForFrame(render_frame->GetWebFrame())),
       finished_(false) {
-  effective_url_ = ScriptContext::GetEffectiveDocumentURL(
-      render_frame->GetWebFrame(), url_, params.match_about_blank);
+  if (url_.SchemeIs(url::kAboutScheme)) {
+    origin_for_about_error_ =
+        render_frame->GetWebFrame()->securityOrigin().toString().utf8();
+  }
 }
 
 ProgrammaticScriptInjector::~ProgrammaticScriptInjector() {
@@ -130,16 +133,15 @@ void ProgrammaticScriptInjector::OnWillNotInject(
   std::string error;
   switch (reason) {
     case NOT_ALLOWED:
-      if (url_.SchemeIs(url::kAboutScheme)) {
+      if (!CanShowUrlInError()) {
+        error = manifest_errors::kCannotAccessPage;
+      } else if (!origin_for_about_error_.empty()) {
         error = ErrorUtils::FormatErrorMessage(
             manifest_errors::kCannotAccessAboutUrl, url_.spec(),
-            url::Origin(effective_url_).Serialize());
+            origin_for_about_error_);
       } else {
-        // TODO(?) It would be nice to show kCannotAccessPageWithUrl here if
-        // this is triggered by an extension with tabs permission. See
-        // https://codereview.chromium.org/1414223005/diff/1/extensions/
-        // common/manifest_constants.cc#newcode269
-        error = manifest_errors::kCannotAccessPage;
+        error = ErrorUtils::FormatErrorMessage(
+            manifest_errors::kCannotAccessPageWithUrl, url_.spec());
       }
       break;
     case EXTENSION_REMOVED:  // no special error here.
@@ -149,6 +151,17 @@ void ProgrammaticScriptInjector::OnWillNotInject(
   Finish(error, render_frame);
 }
 
+bool ProgrammaticScriptInjector::CanShowUrlInError() const {
+  if (params_->host_id.type() != HostID::EXTENSIONS)
+    return false;
+  const Extension* extension =
+      RendererExtensionRegistry::Get()->GetByID(params_->host_id.id());
+  if (!extension)
+    return false;
+  return extension->permissions_data()->active_permissions().HasAPIPermission(
+      APIPermission::kTab);
+}
+
 UserScript::RunLocation ProgrammaticScriptInjector::GetRunLocation() const {
   return static_cast<UserScript::RunLocation>(params_->run_at);
 }
diff --git a/extensions/renderer/programmatic_script_injector.h b/extensions/renderer/programmatic_script_injector.h
index 159ff1c..a7af9ff 100644
--- a/extensions/renderer/programmatic_script_injector.h
+++ b/extensions/renderer/programmatic_script_injector.h
@@ -50,6 +50,9 @@ class ProgrammaticScriptInjector : public ScriptInjector {
   void OnWillNotInject(InjectFailureReason reason,
                        content::RenderFrame* render_frame) override;
 
+  // Whether it is safe to include information about the URL in error messages.
+  bool CanShowUrlInError() const;
+
   // Return the run location for this injector.
   UserScript::RunLocation GetRunLocation() const;
 
@@ -63,10 +66,9 @@ class ProgrammaticScriptInjector : public ScriptInjector {
   // The url of the frame into which we are injecting.
   GURL url_;
 
-  // The URL of the frame's origin. This is usually identical to |url_|, but
-  // could be different for e.g. about:blank URLs. Do not use this value to make
-  // security decisions, to avoid race conditions (e.g. due to navigation).
-  GURL effective_url_;
+  // The serialization of the frame's origin if the frame is an about:-URL. This
+  // is used to provide user-friendly messages.
+  std::string origin_for_about_error_;
 
   // The results of the script execution.
   base::ListValue results_;