[iOS] Avoid WebStateList reentrancy from GridViewController.

There are relatively frequent crashes (see bug) originating from
GridViewController inducing reentrancy into WebStateList. This happens
when a state-mutating method (in this case, ActivateWebStateAt()) is
called on WebStateList while it is still handling a different state-
mutating method call. The initial method call triggers observer methods,
then the observer method (usually very indirectly) calls another state-
mutating method.

Specifically for this bug, this sequence of events happens:
  1) Something causes a new tab to be inserted (there are many ways to do
     this).
  2) The tab insertion is observed by TabGridMediator, which calls
     the -insertItem:atIndex:selectedItemID: consumer method.
  3) The consumer (the GridViewController) eventually calls
     [UICollectionView performBatchUpdates:completion:].
  4) Roughly 24 stack frames of UIKit calls happen.
  5) [UICollectionViewDelegate -shouldSelectItemAtIndexPath:] is called.
  6) GridViewController subsequently calls (eventually) [TabGridMediator
     -selectItemWithID], which calls ActivateWebStateAt().

I don't have repro steps, so I haven't seen a crash with the symbols for
the UIKit stack frames in step (4), so I don't know what the collection
view is doing to make this happen. The collection view docs say that
-shouldSelectItemAtIndexPath: should *only* be called when the user taps
on a cell, and never when the cell is programmatically selected. But it
looks like that's still what's happening.

Absent repro steps, this CL puts two layers of speculative fixes.

First, TabGridMediator's selectItemWithID: method adds several early-
return possibilities; it will now early-return if the selected item is
already selected, and it will early-return if the web state list is
locked for mutation. Note that it's unclear what will happen in this
case, since the activation event might be triggering the tab being
opened.

Second, GridViewController sets an 'updating' flag while the collection
view batch update is in progress, and doesn't handle cell selection
while this flag is set.

Either one of these should fix the crash, but since there aren't repro
steps, I've added both.

Bug: 1134663
Change-Id: Ia838a5da9b763dbe8b82acc69957327d96259261
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2614432
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841560}

ios/chrome/browser/ui/tab_switcher/tab_grid/grid/grid_view_controller.mm

@@ -92,6 +92,9 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
 // horizontal to grid layout.
 @property(nonatomic, strong)
     UICollectionViewTransitionLayout* gridHorizontalTransitionLayout;
+
+// YES while batch updates and the batch update completion are being performed.
+@property(nonatomic) BOOL updating;
 @end
 
 @implementation GridViewController
@@ -836,11 +839,15 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
         (ProceduralBlockWithBool)collectionViewUpdatesCompletion {
   [self.collectionView
       performBatchUpdates:^{
+        self.updating = YES;
         // Synchronize model and view updates.
         modelUpdates();
         collectionViewUpdates();
       }
-               completion:collectionViewUpdatesCompletion];
+      completion:^(BOOL completed) {
+        collectionViewUpdatesCompletion(completed);
+        self.updating = NO;
+      }];
 }
 
 // Returns the index in |self.items| of the first item whose identifier is
@@ -889,6 +896,12 @@ NSIndexPath* CreateIndexPath(NSInteger index) {
     [self.delegate didTapPlusSignInGridViewController:self];
     return;
   }
+
+  // Speculative fix for crbug.com/1134663, where this method is called while
+  // updates from a tab insertion are processing.
+  if (self.updating)
+    return;
+
   NSUInteger index = base::checked_cast<NSUInteger>(indexPath.item);
   DCHECK_LT(index, self.items.count);
   NSString* itemID = self.items[index].identifier;

ios/chrome/browser/ui/tab_switcher/tab_grid/tab_grid_mediator.mm

@@ -82,7 +82,7 @@ NSString* GetActiveTabId(WebStateList* web_state_list) {
 }
 
 // Returns the index of the tab with |identifier| in |web_state_list|. Returns
-// -1 if not found.
+// WebStateList::kInvalidIndex if not found.
 int GetIndexOfTabWithId(WebStateList* web_state_list, NSString* identifier) {
   for (int i = 0; i < web_state_list->count(); i++) {
     web::WebState* web_state = web_state_list->GetWebStateAt(i);
@@ -90,7 +90,7 @@ int GetIndexOfTabWithId(WebStateList* web_state_list, NSString* identifier) {
     if ([identifier isEqualToString:tab_helper->tab_id()])
       return i;
   }
-  return -1;
+  return WebStateList::kInvalidIndex;
 }
 
 // Returns the WebState with |identifier| in |web_state_list|. Returns |nullptr|
@@ -241,8 +241,8 @@ web::WebState* GetWebStateWithId(WebStateList* web_state_list,
   if (webStateList->IsBatchInProgress())
     return;
   // If the selected index changes as a result of the last webstate being
-  // detached, atIndex will be -1.
-  if (atIndex == -1) {
+  // detached, atIndex will be kInvalidIndex.
+  if (atIndex == WebStateList::kInvalidIndex) {
     [self.consumer selectItemWithID:nil];
     return;
   }
@@ -301,19 +301,36 @@ web::WebState* GetWebStateWithId(WebStateList* web_state_list,
 
 - (void)moveItemWithID:(NSString*)itemID toIndex:(NSUInteger)destinationIndex {
   int sourceIndex = GetIndexOfTabWithId(self.webStateList, itemID);
-  if (sourceIndex >= 0)
+  if (sourceIndex != WebStateList::kInvalidIndex)
     self.webStateList->MoveWebStateAt(sourceIndex, destinationIndex);
 }
 
 - (void)selectItemWithID:(NSString*)itemID {
   int index = GetIndexOfTabWithId(self.webStateList, itemID);
-  if (index >= 0)
+
+  // Don't activate non-existent indexes.
+  if (index == WebStateList::kInvalidIndex)
+    return;
+
+  // Don't attempt a no-op activation. Normally this is not an issue, but it's
+  // possible that this method (-selectItemWithID:) is being called as part of
+  // a WebStateListObserver callback, in which case even a no-op activation
+  // will cause a CHECK().
+  if (index == self.webStateList->active_index())
+    return;
+
+  // Avoid a reentrant activation. This is a fix for crbug.com/1134663, although
+  // ignoring the slection at this point may do weird things.
+  if (self.webStateList->IsMutating())
+    return;
+
+  // It should be safe to activate here.
   self.webStateList->ActivateWebStateAt(index);
 }
 
 - (void)closeItemWithID:(NSString*)itemID {
   int index = GetIndexOfTabWithId(self.webStateList, itemID);
-  if (index >= 0)
+  if (index != WebStateList::kInvalidIndex)
     self.webStateList->CloseWebStateAt(index, WebStateList::CLOSE_USER_ACTION);
 }
 
@@ -456,7 +473,7 @@ web::WebState* GetWebStateWithId(WebStateList* web_state_list,
     }
     // Reorder tab within same grid.
     int sourceIndex = GetIndexOfTabWithId(self.webStateList, tabInfo.tabID);
-    if (sourceIndex >= 0)
+    if (sourceIndex != WebStateList::kInvalidIndex)
       self.webStateList->MoveWebStateAt(sourceIndex, destinationIndex);
     return;
   }