Fix callback for chrome.tabs.update with javascript URLs.

UpdateTabFunction is asynchronous when updating a tab with a javascript URL. In this situation, the method was returning early before generating the callback result. This fixes the ExecuteScriptApiTest.NavigationRaceJavaScriptUrl test.

BUG=89731
TEST=ExecuteScriptApiTest.NavigationRaceJavaScriptUrl

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

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

5 files changed, 62 insertions, 42 deletions

diff --git a/chrome/browser/extensions/execute_script_apitest.cc b/chrome/browser/extensions/execute_script_apitest.cc
index ee8db03..3a5e92a 100644
--- a/chrome/browser/extensions/execute_script_apitest.cc
+++ b/chrome/browser/extensions/execute_script_apitest.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -77,16 +77,7 @@ IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest, NavigationRaceExecuteScript) {
                                   "execute_script.html")) << message_;
 }
 
-
-#if defined(OS_LINUX)
-// Fails on Linux.  http://crbug.com/89731
-#define MAYBE_NavigationRaceJavaScriptUrl DISABLED_NavigationRaceJavaScriptUrl
-#else
-#define MAYBE_NavigationRaceJavaScriptUrl NavigationRaceJavaScriptUrl
-#endif
-
-IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest,
-                       MAYBE_NavigationRaceJavaScriptUrl) {
+IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest, NavigationRaceJavaScriptURL) {
   host_resolver()->AddRule("a.com", "127.0.0.1");
   host_resolver()->AddRule("b.com", "127.0.0.1");
   ASSERT_TRUE(StartTestServer());
diff --git a/chrome/browser/extensions/extension_javascript_url_apitest.cc b/chrome/browser/extensions/extension_javascript_url_apitest.cc
index d7ea29e..8a845e3 100644
--- a/chrome/browser/extensions/extension_javascript_url_apitest.cc
+++ b/chrome/browser/extensions/extension_javascript_url_apitest.cc
@@ -1,12 +1,12 @@
-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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 "chrome/browser/extensions/extension_apitest.h"
 #include "net/base/mock_host_resolver.h"
 
-// Disabled, http://crbug.com/63589.
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_JavaScriptURLPermissions) {
+// If crashing, mark disabled and update http://crbug.com/63589.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, JavaScriptURLPermissions) {
   host_resolver()->AddRule("a.com", "127.0.0.1");
   host_resolver()->AddRule("b.com", "127.0.0.1");
   ASSERT_TRUE(StartTestServer());
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index f4eb3ad..f737d37 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -1058,7 +1058,7 @@ bool HighlightTabsFunction::RunImpl() {
   return true;
 }
 
-UpdateTabFunction::UpdateTabFunction() {
+UpdateTabFunction::UpdateTabFunction() : web_contents_(NULL) {
 }
 
 bool UpdateTabFunction::RunImpl() {
@@ -1094,12 +1094,17 @@ bool UpdateTabFunction::RunImpl() {
                   NULL, &tab_strip, &contents, &tab_index, &error_)) {
     return false;
   }
-  NavigationController& controller = contents->web_contents()->GetController();
+
+  web_contents_ = contents->web_contents();
+  NavigationController& controller = web_contents_->GetController();
 
   // TODO(rafaelw): handle setting remaining tab properties:
   // -title
   // -favIconUrl
 
+  // We wait to fire the callback when executing 'javascript:' URLs in tabs.
+  bool is_async = false;
+
   // Navigate the tab to a new location if the url is different.
   std::string url_string;
   if (update_props->HasKey(keys::kUrlKey)) {
@@ -1123,7 +1128,7 @@ bool UpdateTabFunction::RunImpl() {
     // we need to check host permissions before allowing them.
     if (url.SchemeIs(chrome::kJavaScriptScheme)) {
       if (!GetExtension()->CanExecuteScriptOnPage(
-              contents->web_contents()->GetURL(), NULL, &error_)) {
+              web_contents_->GetURL(), NULL, &error_)) {
         return false;
       }
 
@@ -1135,16 +1140,15 @@ bool UpdateTabFunction::RunImpl() {
       params.all_frames = false;
       params.in_main_world = true;
 
-      RenderViewHost* render_view_host =
-          contents->web_contents()->GetRenderViewHost();
+      RenderViewHost* render_view_host = web_contents_->GetRenderViewHost();
       render_view_host->Send(
           new ExtensionMsg_ExecuteCode(render_view_host->routing_id(),
                                        params));
 
-      Observe(contents->web_contents());
-      AddRef();  // balanced in Observe()
+      Observe(web_contents_);
+      AddRef();  // Balanced in OnExecuteCodeFinished().
 
-      return true;
+      is_async = true;
     }
 
     controller.LoadURL(
@@ -1153,7 +1157,7 @@ bool UpdateTabFunction::RunImpl() {
     // The URL of a tab contents never actually changes to a JavaScript URL, so
     // this check only makes sense in other cases.
     if (!url.SchemeIs(chrome::kJavaScriptScheme))
-      DCHECK_EQ(url.spec(), contents->web_contents()->GetURL().spec());
+      DCHECK_EQ(url.spec(), web_contents_->GetURL().spec());
   }
 
   bool active = false;
@@ -1173,7 +1177,7 @@ bool UpdateTabFunction::RunImpl() {
       tab_strip->ActivateTabAt(tab_index, false);
       DCHECK_EQ(contents, tab_strip->GetActiveTabContents());
     }
-    contents->web_contents()->Focus();
+    web_contents_->Focus();
   }
 
   if (update_props->HasKey(keys::kHighlightedKey)) {
@@ -1209,20 +1213,30 @@ bool UpdateTabFunction::RunImpl() {
         tab_index, &opener_contents->web_contents()->GetController());
   }
 
-  if (has_callback()) {
-    if (GetExtension()->HasAPIPermission(ExtensionAPIPermission::kTab)) {
-      result_.reset(ExtensionTabUtil::CreateTabValue(contents->web_contents(),
-                                                     tab_strip,
-                                                     tab_index));
-    } else {
-      result_.reset(Value::CreateNullValue());
-    }
+  if (!is_async) {
+    PopulateResult();
+    SendResponse(true);
   }
-
-  SendResponse(true);
   return true;
 }
 
+void UpdateTabFunction::PopulateResult() {
+  if (!has_callback())
+    return;
+
+  if (GetExtension()->HasAPIPermission(ExtensionAPIPermission::kTab) &&
+      web_contents_ != NULL) {
+    result_.reset(ExtensionTabUtil::CreateTabValue(web_contents_));
+  } else {
+    result_.reset(Value::CreateNullValue());
+  }
+}
+
+void UpdateTabFunction::WebContentsDestroyed(WebContents* tab) {
+  CHECK_EQ(tab, web_contents_);
+  web_contents_ = NULL;
+}
+
 bool UpdateTabFunction::OnMessageReceived(const IPC::Message& message) {
   if (message.type() != ExtensionHostMsg_ExecuteCodeFinished::ID)
     return false;
@@ -1252,10 +1266,12 @@ void UpdateTabFunction::OnExecuteCodeFinished(int request_id,
     error_ = error;
   }
 
+  if (success)
+    PopulateResult();
   SendResponse(success);
 
   Observe(NULL);
-  Release();  // balanced in Execute()
+  Release();  // Balanced in RunImpl().
 }
 
 bool MoveTabsFunction::RunImpl() {
diff --git a/chrome/browser/extensions/extension_tabs_module.h b/chrome/browser/extensions/extension_tabs_module.h
index 944de1c..61f2c39 100644
--- a/chrome/browser/extensions/extension_tabs_module.h
+++ b/chrome/browser/extensions/extension_tabs_module.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
@@ -18,11 +18,12 @@
 
 class BackingStore;
 class SkBitmap;
-
 namespace base {
 class DictionaryValue;
 }  // namespace base
-
+namespace content {
+class WebContents;
+}  // namespace content
 // Windows
 class GetWindowFunction : public SyncExtensionFunction {
   virtual ~GetWindowFunction() {}
@@ -112,10 +113,14 @@ class UpdateTabFunction : public AsyncExtensionFunction,
  private:
   virtual ~UpdateTabFunction() {}
   virtual bool RunImpl() OVERRIDE;
+  virtual void WebContentsDestroyed(content::WebContents* tab) OVERRIDE;
   virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
   void OnExecuteCodeFinished(int request_id,
                              bool success,
                              const std::string& error);
+  void PopulateResult();
+
+  content::WebContents* web_contents_;
   DECLARE_EXTENSION_FUNCTION_NAME("tabs.update")
 };
 class MoveTabsFunction : public SyncExtensionFunction {
diff --git a/chrome/test/data/extensions/api_test/tabs/javascript_url_permissions/test.js b/chrome/test/data/extensions/api_test/tabs/javascript_url_permissions/test.js
index d35d1b1..104dc2b 100644
--- a/chrome/test/data/extensions/api_test/tabs/javascript_url_permissions/test.js
+++ b/chrome/test/data/extensions/api_test/tabs/javascript_url_permissions/test.js
@@ -1,9 +1,12 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 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.
 
+var assertEq = chrome.test.assertEq;
+var pass = chrome.test.callbackPass;
+
 chrome.test.getConfig(function(config) {
-  var javaScriptURL = "javascript:void(document.body.bgColor='red')";
+  var javaScriptURL = "javascript:void(document.title='js-url-success')";
 
   var fixPort = function(url) {
     return url.replace(/PORT/, config.testServer.port);
@@ -26,8 +29,13 @@ chrome.test.getConfig(function(config) {
         },
 
         function javaScriptURLShouldSucceed() {
-          chrome.tabs.update(secondTabId, {url: javaScriptURL},
-              chrome.test.callbackPass());
+          chrome.tabs.update(
+              secondTabId,
+              {url: javaScriptURL},
+              pass(function(tab) {
+            assertEq(secondTabId, tab.id);
+            assertEq('js-url-success', tab.title);
+          }));
         }
       ]);
     });