Close the Zoom Bubble when its WebContents is destroyed or hidden.

ZoomBubbleView has a raw, weak pointer to WebContents which can result
in lifetime problems. On Mac, a UAF can result. Elsewhere, the result
is typically a deleted pointer being passed to FindBrowserWithWebContents
(which is less likely to cause badness).

Also on Mac currently, the zoom bubble isn't immediately hidden on tab
switch. This is because Cocoa bubbles are managed by
base_bubble_controller.mm in a general way: it observes the tab strip
via a bridge and dismisses a bubble of whatever type is currently
showing. However, views bubbles can not opt into this framework.

On other platforms, BrowserView explicitly closes the zoom bubble on
tab switch.

Most other views bubbles observe WebContentsObserver::WasHidden(),
so do the same for the zoom bubble. And test.

Bug: 791907, 404979
Change-Id: I36d174530423dbcdb7f93d5e3a575ce07f74d8e7
Reviewed-on: https://chromium-review.googlesource.com/848113
Commit-Queue: Trent Apted <tapted@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526878}

chrome/browser/ui/views/frame/browser_view.cc

@@ -82,7 +82,6 @@
 #include "chrome/browser/ui/views/infobars/infobar_container_view.h"
 #include "chrome/browser/ui/views/location_bar/location_bar_view.h"
 #include "chrome/browser/ui/views/location_bar/star_view.h"
-#include "chrome/browser/ui/views/location_bar/zoom_bubble_view.h"
 #include "chrome/browser/ui/views/new_back_shortcut_bubble.h"
 #include "chrome/browser/ui/views/omnibox/omnibox_view_views.h"
 #include "chrome/browser/ui/views/profiles/profile_indicator_icon.h"
@@ -796,7 +795,6 @@ void BrowserView::OnActiveTabChanged(content::WebContents* old_contents,
   UpdateTitleBar();
 
   TranslateBubbleView::CloseCurrentBubble();
-  ZoomBubbleView::CloseCurrentBubble();
 }
 
 void BrowserView::ZoomChangedForActiveTab(bool can_show_bubble) {

chrome/browser/ui/views/location_bar/zoom_bubble_view.cc

@@ -153,7 +153,7 @@ void ZoomBubbleView::ShowBubble(content::WebContents* web_contents,
   // initiated by an extension, then the bubble can be reused and only the label
   // text needs to be updated.
   if (zoom_bubble_ && zoom_bubble_->GetAnchorView() == anchor_view && !client) {
-    DCHECK_EQ(web_contents, zoom_bubble_->web_contents_);
+    DCHECK_EQ(web_contents, zoom_bubble_->web_contents());
     zoom_bubble_->Refresh();
     return;
   }
@@ -224,13 +224,13 @@ ZoomBubbleView::ZoomBubbleView(
     DisplayReason reason,
     ImmersiveModeController* immersive_mode_controller)
     : LocationBarBubbleDelegateView(anchor_view, anchor_point, web_contents),
+      WebContentsObserver(web_contents),
       auto_close_duration_(kBubbleCloseDelayDefault),
       image_button_(nullptr),
       label_(nullptr),
       zoom_out_button_(nullptr),
       zoom_in_button_(nullptr),
       reset_button_(nullptr),
-      web_contents_(web_contents),
       auto_close_(reason == AUTOMATIC),
       ignore_close_bubble_(false),
       immersive_mode_controller_(immersive_mode_controller) {
@@ -313,7 +313,7 @@ void ZoomBubbleView::Init() {
   }
 
   // Add zoom label with the new zoom percent.
-  label_ = new ZoomValue(web_contents_);
+  label_ = new ZoomValue(web_contents());
   UpdateZoomPercent();
   label_->SetProperty(views::kMarginsKey,
                       new gfx::Insets(label_vertical_margin));
@@ -358,9 +358,8 @@ void ZoomBubbleView::WindowClosing() {
   bool this_bubble = zoom_bubble_ == this;
   if (this_bubble)
     zoom_bubble_ = nullptr;
+
   UpdateZoomIconVisibility();
-  if (this_bubble)
-    web_contents_ = nullptr;
 }
 
 void ZoomBubbleView::CloseBubble() {
@@ -368,7 +367,7 @@ void ZoomBubbleView::CloseBubble() {
     return;
 
   // Widget's Close() is async, but we don't want to use zoom_bubble_ after
-  // this. Additionally web_contents_ may have been destroyed.
+  // this. Additionally web_contents() may have been destroyed.
   zoom_bubble_ = nullptr;
   LocationBarBubbleDelegateView::CloseBubble();
 }
@@ -385,17 +384,17 @@ void ZoomBubbleView::ButtonPressed(views::Button* sender,
 
   if (sender == image_button_) {
     DCHECK(extension_info_.icon_image) << "Invalid button press.";
-    Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
+    Browser* browser = chrome::FindBrowserWithWebContents(web_contents());
     chrome::AddSelectedTabWithURL(
         browser, GURL(base::StringPrintf("chrome://extensions?id=%s",
                                          extension_info_.id.c_str())),
         ui::PAGE_TRANSITION_FROM_API);
   } else if (sender == zoom_out_button_) {
-    zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_OUT);
+    zoom::PageZoom::Zoom(web_contents(), content::PAGE_ZOOM_OUT);
   } else if (sender == zoom_in_button_) {
-    zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_IN);
+    zoom::PageZoom::Zoom(web_contents(), content::PAGE_ZOOM_IN);
   } else if (sender == reset_button_) {
-    zoom::PageZoom::Zoom(web_contents_, content::PAGE_ZOOM_RESET);
+    zoom::PageZoom::Zoom(web_contents(), content::PAGE_ZOOM_RESET);
   } else {
     NOTREACHED();
   }
@@ -416,6 +415,14 @@ void ZoomBubbleView::OnExtensionIconImageChanged(
   image_button_->SchedulePaint();
 }
 
+void ZoomBubbleView::WasHidden() {
+  CloseBubble();
+}
+
+void ZoomBubbleView::WebContentsDestroyed() {
+  CloseBubble();
+}
+
 void ZoomBubbleView::SetExtensionInfo(const extensions::Extension* extension) {
   DCHECK(extension);
   extension_info_.id = extension->id();
@@ -435,13 +442,9 @@ void ZoomBubbleView::SetExtensionInfo(const extensions::Extension* extension) {
   bool has_default_sized_icon =
       !icons.Get(gfx::kFaviconSize, ExtensionIconSet::MATCH_EXACTLY).empty();
   if (has_default_sized_icon) {
-    extension_info_.icon_image.reset(
-        new extensions::IconImage(web_contents_->GetBrowserContext(),
-                                  extension,
-                                  icons,
-                                  icon_size,
-                                  default_extension_icon_image,
-                                  this));
+    extension_info_.icon_image.reset(new extensions::IconImage(
+        web_contents()->GetBrowserContext(), extension, icons, icon_size,
+        default_extension_icon_image, this));
     return;
   }
 
@@ -452,22 +455,22 @@ void ZoomBubbleView::SetExtensionInfo(const extensions::Extension* extension) {
 
   icon_size = browser_action->default_icon.map().begin()->first;
   extension_info_.icon_image.reset(
-      new extensions::IconImage(web_contents_->GetBrowserContext(),
-                                extension,
-                                browser_action->default_icon,
-                                icon_size,
-                                default_extension_icon_image,
-                                this));
+      new extensions::IconImage(web_contents()->GetBrowserContext(), extension,
+                                browser_action->default_icon, icon_size,
+                                default_extension_icon_image, this));
 }
 
 void ZoomBubbleView::UpdateZoomPercent() {
   label_->SetText(base::FormatPercent(
-      zoom::ZoomController::FromWebContents(web_contents_)->GetZoomPercent()));
+      zoom::ZoomController::FromWebContents(web_contents())->GetZoomPercent()));
   label_->NotifyAccessibilityEvent(ui::AX_EVENT_TEXT_CHANGED, true);
 }
 
 void ZoomBubbleView::UpdateZoomIconVisibility() {
-  Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
+  // Note that we can't rely on web_contents() here, as it may have been
+  // destroyed by the time we get this call.
+  Browser* browser = chrome::FindBrowserWithWindow(
+      platform_util::GetTopLevel(parent_window()));
   if (browser && browser->window() && browser->window()->GetLocationBar())
     browser->window()->GetLocationBar()->UpdateZoomViewVisibility();
 }

chrome/browser/ui/views/location_bar/zoom_bubble_view.h

@@ -12,6 +12,7 @@
 #include "chrome/browser/ui/views/location_bar/location_bar_bubble_delegate_view.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "extensions/browser/extension_icon_image.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/label.h"
@@ -28,7 +29,8 @@ class ImageButton;
 class ZoomBubbleView : public LocationBarBubbleDelegateView,
                        public views::ButtonListener,
                        public ImmersiveModeController::Observer,
-                       public extensions::IconImage::Observer {
+                       public extensions::IconImage::Observer,
+                       public content::WebContentsObserver {
  public:
   // Shows the bubble and automatically closes it after a short time period if
   // |reason| is AUTOMATIC.
@@ -99,6 +101,10 @@ class ZoomBubbleView : public LocationBarBubbleDelegateView,
   // extensions::IconImage::Observer:
   void OnExtensionIconImageChanged(extensions::IconImage* /* image */) override;
 
+  // content::WebContentsObserver:
+  void WasHidden() override;
+  void WebContentsDestroyed() override;
+
   // Sets information about the extension that initiated the zoom change.
   // Calling this method asserts that the extension |extension| did initiate
   // the zoom change.
@@ -142,9 +148,6 @@ class ZoomBubbleView : public LocationBarBubbleDelegateView,
   views::Button* zoom_in_button_;
   views::Button* reset_button_;
 
-  // The WebContents for the page whose zoom has changed.
-  content::WebContents* web_contents_;
-
   // Whether the currently displayed bubble will automatically close.
   bool auto_close_;
 

chrome/browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc

@@ -5,13 +5,20 @@
 #include "chrome/browser/ui/views/location_bar/zoom_bubble_view.h"
 
 #include "build/build_config.h"
+#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller.h"
 #include "chrome/browser/ui/exclusive_access/fullscreen_controller_test.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/ui/test/test_browser_dialog.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "ui/base/ui_features.h"
+#include "ui/views/test/test_widget_observer.h"
+
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/immersive_mode_controller.h"
-#include "chrome/test/base/in_process_browser_test.h"
+#endif
 
 #if defined(OS_CHROMEOS)
 #include "ash/public/cpp/immersive/immersive_fullscreen_controller_test_api.h"
@@ -21,12 +28,25 @@
 
 using ZoomBubbleBrowserTest = InProcessBrowserTest;
 
+namespace {
+
+void ShowInActiveTab(Browser* browser) {
+  content::WebContents* web_contents =
+      browser->tab_strip_model()->GetActiveWebContents();
+  ZoomBubbleView::ShowBubble(web_contents, gfx::Point(),
+                             ZoomBubbleView::USER_GESTURE);
+  EXPECT_TRUE(ZoomBubbleView::GetZoomBubble());
+}
+
+}  // namespace
+
 // TODO(linux_aura) http://crbug.com/163931
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS) && defined(USE_AURA)
 #define MAYBE_NonImmersiveFullscreen DISABLED_NonImmersiveFullscreen
 #else
 #define MAYBE_NonImmersiveFullscreen NonImmersiveFullscreen
 #endif
+#if !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
 // Test whether the zoom bubble is anchored and whether it is visible when in
 // non-immersive fullscreen.
 IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, MAYBE_NonImmersiveFullscreen) {
@@ -72,6 +92,7 @@ IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, MAYBE_NonImmersiveFullscreen) {
     waiter->Wait();
   }
 }
+#endif  // !defined(OS_MACOSX) || BUILDFLAG(MAC_VIEWS_BROWSER)
 
 #if defined(OS_CHROMEOS)
 // Test whether the zoom bubble is anchored and whether it is visible when in
@@ -144,12 +165,11 @@ IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, ImmersiveFullscreen) {
 
 // Tests that trying to open zoom bubble with stale WebContents is safe.
 IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, NoWebContentsIsSafe) {
-  BrowserView* browser_view = static_cast<BrowserView*>(browser()->window());
-  content::WebContents* web_contents = browser_view->GetActiveWebContents();
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
 
   ZoomBubbleView::ShowBubble(web_contents, gfx::Point(),
                              ZoomBubbleView::AUTOMATIC);
-
   // Close the current tab and try opening the zoom bubble with stale
   // |web_contents|.
   chrome::CloseTab(browser());
@@ -157,17 +177,44 @@ IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, NoWebContentsIsSafe) {
                              ZoomBubbleView::AUTOMATIC);
 }
 
+// Ensure a tab switch closes the bubble.
+IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, TabSwitchCloses) {
+  AddTabAtIndex(0, GURL(url::kAboutBlankURL), ui::PAGE_TRANSITION_LINK);
+  ShowInActiveTab(browser());
+  chrome::SelectNextTab(browser());
+  EXPECT_FALSE(ZoomBubbleView::GetZoomBubble());
+}
+
+// Ensure the bubble is dismissed on tab closure and doesn't reference a
+// destroyed WebContents.
+IN_PROC_BROWSER_TEST_F(ZoomBubbleBrowserTest, DestroyedWebContents) {
+  AddTabAtIndex(0, GURL(url::kAboutBlankURL), ui::PAGE_TRANSITION_LINK);
+  ShowInActiveTab(browser());
+
+  ZoomBubbleView* bubble = ZoomBubbleView::GetZoomBubble();
+  EXPECT_TRUE(bubble);
+
+  views::test::TestWidgetObserver observer(bubble->GetWidget());
+  EXPECT_FALSE(bubble->GetWidget()->IsClosed());
+
+  chrome::CloseTab(browser());
+  EXPECT_FALSE(ZoomBubbleView::GetZoomBubble());
+
+  // Widget::Close() completes asynchronously, so it's still safe to access
+  // |bubble| here, even though GetZoomBubble() returned null.
+  EXPECT_FALSE(observer.widget_closed());
+  EXPECT_TRUE(bubble->GetWidget()->IsClosed());
+
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(observer.widget_closed());
+}
+
 class ZoomBubbleDialogTest : public DialogBrowserTest {
  public:
   ZoomBubbleDialogTest() {}
 
   // DialogBrowserTest:
-  void ShowUi(const std::string& name) override {
-    BrowserView* browser_view = static_cast<BrowserView*>(browser()->window());
-    content::WebContents* web_contents = browser_view->GetActiveWebContents();
-    ZoomBubbleView::ShowBubble(web_contents, gfx::Point(),
-                               ZoomBubbleView::USER_GESTURE);
-  }
+  void ShowUi(const std::string& name) override { ShowInActiveTab(browser()); }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ZoomBubbleDialogTest);

chrome/test/BUILD.gn

@@ -1353,6 +1353,7 @@ test("browser_tests") {
         "../browser/ui/views/frame/browser_window_property_manager_browsertest_win.cc",
         "../browser/ui/views/importer/import_lock_dialog_view_browsertest.cc",
         "../browser/ui/views/location_bar/content_setting_bubble_dialog_browsertest.cc",
+        "../browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc",
         "../browser/ui/views/passwords/manage_passwords_bubble_view_browsertest.cc",
         "../browser/ui/views/payments/contact_info_editor_view_controller_browsertest.cc",
         "../browser/ui/views/payments/credit_card_editor_view_controller_browsertest.cc",
@@ -1419,7 +1420,6 @@ test("browser_tests") {
           "../browser/ui/views/frame/browser_non_client_frame_view_browsertest.cc",
           "../browser/ui/views/frame/browser_view_browsertest.cc",
           "../browser/ui/views/location_bar/location_icon_view_browsertest.cc",
-          "../browser/ui/views/location_bar/zoom_bubble_view_browsertest.cc",
           "../browser/ui/views/media_router/media_router_ui_browsertest.cc",
           "../browser/ui/views/page_info/page_info_bubble_view_browsertest.cc",
           "../browser/ui/views/passwords/password_dialog_view_browsertest.cc",