Tab Search: Fix TabSearchButton observer invalidation

TabSearchButton is-a WidgetObserver and uses a ScopedObserver to
observe its associated bubble Widget.

Currently TabSearchButton removes itself as an observer of the
Widget in the OnWidgetClosed() WidgetObserver method.

However this is not called when the operating system deletes the
window and the ScopedObserver causes a heap use-after-free error
when it goes out of scope.

This CL fixes the issue by removing itself as an observer of the
Widet in the OnWidgetDestroying() method which is always called
when the widget is deleted.

See:
https://source.chromium.org/chromium/chromium/src/+/master:ui/aura/window.cc;l=130;drc=f7600fd0b168f7c7ebefbb27a91cbefb0d090c0c

Similar usage of a ScopedObserver observing Widgets:
https://source.chromium.org/chromium/chromium/src/+/master:chrome/browser/ui/views/frame/browser_view.cc;l=2572;drc=f7600fd0b168f7c7ebefbb27a91cbefb0d090c0c

Bug: 1143448
Change-Id: I7e88d5cb64800e6fb2a7c88b2233fc628e7d6d80
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2506561Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Thomas Lukaszewicz <tluk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#822206}

chrome/browser/ui/views/tabs/tab_search_button.cc

@@ -53,7 +53,7 @@ void TabSearchButton::FrameColorsChanged() {
            gfx::CreateVectorIcon(kTabSearchIcon, GetForegroundColor()));
 }
 
-void TabSearchButton::OnWidgetClosing(views::Widget* widget) {
+void TabSearchButton::OnWidgetDestroying(views::Widget* widget) {
   DCHECK_EQ(bubble_, widget);
   observed_bubble_widget_.Remove(bubble_);
   bubble_ = nullptr;

chrome/browser/ui/views/tabs/tab_search_button.h

@@ -39,7 +39,7 @@ class TabSearchButton : public NewTabButton,
   void FrameColorsChanged() override;
 
   // views::WidgetObserver:
-  void OnWidgetClosing(views::Widget* widget) override;
+  void OnWidgetDestroying(views::Widget* widget) override;
 
   // When this is called the bubble may already be showing or be loading in.
   // This returns true if the method call results in the creation of a new Tab

chrome/browser/ui/views/tabs/tab_search_button_browsertest.cc

@@ -7,6 +7,7 @@
 #include <vector>
 
 #include "base/feature_list.h"
+#include "base/run_loop.h"
 #include "base/test/scoped_feature_list.h"
 #include "build/build_config.h"
 #include "chrome/browser/ui/browser.h"
@@ -41,6 +42,15 @@ class TabSearchButtonBrowserTest : public InProcessBrowserTest {
     return browser_view()->GetTabSearchButton();
   }
 
+  void RunUntilBubbleWidgetDestroyed() {
+    ASSERT_NE(nullptr, tab_search_button()->bubble_for_testing());
+    base::RunLoop run_loop;
+    base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                  run_loop.QuitClosure());
+    run_loop.Run();
+    ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
+  }
+
  private:
   base::test::ScopedFeatureList scoped_feature_list_;
 };
@@ -50,10 +60,10 @@ IN_PROC_BROWSER_TEST_F(TabSearchButtonBrowserTest, CreateAndClose) {
   views::test::ButtonTestApi(tab_search_button()).NotifyClick(GetDummyEvent());
   ASSERT_NE(nullptr, tab_search_button()->bubble_for_testing());
 
-  // Close the tab search bubble widget, the bubble should be cleared from the
-  // TabSearchButton.
   tab_search_button()->CloseTabSearchBubble();
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
+  ASSERT_TRUE(tab_search_button()->bubble_for_testing()->IsClosed());
+
+  RunUntilBubbleWidgetDestroyed();
 }
 
 IN_PROC_BROWSER_TEST_F(TabSearchButtonBrowserTest, TestBubbleVisible) {
@@ -73,10 +83,10 @@ IN_PROC_BROWSER_TEST_F(TabSearchButtonBrowserTest, TestBubbleVisible) {
   // The bubble should be visible after being shown.
   EXPECT_TRUE(tab_search_button()->IsBubbleVisible());
 
-  // Close the tab search bubble widget, the bubble should be cleared from the
-  // TabSearchButton.
   tab_search_button()->CloseTabSearchBubble();
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
+  ASSERT_TRUE(tab_search_button()->bubble_for_testing()->IsClosed());
+
+  RunUntilBubbleWidgetDestroyed();
 }
 
 IN_PROC_BROWSER_TEST_F(TabSearchButtonBrowserTest, BubbleNotVisibleIncognito) {
@@ -100,9 +110,9 @@ IN_PROC_BROWSER_TEST_F(TabSearchButtonBrowserTest, TestBubbleKeyboardShortcut) {
   // Accelerator keys should have created the tab search bubble.
   ASSERT_NE(nullptr, tab_search_button()->bubble_for_testing());
 
-  // Close the tab search bubble widget, the bubble should be cleared from the
-  // TabSearchButton.
   tab_search_button()->CloseTabSearchBubble();
-  ASSERT_EQ(nullptr, tab_search_button()->bubble_for_testing());
+  ASSERT_TRUE(tab_search_button()->bubble_for_testing()->IsClosed());
+
+  RunUntilBubbleWidgetDestroyed();
 }
 #endif