[iOS] Add mutation lock to WebStateList.

This CL adds a mutation lock to WebStateList, to prevent observer
callbacks from changing or deleting the WSL while a change to the WSL
is in-flight.

Another approach would have been to make the WebStateList passed back to
observers a const WSL, but in many cases WSL observers already have
another pointer to the WSL which they use for mutations.

The lock implementation is straightforward, using a scoped lock object
to set/unset lock state on the WebStateList. This isn't threadsafe, but
WSL isn't, either. WSL will CHECK in the case of a double-lock, or of
destruction while locked.

Bug: 834263
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I3bf3512cec3297e7580cfc67c71bb12c05bc1cf5
Reviewed-on: https://chromium-review.googlesource.com/1065632
Commit-Queue: Mark Cogan <marq@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561045}

ios/chrome/browser/web_state_list/web_state_list.h

@@ -186,6 +186,12 @@ class WebStateList {
   // Index of the currently active WebState, kInvalidIndex if no such WebState.
   int active_index_ = kInvalidIndex;
 
+  // Lock to prevent observers from mutating or deleting the list while it is
+  // mutating.
+  // TODO(crbug.com/834263): Remove this lock and the code that uses it once
+  // the source of the crash is identified.
+  bool locked_ = false;
+
   DISALLOW_COPY_AND_ASSIGN(WebStateList);
 };
 

ios/chrome/browser/web_state_list/web_state_list.mm

@@ -7,6 +7,7 @@
 #include <algorithm>
 #include <utility>
 
+#include "base/auto_reset.h"
 #include "base/logging.h"
 #import "ios/chrome/browser/web_state_list/web_state_list_delegate.h"
 #import "ios/chrome/browser/web_state_list/web_state_list_observer.h"
@@ -101,6 +102,7 @@ WebStateList::WebStateList(WebStateListDelegate* delegate)
 }
 
 WebStateList::~WebStateList() {
+  CHECK(!locked_);
   CloseAllWebStates(CLOSE_NO_FLAGS);
 }
 
@@ -154,32 +156,40 @@ int WebStateList::InsertWebState(int index,
                                  std::unique_ptr<web::WebState> web_state,
                                  int insertion_flags,
                                  WebStateOpener opener) {
-  if (IsInsertionFlagSet(insertion_flags, INSERT_INHERIT_OPENER))
-    opener = WebStateOpener(GetActiveWebState());
+  const bool activating = IsInsertionFlagSet(insertion_flags, INSERT_ACTIVATE);
 
-  if (!IsInsertionFlagSet(insertion_flags, INSERT_FORCE_INDEX)) {
-    index = order_controller_->DetermineInsertionIndex(opener.opener);
-    if (index < 0 || count() < index)
-      index = count();
-  }
+  {
+    // Inner block for the mutation lock, because ActivateWebState might need to
+    // be called (if |activating| is true), and that method has its own mutation
+    // lock.
+    CHECK(!locked_);
+    base::AutoReset<bool> scoped_lock(&locked_, /* locked */ true);
+    if (IsInsertionFlagSet(insertion_flags, INSERT_INHERIT_OPENER))
+      opener = WebStateOpener(GetActiveWebState());
+
+    if (!IsInsertionFlagSet(insertion_flags, INSERT_FORCE_INDEX)) {
+      index = order_controller_->DetermineInsertionIndex(opener.opener);
+      if (index < 0 || count() < index)
+        index = count();
+    }
 
-  DCHECK(ContainsIndex(index) || index == count());
-  delegate_->WillAddWebState(web_state.get());
+    DCHECK(ContainsIndex(index) || index == count());
+    delegate_->WillAddWebState(web_state.get());
 
-  web::WebState* web_state_ptr = web_state.get();
-  web_state_wrappers_.insert(
-      web_state_wrappers_.begin() + index,
-      std::make_unique<WebStateWrapper>(std::move(web_state)));
+    web::WebState* web_state_ptr = web_state.get();
+    web_state_wrappers_.insert(
+        web_state_wrappers_.begin() + index,
+        std::make_unique<WebStateWrapper>(std::move(web_state)));
 
-  if (active_index_ >= index)
-    ++active_index_;
+    if (active_index_ >= index)
+      ++active_index_;
 
-  const bool activating = IsInsertionFlagSet(insertion_flags, INSERT_ACTIVATE);
-  for (auto& observer : observers_)
-    observer.WebStateInsertedAt(this, web_state_ptr, index, activating);
+    for (auto& observer : observers_)
+      observer.WebStateInsertedAt(this, web_state_ptr, index, activating);
 
-  if (opener.opener)
-    SetOpenerOfWebStateAt(index, opener);
+    if (opener.opener)
+      SetOpenerOfWebStateAt(index, opener);
+  }
 
   if (activating)
     ActivateWebStateAt(index);
@@ -188,6 +198,8 @@ int WebStateList::InsertWebState(int index,
 }
 
 void WebStateList::MoveWebStateAt(int from_index, int to_index) {
+  CHECK(!locked_);
+  base::AutoReset<bool> scoped_lock(&locked_, /* locked */ true);
   DCHECK(ContainsIndex(from_index));
   DCHECK(ContainsIndex(to_index));
   if (from_index == to_index)
@@ -241,6 +253,8 @@ std::unique_ptr<web::WebState> WebStateList::ReplaceWebStateAt(
 }
 
 std::unique_ptr<web::WebState> WebStateList::DetachWebStateAt(int index) {
+  CHECK(!locked_);
+  base::AutoReset<bool> scoped_lock(&locked_, /* locked */ true);
   DCHECK(ContainsIndex(index));
   int new_active_index = order_controller_->DetermineNewActiveIndex(index);
 
@@ -275,8 +289,11 @@ std::unique_ptr<web::WebState> WebStateList::DetachWebStateAt(int index) {
 }
 
 void WebStateList::CloseWebStateAt(int index, int close_flags) {
+  // Lock after detaching, since that has its own lock.
   auto detached_web_state = DetachWebStateAt(index);
 
+  CHECK(!locked_);
+  base::AutoReset<bool> scoped_lock(&locked_, /* locked */ true);
   const bool user_action = IsClosingFlagSet(close_flags, CLOSE_USER_ACTION);
   for (auto& observer : observers_) {
     observer.WillCloseWebStateAt(this, detached_web_state.get(), index,