[Extensions] Support specifying multiple frame IDs in ScriptExecutor

Allow ScriptExecutor to support multiple frame IDs being specified.
If multiple frame IDs are specified, the script will be injected into
each, returning only when all are complete. If INCLUDE_SUB_FRAMES is
also specified, the script is injected into all frames that are a
subframe of any of the specified frames.

Add browsertests for the same.

Bug: 1148878
Change-Id: I8ba90348a9a55e08972576aeb6ff477a5b39d9b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2533412
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: David Bertoni <dbertoni@chromium.org>
Reviewed-by: Kelvin Jiang <kelvinjiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#828885}

chrome/browser/extensions/api/scripting/scripting_api.cc

@@ -70,7 +70,7 @@ void ExecuteScript(ScriptExecutor* script_executor,
                    ScriptExecutor::ScriptFinishedCallback callback) {
   script_executor->ExecuteScript(
       HostID(HostID::EXTENSIONS, extension.id()), UserScript::ADD_JAVASCRIPT,
-      code, frame_scope, ExtensionApiFrameIdMap::kTopFrameId,
+      code, frame_scope, {ExtensionApiFrameIdMap::kTopFrameId},
       ScriptExecutor::MATCH_ABOUT_BLANK, UserScript::DOCUMENT_IDLE,
       ScriptExecutor::DEFAULT_PROCESS,
       /* webview_src */ GURL(), /* script_url */ GURL(), user_gesture,
@@ -105,7 +105,7 @@ bool CanAccessTarget(const PermissionsData& permissions,
   ScriptExecutor::FrameScope frame_scope =
       target.all_frames && *target.all_frames == true
           ? ScriptExecutor::INCLUDE_SUB_FRAMES
-          : ScriptExecutor::SINGLE_FRAME;
+          : ScriptExecutor::SPECIFIED_FRAMES;
 
   // TODO(devlin): It'd be best to do all the permission checks for the frames
   // on the browser side, including child frames. Today, we only check the
@@ -140,7 +140,7 @@ ExtensionFunction::ResponseAction ScriptingExecuteScriptFunction::Run() {
 
   std::string error;
   ScriptExecutor* script_executor = nullptr;
-  ScriptExecutor::FrameScope frame_scope = ScriptExecutor::SINGLE_FRAME;
+  ScriptExecutor::FrameScope frame_scope = ScriptExecutor::SPECIFIED_FRAMES;
   if (!CanAccessTarget(*extension()->permissions_data(), injection.target,
                        browser_context(), include_incognito_information(),
                        &script_executor, &frame_scope, &error)) {
@@ -201,7 +201,7 @@ ExtensionFunction::ResponseAction ScriptingInsertCSSFunction::Run() {
 
   std::string error;
   ScriptExecutor* script_executor = nullptr;
-  ScriptExecutor::FrameScope frame_scope = ScriptExecutor::SINGLE_FRAME;
+  ScriptExecutor::FrameScope frame_scope = ScriptExecutor::SPECIFIED_FRAMES;
   if (!CanAccessTarget(*extension()->permissions_data(), injection.target,
                        browser_context(), include_incognito_information(),
                        &script_executor, &frame_scope, &error)) {
@@ -230,7 +230,7 @@ ExtensionFunction::ResponseAction ScriptingInsertCSSFunction::Run() {
   constexpr UserScript::RunLocation kRunLocation = UserScript::DOCUMENT_START;
   script_executor->ExecuteScript(
       HostID(HostID::EXTENSIONS, extension()->id()), UserScript::ADD_CSS,
-      *injection.css, frame_scope, ExtensionApiFrameIdMap::kTopFrameId,
+      *injection.css, frame_scope, {ExtensionApiFrameIdMap::kTopFrameId},
       ScriptExecutor::MATCH_ABOUT_BLANK, kRunLocation,
       ScriptExecutor::DEFAULT_PROCESS,
       /* webview_src */ GURL(), /* script_url */ GURL(), user_gesture(), origin,

chrome/browser/extensions/script_executor_browsertest.cc

@@ -4,6 +4,7 @@
 
 #include <string>
 
+#include "base/json/json_writer.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/test/bind.h"
 #include "base/values.h"
@@ -21,10 +22,55 @@
 #include "extensions/common/extension_builder.h"
 #include "extensions/common/user_script.h"
 #include "net/dns/mock_host_resolver.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
 namespace extensions {
 
+namespace {
+
+// A helper object to wait for and collect the results from a script execution.
+class ScriptExecutorHelper {
+ public:
+  ScriptExecutorHelper() = default;
+  ScriptExecutorHelper(const ScriptExecutorHelper&) = delete;
+  ScriptExecutorHelper& operator=(const ScriptExecutorHelper&) = delete;
+  ~ScriptExecutorHelper() = default;
+
+  void Wait() { run_loop_.Run(); }
+
+  ScriptExecutor::ScriptFinishedCallback GetCallback() {
+    // The Unretained() is safe because this object is always supposed to
+    // outlive the script execution.
+    return base::BindOnce(&ScriptExecutorHelper::OnScriptFinished,
+                          base::Unretained(this));
+  }
+
+  const std::string& error() const { return error_; }
+  const GURL& url() const { return url_; }
+  const base::Value& result() const { return result_; }
+
+ private:
+  void OnScriptFinished(const std::string& error,
+                        const GURL& url,
+                        const base::ListValue& result) {
+    error_ = error;
+    url_ = url;
+    result_ = result.Clone();
+    run_loop_.Quit();
+  }
+
+  // Note: we include an initial value for the error so that asserting it's
+  // empty is meaningful.
+  std::string error_{"<initial error>"};
+  GURL url_;
+  base::Value result_;
+  base::RunLoop run_loop_;
+};
+
+}  // namespace
+
 class ScriptExecutorBrowserTest : public ExtensionBrowserTest {
  public:
   ScriptExecutorBrowserTest() = default;
@@ -48,6 +94,13 @@ class ScriptExecutorBrowserTest : public ExtensionBrowserTest {
     host_resolver()->AddRule("*", "127.0.0.1");
     ASSERT_TRUE(embedded_test_server()->Start());
   }
+
+  // Returns the frame with the given `name` from `web_contents`.
+  content::RenderFrameHost* GetFrameByName(content::WebContents* web_contents,
+                                           const std::string& name) {
+    return content::FrameMatchingPredicate(
+        web_contents, base::Bind(&content::FrameMatchesName, name));
+  }
 };
 
 IN_PROC_BROWSER_TEST_F(ScriptExecutorBrowserTest, MainFrameExecution) {
@@ -76,35 +129,115 @@ IN_PROC_BROWSER_TEST_F(ScriptExecutorBrowserTest, MainFrameExecution) {
          oldTitle;
         )";
 
-  std::string execution_error("<initial error>");
-  GURL execution_url;
-  base::Value execution_result;
-  base::RunLoop run_loop;
-  auto script_finished = [&execution_error, &execution_url, &execution_result,
-                          &run_loop](const std::string& error, const GURL& url,
-                                     const base::ListValue& value) {
-    execution_error = error;
-    execution_url = url;
-    execution_result = value.Clone();
-    run_loop.Quit();
-  };
-
+  ScriptExecutorHelper helper;
   script_executor.ExecuteScript(
       HostID(HostID::EXTENSIONS, extension->id()), UserScript::ADD_JAVASCRIPT,
-      kCode, ScriptExecutor::SINGLE_FRAME, ExtensionApiFrameIdMap::kTopFrameId,
+      kCode, ScriptExecutor::SPECIFIED_FRAMES,
+      {ExtensionApiFrameIdMap::kTopFrameId},
       ScriptExecutor::DONT_MATCH_ABOUT_BLANK, UserScript::DOCUMENT_IDLE,
       ScriptExecutor::DEFAULT_PROCESS, GURL() /* webview_src */,
       GURL() /* script_url */, false /* user_gesture */,
       base::nullopt /* css_origin */, ScriptExecutor::JSON_SERIALIZED_RESULT,
-      base::BindLambdaForTesting(script_finished));
-  run_loop.Run();
+      helper.GetCallback());
+  helper.Wait();
   EXPECT_EQ("New Title", base::UTF16ToUTF8(web_contents->GetTitle()));
-  EXPECT_EQ(web_contents->GetLastCommittedURL(), execution_url);
-  EXPECT_EQ("", execution_error);
+  EXPECT_EQ(web_contents->GetLastCommittedURL(), helper.url());
+  EXPECT_EQ("", helper.error());
 
   base::Value expected(base::Value::Type::LIST);
   expected.Append("OK");
-  EXPECT_EQ(expected, execution_result);
+  EXPECT_EQ(expected, helper.result());
+}
+
+// Tests script execution into a specified set of frames.
+IN_PROC_BROWSER_TEST_F(ScriptExecutorBrowserTest, SpecifiedFrames) {
+  const Extension* extension =
+      LoadExtensionWithHostPermission("http://example.com/*");
+
+  GURL example_com = embedded_test_server()->GetURL(
+      "example.com", "/extensions/iframes/main.html");
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(web_contents);
+
+  {
+    content::TestNavigationObserver nav_observer(web_contents);
+    ui_test_utils::NavigateToURL(browser(), example_com);
+    nav_observer.Wait();
+    EXPECT_TRUE(nav_observer.last_navigation_succeeded());
+  }
+
+  // Note: The frame hierarchy for main.html looks like:
+  // main
+  //   frame1
+  //   frame2
+  //     frame2_child
+  //   frame3
+  content::RenderFrameHost* frame1 = GetFrameByName(web_contents, "frame1");
+  ASSERT_TRUE(frame1);
+  content::RenderFrameHost* frame2 = GetFrameByName(web_contents, "frame2");
+  ASSERT_TRUE(frame2);
+  content::RenderFrameHost* frame3 = GetFrameByName(web_contents, "frame3");
+  ASSERT_TRUE(frame3);
+  content::RenderFrameHost* frame2_child =
+      GetFrameByName(web_contents, "frame2_child");
+  ASSERT_TRUE(frame2_child);
+
+  ScriptExecutor script_executor(web_contents);
+  // Note: Since other tests verify the code's effects, here we just rely on the
+  // execution result as an indication that it ran.
+  constexpr char kCode[] = "document.title;";
+
+  std::vector<int> frame_ids = {ExtensionApiFrameIdMap::GetFrameId(frame1),
+                                ExtensionApiFrameIdMap::GetFrameId(frame2)};
+
+  {
+    // Execute in frames 1 and 2. These are the only frames for which we should
+    // get a result.
+    ScriptExecutorHelper helper;
+    script_executor.ExecuteScript(
+        HostID(HostID::EXTENSIONS, extension->id()), UserScript::ADD_JAVASCRIPT,
+        kCode, ScriptExecutor::SPECIFIED_FRAMES, frame_ids,
+        ScriptExecutor::DONT_MATCH_ABOUT_BLANK, UserScript::DOCUMENT_IDLE,
+        ScriptExecutor::DEFAULT_PROCESS, GURL() /* webview_src */,
+        GURL() /* script_url */, false /* user_gesture */,
+        base::nullopt /* css_origin */, ScriptExecutor::JSON_SERIALIZED_RESULT,
+        helper.GetCallback());
+    helper.Wait();
+    EXPECT_EQ("", helper.error());
+
+    base::Value frame1_result("Frame 1");
+    base::Value frame2_result("Frame 2");
+    EXPECT_THAT(
+        helper.result().GetList(),
+        testing::UnorderedElementsAre(::testing::Eq(std::cref(frame1_result)),
+                                      ::testing::Eq(std::cref(frame2_result))));
+  }
+
+  {
+    // Repeat the execution in frames 1 and 2, but include subframes. This
+    // should result in frame2_child being added to the results.
+    ScriptExecutorHelper helper;
+    script_executor.ExecuteScript(
+        HostID(HostID::EXTENSIONS, extension->id()), UserScript::ADD_JAVASCRIPT,
+        kCode, ScriptExecutor::INCLUDE_SUB_FRAMES, frame_ids,
+        ScriptExecutor::DONT_MATCH_ABOUT_BLANK, UserScript::DOCUMENT_IDLE,
+        ScriptExecutor::DEFAULT_PROCESS, GURL() /* webview_src */,
+        GURL() /* script_url */, false /* user_gesture */,
+        base::nullopt /* css_origin */, ScriptExecutor::JSON_SERIALIZED_RESULT,
+        helper.GetCallback());
+    helper.Wait();
+    EXPECT_EQ("", helper.error());
+
+    base::Value frame1_result("Frame 1");
+    base::Value frame2_result("Frame 2");
+    base::Value frame2_child_result("Frame 2 Child");
+    EXPECT_THAT(helper.result().GetList(),
+                testing::UnorderedElementsAre(
+                    ::testing::Eq(std::cref(frame1_result)),
+                    ::testing::Eq(std::cref(frame2_result)),
+                    ::testing::Eq(std::cref(frame2_child_result))));
+  }
 }
 
 }  // namespace extensions

chrome/test/data/extensions/iframes/frame1.html

+<!doctype html>
+<html>
+  <title>Frame 1</title>
+</html>

chrome/test/data/extensions/iframes/frame2.html

+<!doctype html>
+<html>
+  <title>Frame 2</title>
+  <iframe name="frame2_child" src="frame2_child.html"></iframe>
+</html>

chrome/test/data/extensions/iframes/frame2_child.html

+<!doctype html>
+<html>
+  <title>Frame 2 Child</title>
+</html>

chrome/test/data/extensions/iframes/frame3.html

+<!doctype html>
+<html>
+  <title>Frame 3</title>
+</html>

chrome/test/data/extensions/iframes/main.html

+<!doctype html>
+<html>
+  <title>Main</title>
+  <body>
+    <iframe name="frame1" src="frame1.html"></iframe>
+    <iframe name="frame2" src="frame2.html"></iframe>
+    <iframe name="frame3" src="frame3.html"></iframe>
+  </body>
+</html>

extensions/browser/api/execute_code_function.cc

@@ -86,7 +86,7 @@ bool ExecuteCodeFunction::Execute(const std::string& code_string,
   ScriptExecutor::FrameScope frame_scope =
       details_->all_frames.get() && *details_->all_frames
           ? ScriptExecutor::INCLUDE_SUB_FRAMES
-          : ScriptExecutor::SINGLE_FRAME;
+          : ScriptExecutor::SPECIFIED_FRAMES;
 
   int frame_id = details_->frame_id.get() ? *details_->frame_id
                                           : ExtensionApiFrameIdMap::kTopFrameId;
@@ -118,7 +118,7 @@ bool ExecuteCodeFunction::Execute(const std::string& code_string,
     css_origin = CSS_ORIGIN_AUTHOR;
 
   executor->ExecuteScript(
-      host_id_, action_type, code_string, frame_scope, frame_id,
+      host_id_, action_type, code_string, frame_scope, {frame_id},
       match_about_blank, run_at,
       IsWebView() ? ScriptExecutor::WEB_VIEW_PROCESS
                   : ScriptExecutor::DEFAULT_PROCESS,

extensions/browser/script_executor.cc

@@ -59,26 +59,54 @@ class Handler : public content::WebContentsObserver {
           content::WebContents* web_contents,
           const ExtensionMsg_ExecuteCode_Params& params,
           ScriptExecutor::FrameScope scope,
-          int frame_id,
+          const std::vector<int>& frame_ids,
           ScriptExecutor::ScriptFinishedCallback callback)
       : content::WebContentsObserver(web_contents),
         observer_(std::move(observer)),
         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_(std::move(callback)) {
-    if (root_rfh_) {
-      if (include_sub_frames_) {
-        web_contents->ForEachFrame(base::BindRepeating(
-            &Handler::SendExecuteCode, base::Unretained(this), params));
-      } else {
-        SendExecuteCode(params, root_rfh_);
-      }
+    for (int frame_id : frame_ids) {
+      content::RenderFrameHost* frame =
+          ExtensionApiFrameIdMap::GetRenderFrameHostById(web_contents,
+                                                         frame_id);
+      DCHECK(!base::Contains(pending_render_frames_, frame));
+      if (frame->IsRenderFrameLive())
+        pending_render_frames_.push_back(frame);
     }
 
+    // If there is a single frame specified (and it was valid), we consider it
+    // the "root" frame, which is used in result ordering and error collection.
+    if (frame_ids.size() == 1 && pending_render_frames_.size() == 1) {
+      root_rfh_ = pending_render_frames_[0];
+      root_is_main_frame_ = root_rfh_->GetParent() == nullptr;
+    }
+
+    // If we are to include subframes, iterate over all frames in the
+    // WebContents and add them iff they are a child of an included frame.
+    if (scope == ScriptExecutor::INCLUDE_SUB_FRAMES) {
+      auto check_frame =
+          [](std::vector<content::RenderFrameHost*>* pending_frames,
+             content::RenderFrameHost* frame) {
+            if (!frame->IsRenderFrameLive() ||
+                base::Contains(*pending_frames, frame)) {
+              return;
+            }
+
+            for (auto* pending_frame : *pending_frames) {
+              if (frame->IsDescendantOf(pending_frame)) {
+                pending_frames->push_back(frame);
+                break;
+              }
+            }
+          };
+      web_contents->ForEachFrame(
+          base::BindRepeating(check_frame, &pending_render_frames_));
+    }
+
+    for (content::RenderFrameHost* frame : pending_render_frames_)
+      SendExecuteCode(params, frame);
+
     if (pending_render_frames_.empty())
       Finish();
   }
@@ -113,7 +141,7 @@ class Handler : public content::WebContentsObserver {
 
   void RenderFrameDeleted(
       content::RenderFrameHost* render_frame_host) override {
-    if (pending_render_frames_.erase(render_frame_host) == 1 &&
+    if (base::Erase(pending_render_frames_, render_frame_host) == 1 &&
         pending_render_frames_.empty()) {
       Finish();
     }
@@ -123,26 +151,12 @@ 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);
+    DCHECK(frame->IsRenderFrameLive());
+    DCHECK(base::Contains(pending_render_frames_, frame));
     URLLoaderFactoryManager::WillExecuteCode(frame, host_id_);
     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,
@@ -151,8 +165,8 @@ class Handler : public content::WebContentsObserver {
                              const base::ListValue& result_list) {
     DCHECK_EQ(request_id_, request_id);
     DCHECK(!pending_render_frames_.empty());
-    bool erased = pending_render_frames_.erase(render_frame_host) == 1;
-    DCHECK(erased);
+    size_t erased = base::Erase(pending_render_frames_, render_frame_host);
+    DCHECK_EQ(1u, erased);
     bool is_root_frame = root_rfh_ == render_frame_host;
 
     // Set the result, if there is one.
@@ -177,7 +191,7 @@ class Handler : public content::WebContentsObserver {
   }
 
   void Finish() {
-    if (root_frame_url_.is_empty()) {
+    if (root_rfh_ && root_frame_url_.is_empty()) {
       // We never finished the root frame injection.
       root_frame_error_ =
           root_is_main_frame_ ? kRendererDestroyed : kFrameRemoved;
@@ -201,20 +215,23 @@ class Handler : public content::WebContentsObserver {
   HostID host_id_;
 
   // 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_;
+  int request_id_ = 0;
 
-  // The frame (and optionally its descendant frames) where the injection will
-  // occur.
-  content::RenderFrameHost* root_rfh_;
+  // The primary frame of the injection, if only a single frame is explicitly
+  // specified.
+  content::RenderFrameHost* root_rfh_ = nullptr;
 
   // Whether |root_rfh_| is the main frame of a tab.
-  bool root_is_main_frame_;
+  bool root_is_main_frame_ = false;
 
-  // The hosts of the still-running injections.
-  std::set<content::RenderFrameHost*> pending_render_frames_;
+  // The hosts of the still-running injections. Note: this is a vector because
+  // order matters (some tests - and therefore perhaps some extensions - rely on
+  // the execution mirroring the frame tree hierarchy). The contents, however,
+  // should be unique (i.e., no duplicated frames).
+  // TODO(devlin): Extensions *shouldn't* rely on order here, because there's
+  // never a guarantee. We should probably just adjust the test and disregard
+  // order (except the root frame).
+  std::vector<content::RenderFrameHost*> pending_render_frames_;
 
   // The results of the injection.
   base::ListValue results_;
@@ -244,7 +261,7 @@ void ScriptExecutor::ExecuteScript(const HostID& host_id,
                                    UserScript::ActionType action_type,
                                    const std::string& code,
                                    ScriptExecutor::FrameScope frame_scope,
-                                   int frame_id,
+                                   const std::vector<int>& frame_ids,
                                    ScriptExecutor::MatchAboutBlank about_blank,
                                    UserScript::RunLocation run_at,
                                    ScriptExecutor::ProcessType process_type,
@@ -287,7 +304,7 @@ void ScriptExecutor::ExecuteScript(const HostID& host_id,
     params.injection_key = GenerateInjectionKey(host_id, script_url, code);
 
   // Handler handles IPCs and deletes itself on completion.
-  new Handler(observer_, web_contents_, params, frame_scope, frame_id,
+  new Handler(observer_, web_contents_, params, frame_scope, frame_ids,
               std::move(callback));
 }
 

extensions/browser/script_executor.h

@@ -8,6 +8,7 @@
 #include <map>
 #include <set>
 #include <string>
+#include <vector>
 
 #include "base/callback.h"
 #include "base/optional.h"
@@ -48,7 +49,7 @@ class ScriptExecutor {
 
   // The scope of the script injection across the frames.
   enum FrameScope {
-    SINGLE_FRAME,
+    SPECIFIED_FRAMES,
     INCLUDE_SUB_FRAMES,
   };
 
@@ -79,9 +80,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.
+  // The script will be executed in the frames identified by |frame_ids| (which
+  // are extension API frame IDs). If |frame_scope| is INCLUDE_SUB_FRAMES,
+  // then the script will also be executed in all descendants of the specified
+  // frames.
   //
   // |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
@@ -90,7 +92,7 @@ class ScriptExecutor {
                      UserScript::ActionType action_type,
                      const std::string& code,
                      FrameScope frame_scope,
-                     int frame_id,
+                     const std::vector<int>& frame_ids,
                      MatchAboutBlank match_about_blank,
                      UserScript::RunLocation run_at,
                      ProcessType process_type,