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

chrome/browser/extensions/extension_tabs_module.cc

 // 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_tabs_module.h"
 
 #include <algorithm>
 #include <vector>
 
 #include "base/base64.h"
@@ skipping 1013 matching lines @@
     error_ = keys::kNoHighlightedTabError;
     return false;
   }
 
   selection.set_active(active_index);
   browser->tabstrip_model()->SetSelectionFromModel(selection);
   result_.reset(ExtensionTabUtil::CreateWindowValue(browser, true));
   return true;
 }
 
-UpdateTabFunction::UpdateTabFunction() {
+UpdateTabFunction::UpdateTabFunction()
+    : web_contents_(NULL),
+      tab_strip_(NULL),
+      tab_index_(-1) {
 }
 
 bool UpdateTabFunction::RunImpl() {
   DictionaryValue* update_props = NULL;
   EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &update_props));
 
   Value* tab_value = NULL;
   if (HasOptionalArgument(0)) {
     EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &tab_value));
   }
 
   int tab_id = -1;
   TabContentsWrapper* contents = NULL;
   if (tab_value == NULL || tab_value->IsType(Value::TYPE_NULL)) {
     Browser* browser = GetCurrentBrowser();
     if (!browser) {
       error_ = keys::kNoCurrentWindowError;
       return false;
     }
     contents = browser->tabstrip_model()->GetActiveTabContents();
     if (!contents) {
       error_ = keys::kNoSelectedTabError;
       return false;
     }
     tab_id = ExtensionTabUtil::GetTabId(contents->web_contents());
   } else {
     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)) {
     EXTENSION_FUNCTION_VALIDATE(update_props->GetString(
         keys::kUrlKey, &url_string));
     GURL url = ResolvePossiblyRelativeURL(url_string, GetExtension());
 
     if (!url.is_valid()) {
       error_ = ExtensionErrorUtils::FormatErrorMessage(keys::kInvalidUrlError,
                                                        url_string);
       return false;
     }
 
     // Don't let the extension crash the browser or renderers.
     if (IsCrashURL(url)) {
       error_ = keys::kNoCrashBrowserError;
       return false;
     }
 
     // JavaScript URLs can do the same kinds of things as cross-origin XHR, so
     // 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;
       }
 
       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->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(
         url, content::Referrer(), content::PAGE_TRANSITION_LINK, std::string());
 
     // 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;
   // TODO(rafaelw): Setting |active| from js doesn't make much sense.
   // Move tab selection management up to window.
   if (update_props->HasKey(keys::kSelectedKey))
     EXTENSION_FUNCTION_VALIDATE(update_props->GetBoolean(
         keys::kSelectedKey, &active));
 
   // The 'active' property has replaced 'selected'.
   if (update_props->HasKey(keys::kActiveKey))
     EXTENSION_FUNCTION_VALIDATE(update_props->GetBoolean(
         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);
   }
+  return 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;
 
   int message_request_id = -1;
   void* iter = NULL;
   if (!message.ReadInt(&iter, &message_request_id)) {
     NOTREACHED() << "malformed extension message";
@@ skipping 11 matching lines @@
 }
 
 void UpdateTabFunction::OnExecuteCodeFinished(int request_id,
                                               bool success,
                                               const std::string& error) {
   if (!error.empty()) {
     CHECK(!success);
     error_ = error;
   }
 
+  if (success)
+    PopulateResult();
   SendResponse(success);
 
   Observe(NULL);
-  Release();  // balanced in Execute()
+  Release();  // Balanced in RunImpl().
 }
 
 bool MoveTabsFunction::RunImpl() {
   Value* tab_value = NULL;
   EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &tab_value));
 
   std::vector<int> tab_ids;
   EXTENSION_FUNCTION_VALIDATE(ReadOneOrMoreIntegers(tab_value, &tab_ids));
 
   DictionaryValue* update_props = NULL;
@@ skipping 420 matching lines @@
   // called for every API call the extension made.
   GotLanguage(language);
 }
 
 void DetectTabLanguageFunction::GotLanguage(const std::string& language) {
   result_.reset(Value::CreateStringValue(language.c_str()));
   SendResponse(true);
 
   Release();  // Balanced in Run()
 }