[iOS] Prevent dispatcher "unrecognized selector" crashes on shutdown.

Puts dispatcher to a special mode where it pretends to recognize all
selectors and silently fail on shutdown.

Bug: 1137686, 1163129
Change-Id: Ib065d7f564f22c5be2285201d5dcd9dea39f60d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2472039
Commit-Queue: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Auto-Submit: Stepan Khapugin <stkhapugin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#844126}

ios/chrome/app/application_delegate/app_state.mm

@@ -435,6 +435,8 @@ initWithBrowserLauncher:(id<BrowserLauncher>)browserLauncher
   }
   _appIsTerminating = YES;
 
+  [_appCommandDispatcher prepareForShutdown];
+
   // Cancel any in-flight distribution notifications.
   CHECK(ios::GetChromeBrowserProvider());
   ios::GetChromeBrowserProvider()

ios/chrome/browser/ui/commands/command_dispatcher.h

@@ -65,6 +65,13 @@
 // otherwise.
 - (CommandDispatcher*)strictCallableForProtocol:(Protocol*)protocol;
 
+// After this method is called, -stopDispatching methods will stop dispatching,
+// but this object will continue to respond to registered selectors by silently
+// failing. This method should be called on -applicationWillTerminate. It helps
+// avoid untangling the dispatcher chains in the correct order, which sometimes
+// can be very hard.
+- (void)prepareForShutdown;
+
 @end
 
 #endif  // IOS_CHROME_BROWSER_UI_COMMANDS_COMMAND_DISPATCHER_H_

ios/chrome/browser/ui/commands/command_dispatcher.mm

@@ -15,13 +15,48 @@
 #error "This file requires ARC support."
 #endif
 
+#pragma mark - SilentlyFailingObject
+
+// Object that responds to any selector and does nothing when its called.
+// Used as a "nice OCMock" equivalent for CommandDispatcher that's preparing for
+// shutdown.
+@interface SilentlyFailingObject : NSProxy
+@end
+@implementation SilentlyFailingObject
+
+- (instancetype)init {
+  return self;
+}
+
+- (void)forwardInvocation:(NSInvocation*)invocation {
+}
+
+- (NSMethodSignature*)methodSignatureForSelector:(SEL)selector {
+  // Return some method signature to silence errors.
+  // Here it's (void)(self, _cmd).
+  return [NSMethodSignature signatureWithObjCTypes:"v@:"];
+}
+
+@end
+
+#pragma mark - CommandDispatcher
+
 @implementation CommandDispatcher {
   // Stores which target to forward to for a given selector.
   std::unordered_map<SEL, __weak id> _forwardingTargets;
+
+  // Stores remembered targets while preparing for shutdown.
+  std::unordered_map<SEL, __weak id> _silentlyFailingTargets;
+
+  // Tracks if preparing for shutdown has been requested.
+  // This is an ivar, not a property, to avoid having synthesized getter/setter
+  // methods.
+  BOOL _preparingForShutdown;
 }
 
 - (void)startDispatchingToTarget:(id)target forSelector:(SEL)selector {
   DCHECK(![self targetForSelector:selector]);
+  DCHECK(![self shouldFailSilentlyForSelector:selector]);
 
   _forwardingTargets[selector] = target;
 }
@@ -40,6 +75,10 @@
 }
 
 - (void)stopDispatchingForSelector:(SEL)selector {
+  if (_preparingForShutdown) {
+    id target = _forwardingTargets[selector];
+    _silentlyFailingTargets[selector] = target;
+  }
   _forwardingTargets.erase(selector);
 }
 
@@ -85,7 +124,9 @@
   BOOL conforming = YES;
   for (unsigned int i = 0; i < methodCount; i++) {
     SEL selector = requiredInstanceMethods[i].name;
-    if (_forwardingTargets.find(selector) == _forwardingTargets.end()) {
+    BOOL targetFound =
+        _forwardingTargets.find(selector) != _forwardingTargets.end();
+    if (!targetFound && ![self shouldFailSilentlyForSelector:selector]) {
       conforming = NO;
       break;
     }
@@ -114,6 +155,10 @@
   return self;
 }
 
+- (void)prepareForShutdown {
+  _preparingForShutdown = YES;
+}
+
 #pragma mark - NSObject
 
 // Overridden to forward messages to registered handlers.
@@ -149,9 +194,18 @@
 // Returns the target registered to receive messeages for |selector|.
 - (id)targetForSelector:(SEL)selector {
   auto target = _forwardingTargets.find(selector);
-  if (target == _forwardingTargets.end())
+  if (target == _forwardingTargets.end()) {
+    if ([self shouldFailSilentlyForSelector:selector]) {
+      return [[SilentlyFailingObject alloc] init];
+    }
     return nil;
+  }
   return target->second;
 }
 
+- (BOOL)shouldFailSilentlyForSelector:(SEL)selector {
+  return _preparingForShutdown && _silentlyFailingTargets.find(selector) !=
+                                      _silentlyFailingTargets.end();
+}
+
 @end

ios/chrome/browser/ui/commands/command_dispatcher_unittest.mm

@@ -355,6 +355,70 @@ TEST_F(CommandDispatcherTest, NoTargetRegisteredForSelector) {
   EXPECT_TRUE(exception_caught);
 }
 
+// Tests that an exception is not thrown when prepareForShutdown was called
+// before the target was removed.
+TEST_F(CommandDispatcherTest,
+       PrepareForShutdownLetsStopDispatchingForSelectorFailSilently) {
+  id dispatcher = [[CommandDispatcher alloc] init];
+  CommandDispatcherTestSimpleTarget* target =
+      [[CommandDispatcherTestSimpleTarget alloc] init];
+
+  // Check stopDispatchingForSelector:
+  [dispatcher startDispatchingToTarget:target forSelector:@selector(show)];
+  [dispatcher prepareForShutdown];
+  [dispatcher stopDispatchingForSelector:@selector(show)];
+  bool exception_caught = false;
+  @try {
+    [dispatcher show];
+  } @catch (NSException* exception) {
+    exception_caught = true;
+  }
+
+  EXPECT_FALSE(exception_caught);
+}
+
+TEST_F(CommandDispatcherTest,
+       PrepareForShutdownLetsStopDispatchingForProtocolFailSilently) {
+  id dispatcher = [[CommandDispatcher alloc] init];
+  CommandDispatcherTestSimpleTarget* target =
+      [[CommandDispatcherTestSimpleTarget alloc] init];
+
+  // Check stopDispatchingForProtocol:
+  [dispatcher startDispatchingToTarget:target
+                           forProtocol:@protocol(HideProtocol)];
+  [dispatcher prepareForShutdown];
+  [dispatcher stopDispatchingForProtocol:@protocol(HideProtocol)];
+  bool exception_caught = false;
+  @try {
+    [dispatcher hide];
+  } @catch (NSException* exception) {
+    exception_caught = true;
+  }
+
+  EXPECT_FALSE(exception_caught);
+}
+
+TEST_F(CommandDispatcherTest,
+       PrepareForShutdownLetsStopDispatchingToTargetFailSilently) {
+  id dispatcher = [[CommandDispatcher alloc] init];
+  CommandDispatcherTestSimpleTarget* target =
+      [[CommandDispatcherTestSimpleTarget alloc] init];
+
+  // Check stopDispatchingToTarget:
+  [dispatcher startDispatchingToTarget:target
+                           forProtocol:@protocol(HideProtocol)];
+  [dispatcher prepareForShutdown];
+  [dispatcher stopDispatchingToTarget:target];
+  bool exception_caught = false;
+  @try {
+    [dispatcher hide];
+  } @catch (NSException* exception) {
+    exception_caught = true;
+  }
+
+  EXPECT_FALSE(exception_caught);
+}
+
 // Tests that -respondsToSelector returns YES for methods once they are
 // dispatched for.
 // Tests handler methods with no arguments.

ios/chrome/browser/ui/main/browser_view_wrangler.mm

@@ -312,6 +312,11 @@
   DCHECK(!_isShutdown);
   _isShutdown = YES;
 
+  [self.mainBrowser->GetCommandDispatcher() prepareForShutdown];
+  if ([self hasIncognitoInterface]) {
+    [self.otrBrowser->GetCommandDispatcher() prepareForShutdown];
+  }
+
   // At this stage, new BrowserCoordinators shouldn't be lazily constructed by
   // calling their property getters.
   [_mainBrowserCoordinator stop];