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

chrome/browser/extensions/extension_tabs_module.cc

Index: chrome/browser/extensions/extension_tabs_module.cc
diff --git a/chrome/browser/extensions/extension_tabs_module.cc b/chrome/browser/extensions/extension_tabs_module.cc
index 6333a30d3b7664f6528896f9c89614009ad865ff..aca7a3344e1a6329b5141f8aae0d8d9576be59fc 100644
--- a/chrome/browser/extensions/extension_tabs_module.cc
+++ b/chrome/browser/extensions/extension_tabs_module.cc
@@ -1031,7 +1031,10 @@ bool HighlightTabsFunction::RunImpl() {
   return true;
 }
 
-UpdateTabFunction::UpdateTabFunction() {
+UpdateTabFunction::UpdateTabFunction()
+    : web_contents_(NULL),
+      tab_strip_(NULL),
+      tab_index_(-1) {
 }
 
 bool UpdateTabFunction::RunImpl() {
@@ -1061,18 +1064,21 @@ bool UpdateTabFunction::RunImpl() {
     EXTENSION_FUNCTION_VALIDATE(tab_value->GetAsInteger(&tab_id));
   }
 
-  int tab_index = -1;
-  TabStripModel* tab_strip = NULL;
   if (!GetTabById(tab_id, profile(), include_incognito(),
-                  NULL, &tab_strip, &contents, &tab_index, &error_)) {
+                  NULL, &tab_strip_, &contents, &tab_index_, &error_)) {
     return false;
   }
-  NavigationController& controller = contents->web_contents()->GetController();
+
+  web_contents_ = contents->web_contents();

[Mihai Parparita -not on Chrome, 2012/01/27 23:03:20]

Is it safe to hold on to this pointer? I'm thinking of scenarios where the tab
gets closed by the time ExecutedCodeFinished returns. You may want to consider
overriding WebContentsDestroyed to null out web_contents_ and check for that in
PopulateResponse.

[jstritar, 2012/01/30 16:28:05]

Done. I also updated it so we don't need to hold on to the tab_strip_ pointer by
using the ExtensionTabUtil::CreateTabValue(contents) method.

+  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)) {
@@ -1096,7 +1102,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;
       }
 
@@ -1108,16 +1114,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(
@@ -1126,7 +1131,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;
@@ -1142,45 +1147,50 @@ bool UpdateTabFunction::RunImpl() {
         keys::kActiveKey, &active));
 
   if (active) {
-    if (tab_strip->active_index() != tab_index) {
-      tab_strip->ActivateTabAt(tab_index, false);
-      DCHECK_EQ(contents, tab_strip->GetActiveTabContents());
+    if (tab_strip_->active_index() != tab_index_) {
+      tab_strip_->ActivateTabAt(tab_index_, false);
+      DCHECK_EQ(contents, tab_strip_->GetActiveTabContents());
     }
-    contents->web_contents()->Focus();
+    web_contents_->Focus();
   }
 
   bool highlighted = false;
   if (update_props->HasKey(keys::kHighlightedKey)) {
     EXTENSION_FUNCTION_VALIDATE(update_props->GetBoolean(
         keys::kHighlightedKey, &highlighted));
-    if (highlighted != tab_strip->IsTabSelected(tab_index))
-      tab_strip->ToggleSelectionAt(tab_index);
+    if (highlighted != tab_strip_->IsTabSelected(tab_index_))
+      tab_strip_->ToggleSelectionAt(tab_index_);
   }
 
   bool pinned = false;
   if (update_props->HasKey(keys::kPinnedKey)) {
     EXTENSION_FUNCTION_VALIDATE(update_props->GetBoolean(keys::kPinnedKey,
                                                          &pinned));
-    tab_strip->SetTabPinned(tab_index, pinned);
+    tab_strip_->SetTabPinned(tab_index_, pinned);
 
     // Update the tab index because it may move when being pinned.
-    tab_index = tab_strip->GetIndexOfTabContents(contents);
+    tab_index_ = tab_strip_->GetIndexOfTabContents(contents);
   }
 
-  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)) {
+    result_.reset(ExtensionTabUtil::CreateTabValue(
+        web_contents_, tab_strip_, tab_index_));
+  } else {
+    result_.reset(Value::CreateNullValue());
+  }
+}
+
 bool UpdateTabFunction::OnMessageReceived(const IPC::Message& message) {
   if (message.type() != ExtensionHostMsg_ExecuteCodeFinished::ID)
     return false;
@@ -1210,10 +1220,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() {