Don't try to use ManagePasswordsBubbleModel if the password bubble was closed.

The bubble may be closed by some navigation in the content area. However, if the user pressed a button before closing and released after then the button action is still sent. Moreover, all the objects like NSWindow, NSWindowController, NSViewController are still alive. The UI code should not ping ManagePasswordsBubbleModel which is actually destroyed.

BUG=579726

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

Cr-Commit-Position: refs/heads/master@{#371513}

chrome/browser/ui/cocoa/passwords/base_passwords_content_view_controller.h

@@ -7,12 +7,17 @@
 
 #import <Cocoa/Cocoa.h>
 
+class ManagePasswordsBubbleModel;
+
 // Handles user interaction with the content view.
 @protocol ManagePasswordsBubbleContentViewDelegate<NSObject>
 
 // The user performed an action that should dismiss the bubble.
 - (void)viewShouldDismiss;
 
+// Returns the model object.
+@property(nonatomic, readonly) ManagePasswordsBubbleModel* model;
+
 @end
 
 // Base class for a state of the password management bubble.

chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.h

@@ -44,10 +44,8 @@ class ManagePasswordsControllerTest : public CocoaProfileTest {
 
 // Helper delegate for testing the views of the password management bubble.
 @interface ContentViewDelegateMock
-    : NSObject<ManagePasswordsBubbleContentViewDelegate> {
- @private
-  BOOL _dismissed;
-}
+    : NSObject<ManagePasswordsBubbleContentViewDelegate>
+@property(nonatomic) ManagePasswordsBubbleModel* model;
 @property(readonly, nonatomic) BOOL dismissed;
 @end
 

chrome/browser/ui/cocoa/passwords/base_passwords_controller_test.mm

@@ -110,6 +110,7 @@ ManagePasswordsControllerTest::GetDisplayReason() const {
 
 @implementation ContentViewDelegateMock
 
+@synthesize model = _model;
 @synthesize dismissed = _dismissed;
 
 - (void)viewShouldDismiss {

chrome/browser/ui/cocoa/passwords/passwords_bubble_controller.mm

@@ -52,6 +52,8 @@
 
 - (void)close {
   [currentController_ bubbleWillDisappear];
+  // The bubble is about to be closed. It destroys the model.
+  model_ = nil;
   [super close];
 }
 
@@ -60,13 +62,11 @@
   currentController_.reset();
   if (model_->state() == password_manager::ui::PENDING_PASSWORD_STATE) {
     currentController_.reset([[SavePendingPasswordViewController alloc]
-        initWithModel:model_
-             delegate:self]);
+        initWithDelegate:self]);
   } else if (model_->state() ==
              password_manager::ui::PENDING_PASSWORD_UPDATE_STATE) {
     currentController_.reset([[UpdatePendingPasswordViewController alloc]
-        initWithModel:model_
-             delegate:self]);
+        initWithDelegate:self]);
   } else if (model_->state() == password_manager::ui::CONFIRMATION_STATE) {
     currentController_.reset(
         [[ManagePasswordsBubbleConfirmationViewController alloc]
@@ -148,6 +148,10 @@
   [self close];
 }
 
+- (ManagePasswordsBubbleModel*)model {
+  return model_;
+}
+
 @end
 
 @implementation ManagePasswordsBubbleController (Testing)

chrome/browser/ui/cocoa/passwords/passwords_bubble_controller_unittest.mm

@@ -81,4 +81,11 @@ TEST_F(ManagePasswordsBubbleControllerTest,
             [[controller() currentController] class]);
 }
 
+TEST_F(ManagePasswordsBubbleControllerTest, ClearModelOnClose) {
+  SetUpUpdatePendingState(false);
+  EXPECT_TRUE(controller().model);
+  [controller() close];
+  EXPECT_FALSE(controller().model);
+}
+
 }  // namespace

chrome/browser/ui/cocoa/passwords/pending_password_view_controller.h

@@ -10,18 +10,14 @@
 #include "base/mac/scoped_nsobject.h"
 #import "chrome/browser/ui/cocoa/passwords/base_passwords_content_view_controller.h"
 
-class ManagePasswordsBubbleModel;
 @class PasswordsListViewController;
 
 // Base class for the views that offer to save/update the user's password.
 @interface PendingPasswordViewController
     : ManagePasswordsBubbleContentViewController<NSTextViewDelegate> {
  @private
-  ManagePasswordsBubbleModel* model_;  // weak
   base::scoped_nsobject<NSButton> closeButton_;
 }
-- (id)initWithModel:(ManagePasswordsBubbleModel*)model
-           delegate:(id<ManagePasswordsBubbleContentViewDelegate>)delegate;
 
 // Creates a controller for showing username/password and returns its view.
 - (NSView*)createPasswordView;

chrome/browser/ui/cocoa/passwords/pending_password_view_controller.mm

@@ -22,18 +22,12 @@ const SkColor kWarmWelcomeColor = SkColorSetRGB(0x64, 0x64, 0x64);
 
 @implementation PendingPasswordViewController
 
-- (id)initWithModel:(ManagePasswordsBubbleModel*)model
-           delegate:(id<ManagePasswordsBubbleContentViewDelegate>)delegate {
-  if (([super initWithDelegate:delegate])) {
-    model_ = model;
-  }
-  return self;
-}
-
 - (BOOL)textView:(NSTextView*)textView
     clickedOnLink:(id)link
           atIndex:(NSUInteger)charIndex {
-  model_->OnBrandLinkClicked();
+  ManagePasswordsBubbleModel* model = [self model];
+  if (model)
+    model->OnBrandLinkClicked();
   [delegate_ viewShouldDismiss];
   return YES;
 }
@@ -90,8 +84,9 @@ const SkColor kWarmWelcomeColor = SkColorSetRGB(0x64, 0x64, 0x64);
   [view addSubview:closeButton_];
 
   // Title.
+  ManagePasswordsBubbleModel* model = [self model];
   HyperlinkTextView* titleView = TitleLabelWithLink(
-      model_->title(), model_->title_brand_link_range(), self);
+      model->title(), model->title_brand_link_range(), self);
 
   // Force the text to wrap to fit in the bubble size.
   int titleRightPadding =
@@ -177,7 +172,7 @@ const SkColor kWarmWelcomeColor = SkColorSetRGB(0x64, 0x64, 0x64);
 }
 
 - (ManagePasswordsBubbleModel*)model {
-  return model_;
+  return [delegate_ model];
 }
 
 @end

chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.h

@@ -18,9 +18,7 @@ class ManagePasswordsBubbleModel;
   base::scoped_nsobject<NSButton> neverButton_;
   base::scoped_nsobject<PasswordsListViewController> passwordItem_;
 }
-- (SavePendingPasswordViewController*)
-initWithModel:(ManagePasswordsBubbleModel*)model
-     delegate:(id<ManagePasswordsBubbleContentViewDelegate>)delegate;
+
 - (NSView*)createPasswordView;
 - (NSArray*)createButtonsAndAddThemToView:(NSView*)view;
 @end

chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller.mm

@@ -16,25 +16,21 @@
 
 @implementation SavePendingPasswordViewController
 
-- (SavePendingPasswordViewController*)
-initWithModel:(ManagePasswordsBubbleModel*)model
-     delegate:(id<ManagePasswordsBubbleContentViewDelegate>)delegate {
-  self = [super initWithModel:model
-                     delegate:delegate];
-  return self;
-}
-
 - (NSButton*)defaultButton {
   return saveButton_;
 }
 
 - (void)onSaveClicked:(id)sender {
-  self.model->OnSaveClicked();
+  ManagePasswordsBubbleModel* model = self.model;
+  if (model)
+    model->OnSaveClicked();
   [delegate_ viewShouldDismiss];
 }
 
 - (void)onNeverForThisSiteClicked:(id)sender {
-  self.model->OnNeverForThisSiteClicked();
+  ManagePasswordsBubbleModel* model = self.model;
+  if (model)
+    model->OnNeverForThisSiteClicked();
   [delegate_ viewShouldDismiss];
 }
 

chrome/browser/ui/cocoa/passwords/save_pending_password_view_controller_unittest.mm

@@ -32,10 +32,10 @@ class SavePendingPasswordViewControllerTest
 
   SavePendingPasswordViewController* controller() {
     if (!controller_) {
+      [delegate() setModel:GetModelAndCreateIfNull()];
       controller_.reset([[SavePendingPasswordViewController alloc]
-          initWithModel:GetModelAndCreateIfNull()
-               delegate:delegate()]);
-      [controller_ loadView];
+          initWithDelegate:delegate()]);
+      [controller_ view];
     }
     return controller_.get();
   }
@@ -86,4 +86,16 @@ TEST_F(SavePendingPasswordViewControllerTest,
   EXPECT_FALSE([controller() createPasswordView]);
 }
 
+TEST_F(SavePendingPasswordViewControllerTest, CloseBubbleAndHandleClick) {
+  // A user may press mouse down, some navigation closes the bubble, mouse up
+  // still sends the action.
+  SetUpSavePendingState(false);
+  EXPECT_CALL(*ui_controller(), SavePassword()).Times(0);
+  EXPECT_CALL(*ui_controller(), NeverSavePassword()).Times(0);
+  [controller() bubbleWillDisappear];
+  [delegate() setModel:nil];
+  [controller().neverButton performClick:nil];
+  [controller().saveButton performClick:nil];
+}
+
 }  // namespace

chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.h

@@ -21,9 +21,7 @@ class ManagePasswordsBubbleModel;
   base::scoped_nsobject<CredentialsSelectionView>
       passwordWithUsernameSelectionItem_;
 }
-- (UpdatePendingPasswordViewController*)
-initWithModel:(ManagePasswordsBubbleModel*)model
-     delegate:(id<ManagePasswordsBubbleContentViewDelegate>)delegate;
+
 - (NSView*)createPasswordView;
 - (NSArray*)createButtonsAndAddThemToView:(NSView*)view;
 @end

chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller.mm

@@ -17,30 +17,28 @@
 
 @implementation UpdatePendingPasswordViewController
 
-- (UpdatePendingPasswordViewController*)
-initWithModel:(ManagePasswordsBubbleModel*)model
-     delegate:(id<ManagePasswordsBubbleContentViewDelegate>)delegate {
-  self = [super initWithModel:model delegate:delegate];
-  return self;
-}
-
 - (NSButton*)defaultButton {
   return updateButton_;
 }
 
 - (void)onUpdateClicked:(id)sender {
-  if (passwordWithUsernameSelectionItem_) {
-    // Multi account case.
-    self.model->OnUpdateClicked(
-        *[passwordWithUsernameSelectionItem_ getSelectedCredentials]);
-  } else {
-    self.model->OnUpdateClicked(self.model->pending_password());
+  ManagePasswordsBubbleModel* model = [self model];
+  if (model) {
+    if (passwordWithUsernameSelectionItem_) {
+      // Multi account case.
+      model->OnUpdateClicked(
+          *[passwordWithUsernameSelectionItem_ getSelectedCredentials]);
+    } else {
+      model->OnUpdateClicked(model->pending_password());
+    }
   }
   [delegate_ viewShouldDismiss];
 }
 
 - (void)onNopeClicked:(id)sender {
-  self.model->OnNopeUpdateClicked();
+  ManagePasswordsBubbleModel* model = [self model];
+  if (model)
+    model->OnNopeUpdateClicked();
   [delegate_ viewShouldDismiss];
 }
 

chrome/browser/ui/cocoa/passwords/update_pending_password_view_controller_unittest.mm

@@ -36,10 +36,10 @@ class UpdatePendingPasswordViewControllerTest
 
   UpdatePendingPasswordViewController* controller() {
     if (!controller_) {
+      [delegate() setModel:GetModelAndCreateIfNull()];
       controller_.reset([[UpdatePendingPasswordViewController alloc]
-          initWithModel:GetModelAndCreateIfNull()
-               delegate:delegate()]);
-      [controller_ loadView];
+          initWithDelegate:delegate()]);
+      [controller_ view];
     }
     return controller_.get();
   }
@@ -92,4 +92,16 @@ TEST_F(UpdatePendingPasswordViewControllerTest,
             [[controller() createPasswordView] class]);
 }
 
+TEST_F(UpdatePendingPasswordViewControllerTest, CloseBubbleAndHandleClick) {
+  // A user may press mouse down, some navigation closes the bubble, mouse up
+  // still sends the action.
+  SetUpUpdatePendingState(false);
+  EXPECT_CALL(*ui_controller(), UpdatePassword(_)).Times(0);
+  EXPECT_CALL(*ui_controller(), OnNopeUpdateClicked()).Times(0);
+  [controller() bubbleWillDisappear];
+  [delegate() setModel:nil];
+  [controller().updateButton performClick:nil];
+  [controller().noButton performClick:nil];
+}
+
 }  // namespace