Cleanup in handling of JavaScript URLs in tabs API

Reuse infrastructure for chrome.tabs.executeScript() to
implement chrome.tabs.update({url:"javascript:...."}) since
it has built-in safeguards against navigation races.

BUG=77026

Review URL: http://codereview.chromium.org/6771062

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@80826 0039d316-1c4b-4281-b951-d872f2087c98

11 files changed, 202 insertions, 35 deletions

diff --git a/chrome/browser/extensions/execute_code_in_tab_function.cc b/chrome/browser/extensions/execute_code_in_tab_function.cc
index 11b0875..9bebbd9 100644
--- a/chrome/browser/extensions/execute_code_in_tab_function.cc
+++ b/chrome/browser/extensions/execute_code_in_tab_function.cc
@@ -20,6 +20,7 @@
 #include "chrome/common/extensions/extension_messages.h"
 #include "content/browser/renderer_host/render_view_host.h"
 #include "content/browser/tab_contents/tab_contents.h"
+#include "content/browser/renderer_host/render_view_host.h"
 #include "content/common/notification_service.h"
 
 namespace keys = extension_tabs_module_constants;
@@ -77,11 +78,10 @@ bool ExecuteCodeInTabFunction::RunImpl() {
     }
   }
 
-  DCHECK(browser);
-  DCHECK(contents);
-
   // NOTE: This can give the wrong answer due to race conditions, but it is OK,
   // we check again in the renderer.
+  CHECK(browser);
+  CHECK(contents);
   if (!GetExtension()->CanExecuteScriptOnPage(
           contents->tab_contents()->GetURL(), NULL, &error_)) {
     return false;
@@ -175,6 +175,7 @@ bool ExecuteCodeInTabFunction::Execute(const std::string& code_string) {
   params.is_javascript = is_js_code;
   params.code = code_string;
   params.all_frames = all_frames_;
+  params.in_main_world = false;
   contents->render_view_host()->Send(new ExtensionMsg_ExecuteCode(
       contents->render_view_host()->routing_id(), params));
 
@@ -205,7 +206,13 @@ bool ExecuteCodeInTabFunction::OnMessageReceived(const IPC::Message& message) {
 }
 
 void ExecuteCodeInTabFunction::OnExecuteCodeFinished(int request_id,
-                                                     bool success) {
+                                                     bool success,
+                                                     const std::string& error) {
+  if (!error.empty()) {
+    CHECK(!success);
+    error_ = error;
+  }
+
   SendResponse(success);
 
   registrar_.Observe(NULL);
diff --git a/chrome/browser/extensions/execute_code_in_tab_function.h b/chrome/browser/extensions/execute_code_in_tab_function.h
index a1a416a..9d06ce8 100644
--- a/chrome/browser/extensions/execute_code_in_tab_function.h
+++ b/chrome/browser/extensions/execute_code_in_tab_function.h
@@ -26,7 +26,8 @@ class ExecuteCodeInTabFunction : public AsyncExtensionFunction,
   virtual bool OnMessageReceived(const IPC::Message& message);
 
   // Message handler.
-  void OnExecuteCodeFinished(int request_id, bool success);
+  void OnExecuteCodeFinished(int request_id, bool success,
+                             const std::string& error);
 
   // Called when contents from the file whose path is specified in JSON
   // arguments has been loaded.
diff --git a/chrome/browser/extensions/execute_script_apitest.cc b/chrome/browser/extensions/execute_script_apitest.cc
index 8295ca4..c12a4e0 100644
--- a/chrome/browser/extensions/execute_script_apitest.cc
+++ b/chrome/browser/extensions/execute_script_apitest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 
@@ -46,3 +46,13 @@ IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest,
   const char* extension_name = "executescript/fragment";
   ASSERT_TRUE(RunExtensionTest(extension_name)) << message_;
 }
+
+IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest, NavigationRace) {
+  host_resolver()->AddRule("a.com", "127.0.0.1");
+  host_resolver()->AddRule("b.com", "127.0.0.1");
+  ASSERT_TRUE(StartTestServer());
+  ASSERT_TRUE(RunExtensionSubtest("executescript/navigation_race",
+                                  "execute_script.html")) << message_;
+  ASSERT_TRUE(RunExtensionSubtest("executescript/navigation_race",
+                                  "javascript_url.html")) << message_;
+}
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index 9f9e1ba..c575fee 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -27,6 +27,7 @@
 #include "chrome/browser/ui/window_sizer.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_error_utils.h"
+#include "chrome/common/extensions/extension_messages.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/common/url_constants.h"
 #include "content/browser/renderer_host/backing_store.h"
@@ -782,6 +783,10 @@ bool GetCurrentTabFunction::RunImpl() {
   return true;
 }
 
+UpdateTabFunction::UpdateTabFunction()
+    : ALLOW_THIS_IN_INITIALIZER_LIST(registrar_(this)) {
+}
+
 bool UpdateTabFunction::RunImpl() {
   int tab_id;
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &tab_id));
@@ -829,9 +834,24 @@ bool UpdateTabFunction::RunImpl() {
         return false;
       }
 
-      // TODO(aa): How does controller queue URLs? Is there any chance that this
-      // JavaScript URL will end up applying to something other than
-      // controller->GetURL()?
+      ExtensionMsg_ExecuteCode_Params params;
+      params.request_id = request_id();
+      params.extension_id = extension_id();
+      params.is_javascript = true;
+      params.code = url.path();
+      params.all_frames = false;
+      params.in_main_world = true;
+
+      RenderViewHost* render_view_host =
+          contents->tab_contents()->render_view_host();
+      render_view_host->Send(
+          new ExtensionMsg_ExecuteCode(render_view_host->routing_id(),
+                                       params));
+
+      registrar_.Observe(contents->tab_contents());
+      AddRef();  // balanced in Observe()
+
+      return true;
     }
 
     controller.LoadURL(url, GURL(), PageTransition::LINK);
@@ -873,9 +893,45 @@ bool UpdateTabFunction::RunImpl() {
         tab_strip,
         tab_index));
 
+  SendResponse(true);
+  return true;
+}
+
+bool UpdateTabFunction::OnMessageReceived(const IPC::Message& message) {
+  if (message.type() != ExtensionHostMsg_ExecuteCodeFinished::ID)
+    return false;
+
+  int message_request_id;
+  void* iter = NULL;
+  if (!message.ReadInt(&iter, &message_request_id)) {
+    NOTREACHED() << "malformed extension message";
+    return true;
+  }
+
+  if (message_request_id != request_id())
+    return false;
+
+  IPC_BEGIN_MESSAGE_MAP(UpdateTabFunction, message)
+    IPC_MESSAGE_HANDLER(ExtensionHostMsg_ExecuteCodeFinished,
+                        OnExecuteCodeFinished)
+  IPC_END_MESSAGE_MAP()
   return true;
 }
 
+void UpdateTabFunction::OnExecuteCodeFinished(int request_id,
+                                              bool success,
+                                              const std::string& error) {
+  if (!error.empty()) {
+    CHECK(!success);
+    error_ = error;
+  }
+
+  SendResponse(success);
+
+  registrar_.Observe(NULL);
+  Release();  // balanced in Execute()
+}
+
 bool MoveTabFunction::RunImpl() {
   int tab_id;
   EXTENSION_FUNCTION_VALIDATE(args_->GetInteger(0, &tab_id));
diff --git a/chrome/browser/extensions/extension_tabs_module.h b/chrome/browser/extensions/extension_tabs_module.h
index 9e48256..e775505 100644
--- a/chrome/browser/extensions/extension_tabs_module.h
+++ b/chrome/browser/extensions/extension_tabs_module.h
@@ -9,6 +9,7 @@
 #include <string>
 
 #include "chrome/browser/extensions/extension_function.h"
+#include "content/browser/tab_contents/tab_contents_observer.h"
 #include "content/common/notification_observer.h"
 #include "content/common/notification_registrar.h"
 
@@ -111,9 +112,17 @@ class CreateTabFunction : public SyncExtensionFunction {
   virtual bool RunImpl();
   DECLARE_EXTENSION_FUNCTION_NAME("tabs.create")
 };
-class UpdateTabFunction : public SyncExtensionFunction {
+class UpdateTabFunction : public AsyncExtensionFunction,
+                          public TabContentsObserver {
+ public:
+  UpdateTabFunction();
+ private:
   ~UpdateTabFunction() {}
   virtual bool RunImpl();
+  virtual bool OnMessageReceived(const IPC::Message& message);
+  void OnExecuteCodeFinished(int request_id, bool success,
+                             const std::string& error);
+  TabContentsObserver::Registrar registrar_;
   DECLARE_EXTENSION_FUNCTION_NAME("tabs.update")
 };
 class MoveTabFunction : public SyncExtensionFunction {
diff --git a/chrome/common/extensions/extension_messages.h b/chrome/common/extensions/extension_messages.h
index 5a0c4fd..69575e9a 100644
--- a/chrome/common/extensions/extension_messages.h
+++ b/chrome/common/extensions/extension_messages.h
@@ -53,6 +53,10 @@ IPC_STRUCT_BEGIN(ExtensionMsg_ExecuteCode_Params)
 
   // Whether to inject into all frames, or only the root frame.
   IPC_STRUCT_MEMBER(bool, all_frames)
+
+  // Whether to execute code in the main world (as opposed to an isolated
+  // world).
+  IPC_STRUCT_MEMBER(bool, in_main_world)
 IPC_STRUCT_END()
 
 IPC_STRUCT_TRAITS_BEGIN(WebApplicationInfo::IconInfo)
@@ -267,10 +271,10 @@ IPC_SYNC_MESSAGE_CONTROL1_1(ExtensionHostMsg_GetMessageBundle,
                             SubstitutionMap /* message bundle */)
 
 // Send from the renderer to the browser to return the script running result.
-IPC_MESSAGE_ROUTED2(ExtensionHostMsg_ExecuteCodeFinished,
-                    int, /* request id */
-                    bool /* whether the script ran successfully */)
-
+IPC_MESSAGE_ROUTED3(ExtensionHostMsg_ExecuteCodeFinished,
+                    int /* request id */,
+                    bool /* whether the script ran successfully */,
+                    std::string /* error message */)
 
 IPC_MESSAGE_ROUTED2(ExtensionHostMsg_DidGetApplicationInfo,
                     int32 /* page_id */,
diff --git a/chrome/renderer/extensions/user_script_idle_scheduler.cc b/chrome/renderer/extensions/user_script_idle_scheduler.cc
index 92b9c14..f21214f 100644
--- a/chrome/renderer/extensions/user_script_idle_scheduler.cc
+++ b/chrome/renderer/extensions/user_script_idle_scheduler.cc
@@ -5,6 +5,7 @@
 #include "chrome/renderer/extensions/user_script_idle_scheduler.h"
 
 #include "base/message_loop.h"
+#include "chrome/common/extensions/extension_error_utils.h"
 #include "chrome/common/extensions/extension_messages.h"
 #include "chrome/renderer/extension_groups.h"
 #include "chrome/renderer/extensions/extension_dispatcher.h"
@@ -114,7 +115,7 @@ void UserScriptIdleScheduler::OnExecuteCode(
   WebFrame* main_frame = GetMainFrame();
   if (!main_frame) {
     Send(new ExtensionHostMsg_ExecuteCodeFinished(
-        routing_id(), params.request_id, false));
+         routing_id(), params.request_id, false, ""));
     return;
   }
 
@@ -130,6 +131,18 @@ void UserScriptIdleScheduler::OnExecuteCode(
 
 void UserScriptIdleScheduler::ExecuteCodeImpl(
     WebFrame* frame, const ExtensionMsg_ExecuteCode_Params& params) {
+  const Extension* extension =
+      ExtensionDispatcher::Get()->extensions()->GetByID(
+          params.extension_id);
+
+  // Since extension info is sent separately from user script info, they can
+  // be out of sync. We just ignore this situation.
+  if (!extension) {
+    Send(new ExtensionHostMsg_ExecuteCodeFinished(
+        routing_id(), params.request_id, true, ""));
+    return;
+  }
+
   std::vector<WebFrame*> frame_vector;
   frame_vector.push_back(frame);
   if (params.all_frames)
@@ -139,32 +152,47 @@ void UserScriptIdleScheduler::ExecuteCodeImpl(
        frame_it != frame_vector.end(); ++frame_it) {
     WebFrame* frame = *frame_it;
     if (params.is_javascript) {
-      const Extension* extension =
-          ExtensionDispatcher::Get()->extensions()->GetByID(
-              params.extension_id);
-
-    // Since extension info is sent separately from user script info, they can
-    // be out of sync. We just ignore this situation.
-    if (!extension)
-      continue;
-
-      if (!extension->CanExecuteScriptOnPage(frame->url(), NULL, NULL))
-        continue;
-
-      std::vector<WebScriptSource> sources;
-      sources.push_back(
-          WebScriptSource(WebString::fromUTF8(params.code)));
-      UserScriptSlave::InsertInitExtensionCode(&sources, params.extension_id);
-      frame->executeScriptInIsolatedWorld(
-          UserScriptSlave::GetIsolatedWorldId(params.extension_id),
-          &sources.front(), sources.size(), EXTENSION_GROUP_CONTENT_SCRIPTS);
+      // 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 the top frame, we can
+      // immediately send an error and stop because the browser process
+      // considers that an error too.
+      //
+      // For child frames, we just skip ones the extension doesn't have access
+      // to and carry on.
+      if (!extension->CanExecuteScriptOnPage(frame->url(), NULL, NULL)) {
+        if (frame->parent()) {
+          continue;
+        } else {
+          Send(new ExtensionHostMsg_ExecuteCodeFinished(
+              routing_id(), params.request_id, false,
+              ExtensionErrorUtils::FormatErrorMessage(
+                  extension_manifest_errors::kCannotAccessPage,
+                  frame->url().spec())));
+          return;
+        }
+      }
+
+      WebScriptSource source(WebString::fromUTF8(params.code));
+      if (params.in_main_world) {
+        frame->executeScript(source);
+      } else {
+        std::vector<WebScriptSource> sources;
+        sources.push_back(source);
+        UserScriptSlave::InsertInitExtensionCode(&sources, params.extension_id);
+        frame->executeScriptInIsolatedWorld(
+            UserScriptSlave::GetIsolatedWorldId(params.extension_id),
+            &sources.front(), sources.size(), EXTENSION_GROUP_CONTENT_SCRIPTS);
+      }
     } else {
       frame->insertStyleText(WebString::fromUTF8(params.code), WebString());
     }
   }
 
   Send(new ExtensionHostMsg_ExecuteCodeFinished(
-      routing_id(), params.request_id, true));
+      routing_id(), params.request_id, true, ""));
 }
 
 bool UserScriptIdleScheduler::GetAllChildFrames(
diff --git a/chrome/test/data/extensions/api_test/executescript/navigation_race/execute_script.html b/chrome/test/data/extensions/api_test/executescript/navigation_race/execute_script.html
new file mode 100644
index 0000000..777e1b7
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/executescript/navigation_race/execute_script.html
@@ -0,0 +1,8 @@
+<script src="test.js"></script>
+<script>
+function executeCodeInTab(tabId, callback) {
+  chrome.tabs.executeScript(tabId,
+                            {code: "console.log('hi')"},
+                            callback);
+}
+</script>
diff --git a/chrome/test/data/extensions/api_test/executescript/navigation_race/javascript_url.html b/chrome/test/data/extensions/api_test/executescript/navigation_race/javascript_url.html
new file mode 100644
index 0000000..7d2ac2d
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/executescript/navigation_race/javascript_url.html
@@ -0,0 +1,8 @@
+<script src="test.js"></script>
+<script>
+function executeCodeInTab(tabId, callback) {
+  chrome.tabs.update(tabId,
+                     {url: "javascript:console.log('hi')"},
+                     callback);
+}
+</script>
diff --git a/chrome/test/data/extensions/api_test/executescript/navigation_race/manifest.json b/chrome/test/data/extensions/api_test/executescript/navigation_race/manifest.json
new file mode 100644
index 0000000..545b344e
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/executescript/navigation_race/manifest.json
@@ -0,0 +1,8 @@
+{
+  "name": "Navigation race",
+  "version": "1",
+  "permissions": [
+    "tabs",
+    "http://b.com/"
+  ]
+}
diff --git a/chrome/test/data/extensions/api_test/executescript/navigation_race/test.js b/chrome/test/data/extensions/api_test/executescript/navigation_race/test.js
new file mode 100644
index 0000000..d22473a5
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/executescript/navigation_race/test.js
@@ -0,0 +1,28 @@
+// Copyright (c) 2011 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.
+
+chrome.test.getConfig(function(config) {
+  var path = "/files/extensions/test_file.txt";
+  var urlA = "http://a.com:" + config.testServer.port + path;
+  var urlB = "http://b.com:" + config.testServer.port + path;
+  var testTabId;
+
+  function onTabUpdated(tabId, changeInfo, tab) {
+    if (testTabId == tab.id && tab.status == "complete") {
+      chrome.tabs.onUpdated.removeListener(onTabUpdated);
+      chrome.tabs.update(tabId, {url: urlB});
+      executeCodeInTab(testTabId, function() {
+        chrome.test.assertTrue(
+            chrome.extension.lastError.message.indexOf(
+                'Cannot access contents of url "http://a.com:') == 0);
+        chrome.test.notifyPass();
+      });
+    }
+  }
+
+  chrome.tabs.onUpdated.addListener(onTabUpdated);
+  chrome.tabs.create({url: urlA}, function(tab) {
+    testTabId = tab.id;
+  });
+});