Fix crash with BrowserActionView not expecting the ImageLoadingTracker to call it back synchronously after a request to load images (when it finds a cached image).

BUG=57536
TEST=Covered by new test: BrowserActionsContainerTest.TestCrash57536

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

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

3 files changed, 70 insertions, 16 deletions

diff --git a/chrome/browser/views/browser_actions_container.cc b/chrome/browser/views/browser_actions_container.cc
index f93769a..d2db56c 100644
--- a/chrome/browser/views/browser_actions_container.cc
+++ b/chrome/browser/views/browser_actions_container.cc
@@ -75,22 +75,6 @@ BrowserActionButton::BrowserActionButton(Extension* extension,
 
   registrar_.Add(this, NotificationType::EXTENSION_BROWSER_ACTION_UPDATED,
                  Source<ExtensionAction>(browser_action_));
-
-  // The Browser Action API does not allow the default icon path to be changed
-  // at runtime, so we can load this now and cache it.
-  std::string relative_path = browser_action_->default_icon_path();
-  if (relative_path.empty())
-    return;
-
-  // This is a bit sketchy because if ImageLoadingTracker calls
-  // ::OnImageLoaded() before our creator appends up to the view hierarchy, we
-  // will crash. But since we know that ImageLoadingTracker is asynchronous,
-  // this should be OK. And doing this in the constructor means that we don't
-  // have to protect against it getting done multiple times.
-  tracker_.LoadImage(extension, extension->GetResource(relative_path),
-                     gfx::Size(Extension::kBrowserActionIconMaxSize,
-                               Extension::kBrowserActionIconMaxSize),
-                     ImageLoadingTracker::DONT_CACHE);
 }
 
 void BrowserActionButton::Destroy() {
@@ -102,6 +86,28 @@ void BrowserActionButton::Destroy() {
   }
 }
 
+void BrowserActionButton::ViewHierarchyChanged(
+    bool is_add, View* parent, View* child) {
+  if (is_add && child == this) {
+    // The Browser Action API does not allow the default icon path to be
+    // changed at runtime, so we can load this now and cache it.
+    std::string relative_path = browser_action_->default_icon_path();
+    if (relative_path.empty())
+      return;
+
+    // LoadImage is not guaranteed to be synchronous, so we might see the
+    // callback OnImageLoaded execute immediately. It (through UpdateState)
+    // expects GetParent() to return the owner for this button, so this
+    // function is as early as we can start this request.
+    tracker_.LoadImage(extension_, extension_->GetResource(relative_path),
+                       gfx::Size(Extension::kBrowserActionIconMaxSize,
+                                 Extension::kBrowserActionIconMaxSize),
+                       ImageLoadingTracker::DONT_CACHE);
+  }
+
+  MenuButton::ViewHierarchyChanged(is_add, parent, child);
+}
+
 void BrowserActionButton::ButtonPressed(views::Button* sender,
                                         const views::Event& event) {
   panel_->OnBrowserActionExecuted(this, false);
diff --git a/chrome/browser/views/browser_actions_container.h b/chrome/browser/views/browser_actions_container.h
index ddef35d..cd376f7 100644
--- a/chrome/browser/views/browser_actions_container.h
+++ b/chrome/browser/views/browser_actions_container.h
@@ -68,6 +68,9 @@ class BrowserActionButton : public views::MenuButton,
   // Returns the default icon, if any.
   const SkBitmap& default_icon() const { return default_icon_; }
 
+  // Overridden from views::View:
+  virtual void ViewHierarchyChanged(bool is_add, View* parent, View* child);
+
   // Overridden from views::ButtonListener:
   virtual void ButtonPressed(views::Button* sender, const views::Event& event);
 
diff --git a/chrome/browser/views/browser_actions_container_browsertest.cc b/chrome/browser/views/browser_actions_container_browsertest.cc
index dd9f9eb..664b510 100644
--- a/chrome/browser/views/browser_actions_container_browsertest.cc
+++ b/chrome/browser/views/browser_actions_container_browsertest.cc
@@ -2,10 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "chrome/browser/browser.h"
 #include "chrome/browser/extensions/browser_action_test_util.h"
 #include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extensions_service.h"
+#include "chrome/browser/profile.h"
 #include "chrome/browser/views/browser_actions_container.h"
 #include "chrome/common/chrome_switches.h"
+#include "chrome/common/extensions/extension_action.h"
+#include "chrome/common/extensions/extension_resource.h"
 
 class BrowserActionsContainerTest : public ExtensionBrowserTest {
  public:
@@ -153,3 +158,43 @@ IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, DISABLED_Visibility) {
   EXPECT_EQ(3, browser_actions_bar()->VisibleBrowserActions());
   EXPECT_EQ(idA, browser_actions_bar()->GetExtensionId(2));
 }
+
+IN_PROC_BROWSER_TEST_F(BrowserActionsContainerTest, TestCrash57536) {
+  std::cout << "Test starting\n" << std::flush;
+
+  ExtensionsService* service = browser()->profile()->GetExtensionsService();
+  const size_t size_before = service->extensions()->size();
+
+  std::cout << "Loading extension\n" << std::flush;
+
+  // Load extension A (contains browser action).
+  ASSERT_TRUE(LoadExtension(test_data_dir_.AppendASCII("api_test")
+                                          .AppendASCII("browser_action")
+                                          .AppendASCII("crash_57536")));
+
+  Extension* extension = service->extensions()->at(size_before);
+
+  std::cout << "Creating bitmap\n" << std::flush;
+
+  // Create and cache and empty bitmap.
+  SkBitmap bitmap;
+  bitmap.setConfig(SkBitmap::kARGB_8888_Config,
+                    Extension::kBrowserActionIconMaxSize,
+                    Extension::kBrowserActionIconMaxSize);
+  bitmap.allocPixels();
+
+  std::cout << "Set as cached image\n" << std::flush;
+
+  gfx::Size size(Extension::kBrowserActionIconMaxSize,
+                 Extension::kBrowserActionIconMaxSize);
+  extension->SetCachedImage(
+      extension->GetResource(extension->browser_action()->default_icon_path()),
+      bitmap,
+      size);
+
+  std::cout << "Disabling extension\n" << std::flush;
+  DisableExtension(extension->id());
+  std::cout << "Enabling extension\n" << std::flush;
+  EnableExtension(extension->id());
+  std::cout << "Test ending\n" << std::flush;
+}