[iOS] Wait until the view controller is presented before dismiss

Chrome can be started with the 2 following events:
+ Open the sign-in upgrade promo
+ Open an URL (from an external app)

The opening the URL interrupts the sign-in promo, while its view
controller is not fully presented yet.

This patch prevent from the coordinator from dismissing its view
controller before it is fully on the screen.

Fixed: 1126170
Change-Id: I03287c9f830f650c10aa2476d6d693af3783c553
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2593107
Commit-Queue: Jérôme Lebel <jlebel@chromium.org>
Auto-Submit: Jérôme Lebel <jlebel@chromium.org>
Reviewed-by: Nohemi Fernandez <fernandex@chromium.org>
Cr-Commit-Position: refs/heads/master@{#837147}

ios/chrome/browser/ui/authentication/signin/user_signin/BUILD.gn

@@ -51,7 +51,10 @@ source_set("user_signin") {
 source_set("unit_tests") {
   configs += [ "//build/config/compiler:enable_arc" ]
   testonly = true
-  sources = [ "user_signin_mediator_unittest.mm" ]
+  sources = [
+    "user_signin_coordinator_unittest.mm",
+    "user_signin_mediator_unittest.mm",
+  ]
   deps = [
     ":user_signin",
     "//base/test:test_support",
@@ -70,6 +73,7 @@ source_set("unit_tests") {
     "//ios/chrome/browser/sync",
     "//ios/chrome/browser/sync:test_support",
     "//ios/chrome/browser/ui/authentication",
+    "//ios/chrome/browser/ui/authentication/signin/user_signin/logging",
     "//ios/chrome/browser/unified_consent",
     "//ios/public/provider/chrome/browser/signin",
     "//ios/public/provider/chrome/browser/signin:fake_chrome_identity",

ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_coordinator.mm

@@ -6,6 +6,7 @@
 #import "ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_coordinator.h"
 
 #import "base/feature_list.h"
+#import "base/ios/block_types.h"
 #import "base/mac/foundation_util.h"
 #import "ios/chrome/browser/main/browser.h"
 #import "ios/chrome/browser/signin/authentication_service.h"
@@ -49,7 +50,8 @@ const CGFloat kFadeOutAnimationDuration = 0.16f;
 @property(nonatomic, strong)
     SigninCoordinator* advancedSettingsSigninCoordinator;
 // View controller that handles the sign-in UI.
-@property(nonatomic, strong) UserSigninViewController* viewController;
+@property(nonatomic, strong, readwrite)
+    UserSigninViewController* viewController;
 // Mediator that handles the sign-in authentication state.
 @property(nonatomic, strong) UserSigninMediator* mediator;
 // Suggested identity shown at sign-in.
@@ -60,6 +62,10 @@ const CGFloat kFadeOutAnimationDuration = 0.16f;
 @property(nonatomic, assign, readonly) UserSigninIntent signinIntent;
 // Whether an account has been added during sign-in flow.
 @property(nonatomic, assign) BOOL addedAccount;
+// YES if the view controller started the presenting animation.
+@property(nonatomic, assign) BOOL viewControllerPresentingAnimation;
+// Callback to be invoked when the view controller presenting animation is done.
+@property(nonatomic, copy) ProceduralBlock interruptCallback;
 
 @end
 
@@ -112,7 +118,7 @@ const CGFloat kFadeOutAnimationDuration = 0.16f;
          !authenticationService->IsAuthenticated() ||
          self.signinIntent == UserSigninIntentFirstRun);
   [super start];
-  self.viewController = [[UserSigninViewController alloc] init];
+  self.viewController = [self generateUserSigninViewController];
   self.viewController.delegate = self;
   self.viewController.useFirstRunSkipButton =
       self.signinIntent == UserSigninIntentFirstRun;
@@ -424,8 +430,6 @@ const CGFloat kFadeOutAnimationDuration = 0.16f;
       break;
     }
     case UserSigninIntentUpgrade: {
-      DCHECK(self.baseViewController);
-
       // Avoid presenting the promo if the current device orientation is not
       // supported. The promo will be presented at a later moment, when the
       // device orientation is supported.
@@ -442,29 +446,58 @@ const CGFloat kFadeOutAnimationDuration = 0.16f;
                        showAdvancedSettingsSignin:NO];
         return;
       }
-
-      [self.baseViewController presentViewController:self.viewController
-                                            animated:YES
-                                          completion:nil];
+      [self presentUserViewControllerToBaseViewController];
       break;
     }
     case UserSigninIntentSignin: {
-      DCHECK(self.baseViewController);
-
-      [self.baseViewController presentViewController:self.viewController
-                                            animated:YES
-                                          completion:nil];
+      [self presentUserViewControllerToBaseViewController];
       break;
     }
   }
 }
 
+// Presents |self.viewController|. This method is only relevant when
+// |self.signinIntent| is not UserSigninIntentFirstRun.
+- (void)presentUserViewControllerToBaseViewController {
+  DCHECK_NE(UserSigninIntentFirstRun, self.signinIntent);
+  DCHECK(self.baseViewController);
+  self.viewControllerPresentingAnimation = YES;
+  __weak __typeof(self) weakSelf = self;
+  ProceduralBlock completion = ^{
+    weakSelf.viewControllerPresentingAnimation = NO;
+    if (weakSelf.interruptCallback) {
+      // The view controller is fully presented, the coordinator
+      // can be dismissed. UIKit doesn't allow a view controller
+      // to be dismissed during the animation.
+      // See crbug.com/1126170
+      ProceduralBlock interruptCallback = weakSelf.interruptCallback;
+      weakSelf.interruptCallback = nil;
+      interruptCallback();
+    }
+  };
+  [self.baseViewController presentViewController:self.viewController
+                                        animated:YES
+                                      completion:completion];
+}
+
 // Interrupts the sign-in when |self.viewController| is presented, by dismissing
 // it if needed (according to |action|). Then |completion| is called.
 // This method should not be called if |self.addAccountSigninCoordinator| has
 // not been stopped before.
 - (void)interruptUserSigninUIWithAction:(SigninCoordinatorInterruptAction)action
                              completion:(ProceduralBlock)completion {
+  if (self.viewControllerPresentingAnimation) {
+    // UIKit doesn't allow a view controller to be dismissed during the
+    // animation. The interruption has to be processed when the view controller
+    // will be fully presented.
+    // See crbug.com/1126170
+    DCHECK(!self.interruptCallback);
+    __weak __typeof(self) weakSelf = self;
+    self.interruptCallback = ^() {
+      [weakSelf interruptUserSigninUIWithAction:action completion:completion];
+    };
+    return;
+  }
   DCHECK(self.viewController);
   DCHECK(self.mediator);
   DCHECK(self.unifiedConsentCoordinator);
@@ -579,4 +612,12 @@ const CGFloat kFadeOutAnimationDuration = 0.16f;
   }
 }
 
+#pragma mark - Methods for unittests
+
+// Returns a UserSigninViewController instance. This method is overriden for
+// unittests.
+- (UserSigninViewController*)generateUserSigninViewController {
+  return [[UserSigninViewController alloc] init];
+}
+
 @end

ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_coordinator_unittest.mm

+// Copyright 2020 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.
+
+#import "ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_mediator.h"
+
+#import <UIKit/UIKit.h>
+
+#import "base/ios/block_types.h"
+#import "ios/chrome/browser/browser_state/test_chrome_browser_state.h"
+#import "ios/chrome/browser/main/test_browser.h"
+#import "ios/chrome/browser/signin/authentication_service_factory.h"
+#import "ios/chrome/browser/signin/authentication_service_fake.h"
+#import "ios/chrome/browser/ui/authentication/signin/user_signin/logging/user_signin_logger.h"
+#import "ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_coordinator.h"
+#import "ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_view_controller.h"
+#import "ios/web/public/test/web_task_environment.h"
+#import "testing/gmock/include/gmock/gmock.h"
+#import "testing/platform_test.h"
+#import "third_party/ocmock/OCMock/OCMock.h"
+#import "third_party/ocmock/gtest_support.h"
+#import "third_party/ocmock/ocmock_extensions.h"
+
+#if !defined(__has_feature) || !__has_feature(objc_arc)
+#error "This file requires ARC support."
+#endif
+
+@interface TestUserSigninCoordinator : UserSigninCoordinator
+
+@property(nonatomic, strong)
+    UserSigninViewController* userSigninViewControllerMock;
+
+@end
+
+@implementation TestUserSigninCoordinator
+
+- (UserSigninViewController*)generateUserSigninViewController {
+  return self.userSigninViewControllerMock;
+}
+
+@end
+
+class UserSigninCoordinatorTest : public PlatformTest {
+ public:
+  UserSigninCoordinatorTest() {}
+
+  void SetUp() override {
+    PlatformTest::SetUp();
+
+    TestChromeBrowserState::Builder builder;
+    builder.AddTestingFactory(
+        AuthenticationServiceFactory::GetInstance(),
+        base::BindRepeating(
+            &AuthenticationServiceFake::CreateAuthenticationService));
+    browser_state_ = builder.Build();
+    WebStateList* web_state_list = nullptr;
+    browser_ =
+        std::make_unique<TestBrowser>(browser_state_.get(), web_state_list);
+
+    SetupLoggerMock();
+    SetupUserSigninViewControllerMock();
+    SetupBaseViewControllerMock();
+    coordinator_ = [[TestUserSigninCoordinator alloc]
+        initWithBaseViewController:base_view_controller_mock_
+                           browser:browser_.get()
+                          identity:nil
+                      signinIntent:UserSigninIntentUpgrade
+                            logger:logger_mock_];
+    coordinator_.userSigninViewControllerMock =
+        user_signin_view_controller_mock_;
+  }
+
+  void TearDown() override {
+    EXPECT_OCMOCK_VERIFY((id)base_view_controller_mock_);
+    EXPECT_OCMOCK_VERIFY((id)logger_mock_);
+    EXPECT_OCMOCK_VERIFY((id)user_signin_view_controller_mock_);
+    PlatformTest::TearDown();
+  }
+
+  // Sets up the logger mock.
+  void SetupLoggerMock() {
+    DCHECK(!logger_mock_);
+    logger_mock_ = OCMStrictClassMock([UserSigninLogger class]);
+    OCMStub([logger_mock_ promoAction])
+        .andReturn(signin_metrics::PromoAction::PROMO_ACTION_WITH_DEFAULT);
+    OCMExpect([logger_mock_ logSigninStarted]);
+  }
+
+  // Sets up the base view controller mock. Using a view controller mock gives
+  // a fine-grained control to simulate UIKit timing when completion blocks are
+  // called.
+  void SetupBaseViewControllerMock() {
+    base_view_controller_mock_ = OCMStrictClassMock([UIViewController class]);
+    id present_completion_handler =
+        [OCMArg checkWithBlock:^(ProceduralBlock completion) {
+          EXPECT_EQ(nil, view_controller_present_completion_);
+          view_controller_present_completion_ = [completion copy];
+          return YES;
+        }];
+    OCMExpect([base_view_controller_mock_
+        presentViewController:user_signin_view_controller_mock_
+                     animated:YES
+                   completion:present_completion_handler]);
+  }
+
+  // Sets up the user sign-in view controller.
+  void SetupUserSigninViewControllerMock() {
+    user_signin_view_controller_mock_ =
+        OCMStrictClassMock([UserSigninViewController class]);
+    OCMExpect([user_signin_view_controller_mock_ setDelegate:[OCMArg any]]);
+    OCMExpect([user_signin_view_controller_mock_ setUseFirstRunSkipButton:NO]);
+    OCMExpect([user_signin_view_controller_mock_
+        setUnifiedConsentViewController:[OCMArg any]]);
+    OCMExpect([user_signin_view_controller_mock_
+        setModalPresentationStyle:UIModalPresentationFormSheet]);
+    OCMExpect([user_signin_view_controller_mock_ presentationController])
+        .andDo(^(NSInvocation* invocation) {
+          id returnValue = nil;
+          [invocation setReturnValue:&returnValue];
+        });
+    OCMExpect(
+        [user_signin_view_controller_mock_ supportedInterfaceOrientations])
+        .andReturn(UIInterfaceOrientationMaskAll);
+    id dismiss_completion_handler =
+        [OCMArg checkWithBlock:^(ProceduralBlock completion) {
+          EXPECT_EQ(nil, view_controller_dismiss_completion_);
+          view_controller_dismiss_completion_ = [completion copy];
+          return YES;
+        }];
+    OCMExpect([user_signin_view_controller_mock_
+        dismissViewControllerAnimated:YES
+                           completion:dismiss_completion_handler]);
+  }
+
+ protected:
+  // Needed for test browser state created by TestChromeBrowserState().
+  web::WebTaskEnvironment task_environment_;
+
+  std::unique_ptr<Browser> browser_;
+  std::unique_ptr<TestChromeBrowserState> browser_state_;
+
+  // UserSigninCoordinator to test.
+  TestUserSigninCoordinator* coordinator_ = nil;
+
+  // Procedure to finish the view controller presentation animation.
+  ProceduralBlock view_controller_present_completion_ = nil;
+  // Procedure to finish the dismiss view controller animation.
+  ProceduralBlock view_controller_dismiss_completion_ = nil;
+
+  // Mocks
+  UIViewController* base_view_controller_mock_ = nil;
+  UserSigninLogger* logger_mock_ = nil;
+  UserSigninViewController* user_signin_view_controller_mock_ = nil;
+};
+
+// Tests a sequence of start and interrupt the coordinator.
+TEST_F(UserSigninCoordinatorTest, StartAndInterruptCoordinator) {
+  __block bool completion_done = false;
+  __block bool interrupt_done = false;
+  coordinator_.signinCompletion =
+      ^(SigninCoordinatorResult signinResult,
+        SigninCompletionInfo* signinCompletionInfo) {
+        EXPECT_FALSE(completion_done);
+        EXPECT_FALSE(interrupt_done);
+        EXPECT_EQ(SigninCoordinatorResultInterrupted, signinResult);
+        completion_done = true;
+      };
+  [coordinator_ start];
+  EXPECT_NE(nil, view_controller_present_completion_);
+  [coordinator_
+      interruptWithAction:SigninCoordinatorInterruptActionDismissWithAnimation
+               completion:^{
+                 EXPECT_TRUE(completion_done);
+                 EXPECT_FALSE(interrupt_done);
+                 interrupt_done = true;
+               }];
+  // The view controller should not be dismissed yet.
+  EXPECT_EQ(nil, view_controller_dismiss_completion_);
+  // Simulate the end of -[UIViewController
+  // presentViewController:animated:completion] by calling the completion block.
+  view_controller_present_completion_();
+  EXPECT_FALSE(interrupt_done);
+  EXPECT_FALSE(completion_done);
+  // Dismiss method is expected to be called.
+  EXPECT_NE(nil, view_controller_dismiss_completion_);
+  // Simulate the end of -[UIViewController dismissViewController:] by calling
+  // the completion block.
+  view_controller_dismiss_completion_();
+  EXPECT_TRUE(interrupt_done);
+  EXPECT_TRUE(completion_done);
+}

ios/chrome/browser/ui/authentication/signin/user_signin/user_signin_view_controller.mm

@@ -192,6 +192,7 @@ enum AuthenticationButtonType {
 
 - (void)viewDidLoad {
   [super viewDidLoad];
+  DCHECK(self.unifiedConsentViewController);
   self.view.backgroundColor = self.systemBackgroundColor;
 
   self.containerView = [[UIView alloc] init];