Page actions that don't specify an icon (ie.

have a spelling error in the manifest, such as
icon instead of icons/default_icon) caused a
crash when they try to display their icon.

We now check when the extension tries to enable
the page action whether there are any icons to
display. If not, we don't proceed and log an
error to the console.

BUG=25562
TEST=Covered by browser test.
Review URL: http://codereview.chromium.org/316018

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

8 files changed, 91 insertions, 18 deletions

diff --git a/chrome/browser/extensions/extension_browsertests_misc.cc b/chrome/browser/extensions/extension_browsertests_misc.cc
index e991926..9490510 100644
--- a/chrome/browser/extensions/extension_browsertests_misc.cc
+++ b/chrome/browser/extensions/extension_browsertests_misc.cc
@@ -207,6 +207,12 @@ IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, TabContents) {
 #if defined(OS_WIN) || defined(OS_LINUX)
 // Tests that we can load page actions in the Omnibox.
 IN_PROC_BROWSER_TEST_F(ExtensionBrowserTest, PageAction) {
+  // This page action will not show an icon, since it doesn't specify one but
+  // is included here to test for a crash (http://crbug.com/25562).
+  ASSERT_TRUE(LoadExtension(
+      test_data_dir_.AppendASCII("browsertest")
+                    .AppendASCII("crash_25562")));
+
   ASSERT_TRUE(LoadExtension(
       test_data_dir_.AppendASCII("samples")
                     .AppendASCII("subscribe_page_action")));
diff --git a/chrome/browser/extensions/extension_page_actions_module.cc b/chrome/browser/extensions/extension_page_actions_module.cc
index 743e0a5..4738891 100644
--- a/chrome/browser/extensions/extension_page_actions_module.cc
+++ b/chrome/browser/extensions/extension_page_actions_module.cc
@@ -21,12 +21,12 @@ namespace keys = extension_page_actions_module_constants;
 
 namespace {
 // Errors.
-const char kNoExtensionError[] = "No extension with id: *.";
 const char kNoTabError[] = "No tab with id: *.";
 const char kNoPageActionError[] =
     "This extension has no page action specified.";
 const char kUrlNotActiveError[] = "This url is no longer active: *.";
 const char kIconIndexOutOfBounds[] = "Page action icon index out of bounds.";
+const char kNoIconSpecified[] = "Page action has no icons to show.";
 }
 
 // TODO(EXTENSIONS_DEPRECATED): obsolete API.
@@ -56,6 +56,19 @@ bool PageActionFunction::SetPageActionEnabled(bool enable) {
     }
   }
 
+  const ExtensionAction* page_action =
+      dispatcher()->GetExtension()->page_action();
+  if (!page_action) {
+    error_ = kNoPageActionError;
+    return false;
+  }
+
+  if (icon_id < 0 ||
+      static_cast<size_t>(icon_id) >= page_action->icon_paths().size()) {
+    error_ = (icon_id == 0) ? kNoIconSpecified : kIconIndexOutOfBounds;
+    return false;
+  }
+
   // Find the TabContents that contains this tab id.
   TabContents* contents = NULL;
   ExtensionTabUtil::GetTabById(tab_id, profile(), NULL, NULL, &contents, NULL);
@@ -72,22 +85,6 @@ bool PageActionFunction::SetPageActionEnabled(bool enable) {
     return false;
   }
 
-  // Find our extension.
-  Extension* extension = NULL;
-  ExtensionsService* service = profile()->GetExtensionsService();
-  extension = service->GetExtensionById(extension_id());
-  if (!extension) {
-    error_ = ExtensionErrorUtils::FormatErrorMessage(kNoExtensionError,
-                                                     extension_id());
-    return false;
-  }
-
-  const ExtensionAction* page_action = extension->page_action();
-  if (!page_action) {
-    error_ = kNoPageActionError;
-    return false;
-  }
-
   // Set visibility and broadcast notifications that the UI should be updated.
   contents->SetPageActionEnabled(page_action, enable, title, icon_id);
   contents->PageActionStateChanged();
diff --git a/chrome/browser/gtk/location_bar_view_gtk.cc b/chrome/browser/gtk/location_bar_view_gtk.cc
index 8648099..382563a 100644
--- a/chrome/browser/gtk/location_bar_view_gtk.cc
+++ b/chrome/browser/gtk/location_bar_view_gtk.cc
@@ -409,6 +409,15 @@ void LocationBarViewGtk::UpdatePageActions() {
   if (profile_->GetExtensionsService())
       page_actions = profile_->GetExtensionsService()->GetPageActions();
 
+  // Page actions can be created without an icon, so make sure we count only
+  // those that have been given an icon.
+  for (size_t i = 0; i < page_actions.size();) {
+    if (page_actions[i]->icon_paths().empty())
+      page_actions.erase(page_actions.begin() + i);
+    else
+      ++i;
+  }
+
   // Initialize on the first call, or re-inialize if more extensions have been
   // loaded or added after startup.
   if (page_actions.size() != page_action_views_.size()) {
diff --git a/chrome/browser/views/location_bar_view.cc b/chrome/browser/views/location_bar_view.cc
index 9ca58e8b..dec21b8 100644
--- a/chrome/browser/views/location_bar_view.cc
+++ b/chrome/browser/views/location_bar_view.cc
@@ -92,7 +92,7 @@ static std::wstring GetKeywordName(Profile* profile,
 // A container for the PageActionImageView plus its badge.
 class LocationBarView::PageActionWithBadgeView : public views::View {
  public:
-  PageActionWithBadgeView(PageActionImageView* image_view);
+  explicit PageActionWithBadgeView(PageActionImageView* image_view);
 
   PageActionImageView* image_view() { return image_view_; }
 
@@ -698,6 +698,15 @@ void LocationBarView::RefreshPageActionViews() {
   if (profile_->GetExtensionsService())
     page_actions = profile_->GetExtensionsService()->GetPageActions();
 
+  // Page actions can be created without an icon, so make sure we count only
+  // those that have been given an icon.
+  for (size_t i = 0; i < page_actions.size();) {
+    if (page_actions[i]->icon_paths().empty())
+      page_actions.erase(page_actions.begin() + i);
+    else
+      ++i;
+  }
+
   // On startup we sometimes haven't loaded any extensions. This makes sure
   // we catch up when the extensions (and any page actions) load.
   if (page_actions.size() != page_action_views_.size()) {
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/background.html b/chrome/test/data/extensions/browsertest/crash_25562/background.html
new file mode 100644
index 0000000..f27aba8
--- /dev/null
+++ b/chrome/test/data/extensions/browsertest/crash_25562/background.html
@@ -0,0 +1,28 @@
+<html>
+<script>
+  // The Page Action ID.
+  var pageActionId = "TestId";
+
+  // The window this Page Action is associated with.
+  var windowId = -1;
+
+  // The TabId this Page Action is associated with.
+  var tabId = -1;
+
+  // The URL of the page on build.chromium.org.
+  var pageUrl = "";
+
+  chrome.self.onConnect.addListener(function(port) {
+    windowId = port.tab.windowId;
+    tabId = port.tab.id;
+    pageUrl = port.tab.url;
+
+    port.onMessage.addListener(function(mybool) {
+      // Let Chrome know that the PageAction needs to be enabled for this tabId
+      // and for the url of this page.
+      chrome.pageActions.enableForTab(pageActionId,
+                                      {tabId: tabId, url: pageUrl});
+    });
+  });
+</script>
+</html>
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/chrome-16.png b/chrome/test/data/extensions/browsertest/crash_25562/chrome-16.png
new file mode 100644
index 0000000..a3a4f3a
--- /dev/null
+++ b/chrome/test/data/extensions/browsertest/crash_25562/chrome-16.png
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/manifest.json b/chrome/test/data/extensions/browsertest/crash_25562/manifest.json
new file mode 100644
index 0000000..9d9e8fb
--- /dev/null
+++ b/chrome/test/data/extensions/browsertest/crash_25562/manifest.json
@@ -0,0 +1,17 @@
+{
+   "background_page": "background.html",
+   "content_scripts": [ {
+      "js": [ "script.js" ],
+      "matches": [ "http://*/*" ]
+   } ],
+   "description": "A page action with incorrect icon specification",
+   "name": "Test extension",
+   "page_actions": [ {
+      "icon": "chrome-16.png",
+      "id": "TestId",
+      "name": "Page action name",
+      "tooltip": "Page action tooltip"
+   } ],
+   "permissions": [ "http://*/*" ],
+   "version": "1.0"
+}
diff --git a/chrome/test/data/extensions/browsertest/crash_25562/script.js b/chrome/test/data/extensions/browsertest/crash_25562/script.js
new file mode 100644
index 0000000..b602eb8
--- /dev/null
+++ b/chrome/test/data/extensions/browsertest/crash_25562/script.js
@@ -0,0 +1,7 @@
+// Copyright (c) 2009 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.
+
+if (window == top) {
+  chrome.extension.connect().postMessage(true);
+}