Fixed race in BrowserActionWithRectangularIcon

This CL add waiting for default icon image to be loaded in the
BrowserActionApiTest.BrowserActionWithRectangularIcon tests. It adds
TestIconImageObserver for this purpose (instead of local
WaitForIconLoaded in MultiActionAPICanvasTest)

Bug: 997820
Change-Id: I447d7563cd0bde20f8787f9eb412978addaecab9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1772128
Commit-Queue: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#690887}

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

@@ -15,6 +15,8 @@
 #include "build/build_config.h"
 #include "chrome/browser/download/download_prefs.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
+#include "chrome/browser/extensions/api/extension_action/test_extension_action_api_observer.h"
+#include "chrome/browser/extensions/api/extension_action/test_icon_image_observer.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_icon_factory.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
@@ -881,14 +883,28 @@ IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, BrowserActionPopupWithIframe) {
 
 IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, BrowserActionWithRectangularIcon) {
   ExtensionTestMessageListener ready_listener("ready", true);
-  ASSERT_TRUE(LoadExtension(
-      test_data_dir_.AppendASCII("browser_action").AppendASCII("rect_icon")));
+
+  const Extension* extension = LoadExtension(
+      test_data_dir_.AppendASCII("browser_action").AppendASCII("rect_icon"));
+  ASSERT_TRUE(extension);
   EXPECT_TRUE(ready_listener.WaitUntilSatisfied());
+
+  // Wait for the default icon to load before accessing the underlying
+  // gfx::Image.
+  TestIconImageObserver::WaitForExtensionActionIcon(extension, profile());
+
   gfx::Image first_icon = GetBrowserActionsBar()->GetIcon(0);
+  ASSERT_FALSE(first_icon.IsEmpty());
+
+  TestExtensionActionAPIObserver observer(profile(), extension->id());
   ResultCatcher catcher;
   ready_listener.Reply(std::string());
   EXPECT_TRUE(catcher.GetNextResult());
+  // Wait for extension action to be updated.
+  observer.Wait();
+
   gfx::Image next_icon = GetBrowserActionsBar()->GetIcon(0);
+  ASSERT_FALSE(next_icon.IsEmpty());
   EXPECT_FALSE(gfx::test::AreImagesEqual(first_icon, next_icon));
 }
 

chrome/browser/extensions/api/extension_action/extension_action_apitest.cc

@@ -13,6 +13,7 @@
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/extensions/api/extension_action/test_extension_action_api_observer.h"
+#include "chrome/browser/extensions/api/extension_action/test_icon_image_observer.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_apitest.h"
@@ -266,39 +267,6 @@ class MultiActionAPITest
     return action_manager->GetExtensionAction(extension);
   }
 
-  // Waits for the given |icon| to finish it's first load.
-  // TODO(devlin): It's unfortunate we need this here. Ideally, either this
-  // would be less convoluted, or would even be taken care of by the extension
-  // loading methods.
-  void WaitForIconLoaded(IconImage* icon) {
-    class IconImageWaiter : public IconImage::Observer {
-     public:
-      IconImageWaiter() : observer_(this) {}
-      ~IconImageWaiter() override = default;
-
-      void Wait(IconImage* icon) {
-        if (!icon->did_complete_initial_load()) {
-          observer_.Add(icon);
-          run_loop_.Run();
-        }
-      }
-
-     private:
-      // IconImage::Observer:
-      void OnExtensionIconImageChanged(IconImage* icon) override {
-        DCHECK(icon->did_complete_initial_load());
-        run_loop_.Quit();
-      }
-
-      base::RunLoop run_loop_;
-      ScopedObserver<IconImage, IconImage::Observer> observer_;
-
-      DISALLOW_COPY_AND_ASSIGN(IconImageWaiter);
-    };
-
-    IconImageWaiter().Wait(icon);
-  }
-
  private:
   std::unique_ptr<ScopedCurrentChannel> current_channel_;
 
@@ -641,7 +609,7 @@ IN_PROC_BROWSER_TEST_P(MultiActionAPICanvasTest, DynamicSetIcon) {
   ASSERT_TRUE(action->default_icon());
   // Wait for the default icon to finish loading; otherwise it may be empty
   // when we check it.
-  WaitForIconLoaded(action->default_icon_image());
+  TestIconImageObserver::WaitForIcon(action->default_icon_image());
 
   int tab_id = GetActiveTabId();
   EXPECT_TRUE(ActionHasDefaultState(*action, tab_id));

chrome/browser/extensions/api/extension_action/test_icon_image_observer.cc

+// Copyright 2019 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/api/extension_action/test_icon_image_observer.h"
+
+#include "chrome/browser/extensions/extension_action.h"
+#include "chrome/browser/extensions/extension_action_manager.h"
+
+namespace extensions {
+
+TestIconImageObserver::TestIconImageObserver() : observer_(this) {}
+TestIconImageObserver::~TestIconImageObserver() = default;
+
+void TestIconImageObserver::Wait(IconImage* icon) {
+  if (!icon->did_complete_initial_load()) {
+    observer_.Add(icon);
+    run_loop_.Run();
+  }
+}
+
+void TestIconImageObserver::OnExtensionIconImageChanged(IconImage* icon) {
+  DCHECK(icon->did_complete_initial_load());
+  run_loop_.Quit();
+}
+
+void TestIconImageObserver::WaitForIcon(IconImage* icon) {
+  TestIconImageObserver().Wait(icon);
+}
+void TestIconImageObserver::WaitForExtensionActionIcon(
+    const Extension* extension,
+    content::BrowserContext* context) {
+  DCHECK(extension);
+  auto* action_manager = ExtensionActionManager::Get(context);
+  ExtensionAction* action = action_manager->GetExtensionAction(*extension);
+
+  DCHECK(action);
+  DCHECK(action->default_icon_image());
+  WaitForIcon(action->default_icon_image());
+}
+
+}  // namespace extensions

chrome/browser/extensions/api/extension_action/test_icon_image_observer.h

+// Copyright 2019 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_API_EXTENSION_ACTION_TEST_ICON_IMAGE_OBSERVER_H_
+#define CHROME_BROWSER_EXTENSIONS_API_EXTENSION_ACTION_TEST_ICON_IMAGE_OBSERVER_H_
+
+#include "base/macros.h"
+#include "base/run_loop.h"
+#include "base/scoped_observer.h"
+#include "extensions/browser/extension_icon_image.h"
+
+namespace extensions {
+
+class Extension;
+
+// This class helps to observe action icons. As default action icons load
+// asynchronously we need to wait for it to finish before using them.
+class TestIconImageObserver : public IconImage::Observer {
+ public:
+  TestIconImageObserver();
+  ~TestIconImageObserver() override;
+
+  void Wait(IconImage* icon);
+
+  static void WaitForIcon(IconImage* icon);
+  static void WaitForExtensionActionIcon(const Extension* extension,
+                                         content::BrowserContext* context);
+
+ private:
+  // IconImage::Observer:
+  void OnExtensionIconImageChanged(IconImage* icon) override;
+
+  base::RunLoop run_loop_;
+  ScopedObserver<IconImage, IconImage::Observer> observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestIconImageObserver);
+};
+}  // namespace extensions
+
+#endif  // CHROME_BROWSER_EXTENSIONS_API_EXTENSION_ACTION_TEST_ICON_IMAGE_OBSERVER_H_

chrome/test/BUILD.gn

@@ -1567,6 +1567,8 @@ if (!is_android) {
         "../browser/extensions/api/extension_action/page_action_apitest.cc",
         "../browser/extensions/api/extension_action/test_extension_action_api_observer.cc",
         "../browser/extensions/api/extension_action/test_extension_action_api_observer.h",
+        "../browser/extensions/api/extension_action/test_icon_image_observer.cc",
+        "../browser/extensions/api/extension_action/test_icon_image_observer.h",
         "../browser/extensions/api/feedback_private/feedback_browsertest.cc",
         "../browser/extensions/api/file_system/file_system_apitest.cc",
         "../browser/extensions/api/file_system/file_system_apitest_chromeos.cc",