[osx] Dynamically retrieve WebContents when updating zoom bubble.

The cached content::WebContents* can become invalid.  The cases where
this happens appear to involve having a new instance in control, so
fetch the pointer dynamically rather than caching it.

BUG=318425

Review URL: https://codereview.chromium.org/65503003

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

5 files changed, 126 insertions, 96 deletions

diff --git a/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h b/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h
index 068f38e..d9e93e5 100644
--- a/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h
+++ b/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h
@@ -14,22 +14,30 @@ namespace content {
 class WebContents;
 }
 
+// TODO(shess): This helper interface exists to let ZoomBubbleController
+// reach ZoomDecoration.  Since ZoomBubbleController is an implementation
+// detail of ZoomDecoration, it would make more sense to just push it
+// into location_bar/ and directly access ZoomDecoration.
+class ZoomBubbleControllerDelegate {
+ public:
+  // Get the web contents associated with this bubble.
+  virtual content::WebContents* GetWebContents() = 0;
+
+  // Called when the bubble is being closed.
+  virtual void OnClose() = 0;
+};
+
 // The ZoomBubbleController is used to display the current page zoom percent
 // when not at the user's default. It is opened by the ZoomDecoration in the
 // location bar.
 @interface ZoomBubbleController : BaseBubbleController {
  @private
-  // The contents for which the zoom percent is being displayed.
-  content::WebContents* contents_;
+  ZoomBubbleControllerDelegate* delegate_;
 
   // Whether or not the bubble should automatically close itself after being
   // opened.
   BOOL autoClose_;
 
-  // A block that is run when the bubble is being closed. This allows any
-  // weak references to be niled.
-  base::mac::ScopedBlock<void(^)(ZoomBubbleController*)> closeObserver_;
-
   // The text field that displays the current zoom percentage.
   base::scoped_nsobject<NSTextField> zoomPercent_;
 
@@ -42,15 +50,13 @@ class WebContents;
 
 // Creates the bubble for a parent window but does not show it.
 - (id)initWithParentWindow:(NSWindow*)parentWindow
-             closeObserver:(void(^)(ZoomBubbleController*))closeObserver;
+                  delegate:(ZoomBubbleControllerDelegate*)delegate;
 
-// Shows the bubble for a given contents at an |anchorPoint| in window
-// coordinates. If |autoClose| is YES, then the bubble was opened in response
-// to a zoom change rather than a direct user action, and it will automatically
-// dismiss itself after a few seconds.
-- (void)showForWebContents:(content::WebContents*)contents
-                anchoredAt:(NSPoint)anchorPoint
-                 autoClose:(BOOL)autoClose;
+// Shows the bubble at |anchorPoint| in window coordinates. If
+// |autoClose| is YES, then the bubble was opened in response to a
+// zoom change rather than a direct user action, and it will
+// automatically dismiss itself after a few seconds.
+- (void)showAnchoredAt:(NSPoint)anchorPoint autoClose:(BOOL)autoClose;
 
 // Called by the ZoomDecoration when the zoom percentage changes.
 - (void)onZoomChanged;
@@ -67,9 +73,6 @@ class WebContents;
 // Closes the bubble synchronously, bypassing any animations.
 - (void)closeWithoutAnimation;
 
-// TODO(shess): For diagnosing <http://crbug.com/318425>.
-- (content::WebContents*)webContents;
-
 @end
 
 namespace chrome {
diff --git a/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm b/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm
index f33a49f..51dcebc 100644
--- a/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm
+++ b/chrome/browser/ui/cocoa/browser/zoom_bubble_controller.mm
@@ -29,6 +29,9 @@
                            action:(SEL)action;
 - (NSTextField*)addZoomPercentTextField;
 - (void)updateAutoCloseTimer;
+
+// Get the WebContents instance and apply the indicated zoom.
+- (void)zoomHelper:(content::PageZoom)alterPageZoom;
 @end
 
 // Button that highlights the background on mouse over.
@@ -73,7 +76,7 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
 @implementation ZoomBubbleController
 
 - (id)initWithParentWindow:(NSWindow*)parentWindow
-             closeObserver:(void(^)(ZoomBubbleController*))closeObserver {
+                  delegate:(ZoomBubbleControllerDelegate*)delegate {
   base::scoped_nsobject<InfoBubbleWindow> window(
       [[InfoBubbleWindow alloc] initWithContentRect:NSMakeRect(0, 0, 200, 100)
                                           styleMask:NSBorderlessWindowMask
@@ -83,7 +86,7 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
                        parentWindow:parentWindow
                          anchoredAt:NSZeroPoint])) {
     [window setCanBecomeKeyWindow:NO];
-    closeObserver_.reset(Block_copy(closeObserver));
+    delegate_ = delegate;
 
     ui::NativeTheme* nativeTheme = ui::NativeTheme::instance();
     [[self bubble] setAlignment:info_bubble::kAlignRightEdgeToAnchorEdge];
@@ -107,10 +110,7 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
   return self;
 }
 
-- (void)showForWebContents:(content::WebContents*)contents
-                anchoredAt:(NSPoint)anchorPoint
-                 autoClose:(BOOL)autoClose {
-  contents_ = contents;
+- (void)showAnchoredAt:(NSPoint)anchorPoint autoClose:(BOOL)autoClose {
   [self onZoomChanged];
   InfoBubbleWindow* window =
       base::mac::ObjCCastStrict<InfoBubbleWindow>([self window]);
@@ -126,10 +126,15 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
 }
 
 - (void)onZoomChanged {
-  if (!contents_)
-    return;  // NULL in tests.
+  // TODO(shess): It may be appropriate to close the window if
+  // |contents| or |zoomController| are NULL.  But they can be NULL in
+  // tests.
+
+  content::WebContents* contents = delegate_->GetWebContents();
+  if (!contents)
+    return;
 
-  ZoomController* zoomController = ZoomController::FromWebContents(contents_);
+  ZoomController* zoomController = ZoomController::FromWebContents(contents);
   if (!zoomController)
     return;
 
@@ -144,15 +149,15 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
 }
 
 - (void)resetToDefault:(id)sender {
-  chrome_page_zoom::Zoom(contents_, content::PAGE_ZOOM_RESET);
+  [self zoomHelper:content::PAGE_ZOOM_RESET];
 }
 
 - (void)zoomIn:(id)sender {
-  chrome_page_zoom::Zoom(contents_, content::PAGE_ZOOM_IN);
+  [self zoomHelper:content::PAGE_ZOOM_IN];
 }
 
 - (void)zoomOut:(id)sender {
-  chrome_page_zoom::Zoom(contents_, content::PAGE_ZOOM_OUT);
+  [self zoomHelper:content::PAGE_ZOOM_OUT];
 }
 
 - (void)closeWithoutAnimation {
@@ -163,8 +168,8 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
 }
 
 - (void)windowWillClose:(NSNotification*)notification {
-  contents_ = NULL;
-  closeObserver_.get()(self);
+  delegate_->OnClose();
+  delegate_ = NULL;
   [NSObject cancelPreviousPerformRequestsWithTarget:self
                                            selector:@selector(autoCloseBubble)
                                              object:nil];
@@ -298,8 +303,14 @@ void SetZoomBubbleAutoCloseDelayForTesting(NSTimeInterval time_interval) {
   }
 }
 
-- (content::WebContents*)webContents {
-  return contents_;
+- (void)zoomHelper:(content::PageZoom)alterPageZoom {
+  content::WebContents* webContents = delegate_->GetWebContents();
+
+  // TODO(shess): Zoom() immediately dereferences |webContents|, and
+  // there haven't been associated crashes in the wild, so it seems
+  // fine in practice.  It might make sense to close the bubble in
+  // that case, though.
+  chrome_page_zoom::Zoom(webContents, alterPageZoom);
 }
 
 @end
diff --git a/chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm b/chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm
index d5d4301..73a251b 100644
--- a/chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm
+++ b/chrome/browser/ui/cocoa/browser/zoom_bubble_controller_unittest.mm
@@ -13,65 +13,82 @@
 
 typedef CocoaTest ZoomBubbleControllerTest;
 
+namespace {
+
+class TestZoomBubbleControllerDelegate : public ZoomBubbleControllerDelegate {
+ public:
+  TestZoomBubbleControllerDelegate() : did_close_(false) {}
+
+  // Get the web contents associated with this bubble.
+  virtual content::WebContents* GetWebContents() OVERRIDE {
+    return NULL;
+  }
+
+  // Called when the bubble is being closed.
+  virtual void OnClose() OVERRIDE {
+    did_close_ = true;
+  }
+
+  bool did_close() { return did_close_; }
+
+ private:
+  bool did_close_;
+};
+
+}  // namespace
+
 TEST_F(ZoomBubbleControllerTest, CloseObserver) {
-  __block ZoomBubbleController* controller = nil;
-  __block BOOL didObserve = NO;
-  void(^observer)(ZoomBubbleController*) = ^(ZoomBubbleController* bubble) {
-      EXPECT_EQ(controller, bubble);
-      didObserve = YES;
-  };
-
-  controller =
+  TestZoomBubbleControllerDelegate test_delegate;
+
+  ZoomBubbleController* controller =
       [[ZoomBubbleController alloc] initWithParentWindow:test_window()
-                                           closeObserver:observer];
-  [controller showForWebContents:NULL anchoredAt:NSZeroPoint autoClose:NO];
+                                                delegate:&test_delegate];
+  [controller showAnchoredAt:NSZeroPoint autoClose:NO];
   [base::mac::ObjCCastStrict<InfoBubbleWindow>([controller window])
       setAllowedAnimations:info_bubble::kAnimateNone];
 
-  EXPECT_FALSE(didObserve);
+  EXPECT_FALSE(test_delegate.did_close());
 
   [controller close];
   chrome::testing::NSRunLoopRunAllPending();
 
-  EXPECT_TRUE(didObserve);
+  EXPECT_TRUE(test_delegate.did_close());
 }
 
 TEST_F(ZoomBubbleControllerTest, AutoClose) {
-  __block BOOL didObserve = NO;
-  ZoomBubbleController* controller = [[ZoomBubbleController alloc]
-      initWithParentWindow:test_window()
-             closeObserver:^(ZoomBubbleController*) {
-                didObserve = YES;
-             }];
+  TestZoomBubbleControllerDelegate test_delegate;
+
+  ZoomBubbleController* controller =
+      [[ZoomBubbleController alloc] initWithParentWindow:test_window()
+                                                delegate:&test_delegate];
   chrome::SetZoomBubbleAutoCloseDelayForTesting(0);
-  [controller showForWebContents:NULL anchoredAt:NSZeroPoint autoClose:YES];
+  [controller showAnchoredAt:NSZeroPoint autoClose:YES];
   [base::mac::ObjCCastStrict<InfoBubbleWindow>([controller window])
       setAllowedAnimations:info_bubble::kAnimateNone];
 
-  EXPECT_FALSE(didObserve);
+  EXPECT_FALSE(test_delegate.did_close());
   chrome::testing::NSRunLoopRunAllPending();
-  EXPECT_TRUE(didObserve);
+  EXPECT_TRUE(test_delegate.did_close());
 }
 
 TEST_F(ZoomBubbleControllerTest, MouseEnteredExited) {
-  __block BOOL didObserve = NO;
-  ZoomBubbleController* controller = [[ZoomBubbleController alloc]
-      initWithParentWindow:test_window()
-             closeObserver:^(ZoomBubbleController*) {
-                didObserve = YES;
-             }];
+  TestZoomBubbleControllerDelegate test_delegate;
+
+  ZoomBubbleController* controller =
+      [[ZoomBubbleController alloc] initWithParentWindow:test_window()
+                                                delegate:&test_delegate];
 
   chrome::SetZoomBubbleAutoCloseDelayForTesting(0);
-  [controller showForWebContents:NULL anchoredAt:NSZeroPoint autoClose:YES];
+  [controller showAnchoredAt:NSZeroPoint autoClose:YES];
   [base::mac::ObjCCastStrict<InfoBubbleWindow>([controller window])
       setAllowedAnimations:info_bubble::kAnimateNone];
 
-  EXPECT_FALSE(didObserve);
+  EXPECT_FALSE(test_delegate.did_close());
   [controller mouseEntered:nil];
   chrome::testing::NSRunLoopRunAllPending();
-  EXPECT_FALSE(didObserve);
+  EXPECT_FALSE(test_delegate.did_close());
 
   [controller mouseExited:nil];
   chrome::testing::NSRunLoopRunAllPending();
-  EXPECT_TRUE(didObserve);
+  EXPECT_TRUE(test_delegate.did_close());
 }
diff --git a/chrome/browser/ui/cocoa/location_bar/zoom_decoration.h b/chrome/browser/ui/cocoa/location_bar/zoom_decoration.h
index 6bbbe6d..b1cbe19 100644
--- a/chrome/browser/ui/cocoa/location_bar/zoom_decoration.h
+++ b/chrome/browser/ui/cocoa/location_bar/zoom_decoration.h
@@ -8,6 +8,7 @@
 #import <Cocoa/Cocoa.h>
 
 #include "base/basictypes.h"
+#import "chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h"
 #include "chrome/browser/ui/cocoa/location_bar/image_decoration.h"
 
 class LocationBarViewMac;
@@ -17,7 +18,8 @@ class ZoomDecorationTest;
 
 // Zoom icon at the end of the omnibox (close to page actions) when at a
 // non-standard zoom level.
-class ZoomDecoration : public ImageDecoration {
+class ZoomDecoration : public ImageDecoration,
+                       public ZoomBubbleControllerDelegate {
  public:
   explicit ZoomDecoration(LocationBarViewMac* owner);
   virtual ~ZoomDecoration();
@@ -45,6 +47,10 @@ class ZoomDecoration : public ImageDecoration {
   virtual bool OnMousePressed(NSRect frame) OVERRIDE;
   virtual NSString* GetToolTip() OVERRIDE;
 
+  // ZoomBubbleControllerDelegate implementation.
+  virtual content::WebContents* GetWebContents() OVERRIDE;
+  virtual void OnClose() OVERRIDE;
+
   // The control that owns this. Weak.
   LocationBarViewMac* owner_;
 
diff --git a/chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm b/chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm
index b8c1fb2..912b17d 100644
--- a/chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm
+++ b/chrome/browser/ui/cocoa/location_bar/zoom_decoration.mm
@@ -7,7 +7,6 @@
 #include "base/strings/string16.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
-#import "chrome/browser/ui/cocoa/browser/zoom_bubble_controller.h"
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field.h"
 #import "chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.h"
 #import "chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h"
@@ -41,13 +40,6 @@ void ZoomDecoration::Update(ZoomController* zoom_controller) {
   tooltip_.reset([zoom_string retain]);
 
   SetVisible(true);
-
-  // TODO(shess): For diagnosing <http://crbug.com/318425>.  If the
-  // crashes shift from GetUserData() to here, then things should be
-  // refactored to remove |ZoomBubbleController.content_|.
-  if (bubble_)
-    CHECK_EQ(owner_->GetWebContents(), [bubble_ webContents]);
-
   [bubble_ onZoomChanged];
 }
 
@@ -61,34 +53,17 @@ void ZoomDecoration::ShowBubble(BOOL auto_close) {
 
   // Get the frame of the decoration.
   AutocompleteTextField* field = owner_->GetAutocompleteTextField();
-  AutocompleteTextFieldCell* cell = [field cell];
-  const NSRect frame = [cell frameForDecoration:this
-                                        inFrame:[field bounds]];
+  const NSRect frame =
+      [[field cell] frameForDecoration:this inFrame:[field bounds]];
 
   // Find point for bubble's arrow in screen coordinates.
   NSPoint anchor = GetBubblePointInFrame(frame);
   anchor = [field convertPoint:anchor toView:nil];
   anchor = [[field window] convertBaseToScreen:anchor];
 
-  if (!bubble_) {
-    void(^observer)(ZoomBubbleController*) = ^(ZoomBubbleController*) {
-        bubble_ = nil;
-        // If the page is at default zoom then hiding the zoom decoration was
-        // suppressed while the bubble was open. Now that the bubble is closed
-        // the decoration can be hidden.
-        if (IsAtDefaultZoom() && IsVisible()) {
-          SetVisible(false);
-          owner_->OnDecorationsChanged();
-        }
-    };
-    bubble_ =
-        [[ZoomBubbleController alloc] initWithParentWindow:[field window]
-                                             closeObserver:observer];
-  }
-
-  [bubble_ showForWebContents:web_contents
-                   anchoredAt:anchor
-                    autoClose:auto_close];
+  bubble_ = [[ZoomBubbleController alloc] initWithParentWindow:[field window]
+                                                      delegate:this];
+  [bubble_ showAnchoredAt:anchor autoClose:auto_close];
 }
 
 void ZoomDecoration::CloseBubble() {
@@ -103,13 +78,15 @@ bool ZoomDecoration::IsAtDefaultZoom() const {
   content::WebContents* web_contents = owner_->GetWebContents();
   if (!web_contents)
     return false;
+
   ZoomController* zoomController =
       ZoomController::FromWebContents(web_contents);
   return zoomController && zoomController->IsAtDefaultZoom();
 }
 
 bool ZoomDecoration::ShouldShowDecoration() const {
-  return !owner_->GetToolbarModel()->input_in_progress() &&
+  return owner_->GetWebContents() != NULL &&
+      !owner_->GetToolbarModel()->input_in_progress() &&
       (bubble_ || !IsAtDefaultZoom());
 }
 
@@ -128,3 +105,19 @@ bool ZoomDecoration::OnMousePressed(NSRect frame) {
 NSString* ZoomDecoration::GetToolTip() {
   return tooltip_.get();
 }
+
+content::WebContents* ZoomDecoration::GetWebContents() {
+  return owner_->GetWebContents();
+}
+
+void ZoomDecoration::OnClose() {
+  bubble_ = nil;
+
+  // If the page is at default zoom then hiding the zoom decoration
+  // was suppressed while the bubble was open. Now that the bubble is
+  // closed the decoration can be hidden.
+  if (IsAtDefaultZoom() && IsVisible()) {
+    SetVisible(false);
+    owner_->OnDecorationsChanged();
+  }
+}