[ios] Add lock preventing nest batch operation on WebStateList

WebStateList is not re-entrant (already asserted by locked_)
so there is no point nesting batched operations. Add a bool
to ensure this is not the case.

Add a method to check whether a batched operation is in
progress (could be used internally to also omit sending
notification during a batched operation).

Bug: 1014526
Change-Id: I6b6dc3278312895a32898d898245f538f9a5264d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2089675
Commit-Queue: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#750916}

ios/chrome/browser/web_state_list/web_state_list.h

@@ -85,6 +85,9 @@ class WebStateList {
   // Returns true if the list is currently mutating.
   bool IsMutating() const;
 
+  // Returns true if a batch operation is in progress.
+  bool IsBatchInProgress() const;
+
   // Returns the currently active WebState or null if there is none.
   web::WebState* GetActiveWebState() const;
 
@@ -276,6 +279,9 @@ class WebStateList {
   // by the returned base::AutoReset<bool>).
   bool locked_ = false;
 
+  // Lock to prevent nesting batched operations.
+  bool batch_operation_in_progress_ = false;
+
   SEQUENCE_CHECKER(sequence_checker_);
 
   DISALLOW_COPY_AND_ASSIGN(WebStateList);

ios/chrome/browser/web_state_list/web_state_list.mm

@@ -132,6 +132,11 @@ bool WebStateList::IsMutating() const {
   return locked_;
 }
 
+bool WebStateList::IsBatchInProgress() const {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  return batch_operation_in_progress_;
+}
+
 web::WebState* WebStateList::GetActiveWebState() const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (active_index_ != kInvalidIndex)
@@ -451,6 +456,10 @@ void WebStateList::RemoveObserver(WebStateListObserver* observer) {
 void WebStateList::PerformBatchOperation(
     base::OnceCallback<void(WebStateList*)> operation) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+
+  DCHECK(!batch_operation_in_progress_);
+  base::AutoReset<bool> lock(&batch_operation_in_progress_, /*locked=*/true);
+
   for (auto& observer : observers_)
     observer.WillBeginBatchOperation(this);
   if (!operation.is_null())

ios/chrome/browser/web_state_list/web_state_list_unittest.mm

@@ -201,7 +201,7 @@ class WebStateListTest : public PlatformTest {
   DISALLOW_COPY_AND_ASSIGN(WebStateListTest);
 };
 
-// Test that empty() matches count() != 0.
+// Tests that empty() matches count() != 0.
 TEST_F(WebStateListTest, IsEmpty) {
   EXPECT_EQ(0, web_state_list_.count());
   EXPECT_TRUE(web_state_list_.empty());
@@ -213,7 +213,7 @@ TEST_F(WebStateListTest, IsEmpty) {
   EXPECT_FALSE(web_state_list_.empty());
 }
 
-// Test that inserting a single webstate works.
+// Tests that inserting a single webstate works.
 TEST_F(WebStateListTest, InsertUrlSingle) {
   AppendNewWebState(kURL0);
 
@@ -222,7 +222,7 @@ TEST_F(WebStateListTest, InsertUrlSingle) {
   EXPECT_EQ(kURL0, web_state_list_.GetWebStateAt(0)->GetVisibleURL().spec());
 }
 
-// Test that inserting multiple webstates puts them in the expected places.
+// Tests that inserting multiple webstates puts them in the expected places.
 TEST_F(WebStateListTest, InsertUrlMultiple) {
   web_state_list_.InsertWebState(0, CreateWebState(kURL0),
                                  WebStateList::INSERT_FORCE_INDEX,
@@ -241,7 +241,7 @@ TEST_F(WebStateListTest, InsertUrlMultiple) {
   EXPECT_EQ(kURL0, web_state_list_.GetWebStateAt(2)->GetVisibleURL().spec());
 }
 
-// Test webstate activation.
+// Tests webstate activation.
 TEST_F(WebStateListTest, ActivateWebState) {
   AppendNewWebState(kURL0);
   EXPECT_EQ(nullptr, web_state_list_.GetActiveWebState());
@@ -254,7 +254,7 @@ TEST_F(WebStateListTest, ActivateWebState) {
             web_state_list_.GetActiveWebState());
 }
 
-// Test activating a webstate as it is inserted.
+// Tests activating a webstate as it is inserted.
 TEST_F(WebStateListTest, InsertActivate) {
   web_state_list_.InsertWebState(
       0, CreateWebState(kURL0),
@@ -267,7 +267,7 @@ TEST_F(WebStateListTest, InsertActivate) {
             web_state_list_.GetActiveWebState());
 }
 
-// Test finding a known webstate.
+// Tests finding a known webstate.
 TEST_F(WebStateListTest, GetIndexOfWebState) {
   std::unique_ptr<web::TestWebState> web_state_0 = CreateWebState(kURL0);
   web::WebState* target_web_state = web_state_0.get();
@@ -295,7 +295,7 @@ TEST_F(WebStateListTest, GetIndexOfWebState) {
   EXPECT_EQ(2, web_state_list_.GetIndexOfWebState(target_web_state));
 }
 
-// Test finding a webstate by URL.
+// Tests finding a webstate by URL.
 TEST_F(WebStateListTest, GetIndexOfWebStateWithURL) {
   // Empty list.
   EXPECT_EQ(WebStateList::kInvalidIndex,
@@ -316,7 +316,7 @@ TEST_F(WebStateListTest, GetIndexOfWebStateWithURL) {
   EXPECT_EQ(1, web_state_list_.GetIndexOfWebStateWithURL(GURL(kURL0)));
 }
 
-// Test finding a non-active webstate by URL.
+// Tests finding a non-active webstate by URL.
 TEST_F(WebStateListTest, GetIndexOfInactiveWebStateWithURL) {
   // Empty list.
   EXPECT_EQ(WebStateList::kInvalidIndex,
@@ -356,7 +356,7 @@ TEST_F(WebStateListTest, GetIndexOfInactiveWebStateWithURL) {
   EXPECT_EQ(2, web_state_list_.GetIndexOfInactiveWebStateWithURL(GURL(kURL0)));
 }
 
-// Test that inserted webstates correctly inherit openers.
+// Tests that inserted webstates correctly inherit openers.
 TEST_F(WebStateListTest, InsertInheritOpener) {
   AppendNewWebState(kURL0);
   web_state_list_.ActivateWebStateAt(0);
@@ -374,7 +374,7 @@ TEST_F(WebStateListTest, InsertInheritOpener) {
             web_state_list_.GetOpenerOfWebStateAt(1).opener);
 }
 
-// Test moving webstates one place to the "right" (to a higher index).
+// Tests moving webstates one place to the "right" (to a higher index).
 TEST_F(WebStateListTest, MoveWebStateAtRightByOne) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -396,7 +396,8 @@ TEST_F(WebStateListTest, MoveWebStateAtRightByOne) {
   EXPECT_EQ(kURL2, web_state_list_.GetWebStateAt(2)->GetVisibleURL().spec());
 }
 
-// Test moving webstates more than one place to the "right" (to a higher index).
+// Tests moving webstates more than one place to the "right" (to a higher
+// index).
 TEST_F(WebStateListTest, MoveWebStateAtRightByMoreThanOne) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -418,7 +419,7 @@ TEST_F(WebStateListTest, MoveWebStateAtRightByMoreThanOne) {
   EXPECT_EQ(kURL0, web_state_list_.GetWebStateAt(2)->GetVisibleURL().spec());
 }
 
-// Test moving webstates one place to the "left" (to a lower index).
+// Tests moving webstates one place to the "left" (to a lower index).
 TEST_F(WebStateListTest, MoveWebStateAtLeftByOne) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -440,7 +441,7 @@ TEST_F(WebStateListTest, MoveWebStateAtLeftByOne) {
   EXPECT_EQ(kURL1, web_state_list_.GetWebStateAt(2)->GetVisibleURL().spec());
 }
 
-// Test moving webstates more than one place to the "left" (to a lower index).
+// Tests moving webstates more than one place to the "left" (to a lower index).
 TEST_F(WebStateListTest, MoveWebStateAtLeftByMoreThanOne) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -462,7 +463,7 @@ TEST_F(WebStateListTest, MoveWebStateAtLeftByMoreThanOne) {
   EXPECT_EQ(kURL1, web_state_list_.GetWebStateAt(2)->GetVisibleURL().spec());
 }
 
-// Test "moving" webstates (calling MoveWebStateAt with the same source and
+// Tests "moving" webstates (calling MoveWebStateAt with the same source and
 // destination indexes.
 TEST_F(WebStateListTest, MoveWebStateAtSameIndex) {
   AppendNewWebState(kURL0);
@@ -485,7 +486,7 @@ TEST_F(WebStateListTest, MoveWebStateAtSameIndex) {
   EXPECT_EQ(kURL2, web_state_list_.GetWebStateAt(2)->GetVisibleURL().spec());
 }
 
-// Test replacing webstates.
+// Tests replacing webstates.
 TEST_F(WebStateListTest, ReplaceWebStateAt) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -507,7 +508,7 @@ TEST_F(WebStateListTest, ReplaceWebStateAt) {
   EXPECT_EQ(kURL1, old_web_state->GetVisibleURL().spec());
 }
 
-// Test detaching webstates at index 0.
+// Tests detaching webstates at index 0.
 TEST_F(WebStateListTest, DetachWebStateAtIndexBegining) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -528,7 +529,7 @@ TEST_F(WebStateListTest, DetachWebStateAtIndexBegining) {
   EXPECT_EQ(kURL2, web_state_list_.GetWebStateAt(1)->GetVisibleURL().spec());
 }
 
-// Test detaching webstates at an index that isn't 0 or the last index.
+// Tests detaching webstates at an index that isn't 0 or the last index.
 TEST_F(WebStateListTest, DetachWebStateAtIndexMiddle) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -549,7 +550,7 @@ TEST_F(WebStateListTest, DetachWebStateAtIndexMiddle) {
   EXPECT_EQ(kURL2, web_state_list_.GetWebStateAt(1)->GetVisibleURL().spec());
 }
 
-// Test detaching webstates at the last index.
+// Tests detaching webstates at the last index.
 TEST_F(WebStateListTest, DetachWebStateAtIndexLast) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -570,7 +571,7 @@ TEST_F(WebStateListTest, DetachWebStateAtIndexLast) {
   EXPECT_EQ(kURL1, web_state_list_.GetWebStateAt(1)->GetVisibleURL().spec());
 }
 
-// Test finding opended-by indexes on an empty list.
+// Tests finding opended-by indexes on an empty list.
 TEST_F(WebStateListTest, OpenersEmptyList) {
   EXPECT_TRUE(web_state_list_.empty());
 
@@ -589,7 +590,7 @@ TEST_F(WebStateListTest, OpenersEmptyList) {
                 nullptr, WebStateList::kInvalidIndex, true));
 }
 
-// Test finding opended-by indexes when no webstates have been opened.
+// Tests finding opended-by indexes when no webstates have been opened.
 TEST_F(WebStateListTest, OpenersNothingOpened) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -613,7 +614,7 @@ TEST_F(WebStateListTest, OpenersNothingOpened) {
   }
 }
 
-// Test finding opended-by indexes when the opened child is at an index after
+// Tests finding opended-by indexes when the opened child is at an index after
 // the parent.
 TEST_F(WebStateListTest, OpenersChildsAfterOpener) {
   AppendNewWebState(kURL0);
@@ -667,7 +668,7 @@ TEST_F(WebStateListTest, OpenersChildsAfterOpener) {
                    opener, start_index, true));
 }
 
-// Test finding opended-by indexes when the opened child is at an index before
+// Tests finding opended-by indexes when the opened child is at an index before
 // the parent.
 TEST_F(WebStateListTest, OpenersChildsBeforeOpener) {
   AppendNewWebState(kURL0);
@@ -684,7 +685,7 @@ TEST_F(WebStateListTest, OpenersChildsBeforeOpener) {
                    opener, start_index, false));
 }
 
-// Test closing all webstates.
+// Tests closing all webstates.
 TEST_F(WebStateListTest, CloseAllWebStates) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -703,7 +704,7 @@ TEST_F(WebStateListTest, CloseAllWebStates) {
   EXPECT_TRUE(observer_.batch_operation_ended());
 }
 
-// Test closing one webstate.
+// Tests closing one webstate.
 TEST_F(WebStateListTest, CloseWebState) {
   AppendNewWebState(kURL0);
   AppendNewWebState(kURL1);
@@ -721,7 +722,7 @@ TEST_F(WebStateListTest, CloseWebState) {
   EXPECT_FALSE(observer_.batch_operation_ended());
 }
 
-// Test that batch operation can be empty.
+// Tests that batch operation can be empty.
 TEST_F(WebStateListTest, PerformBatchOperation_EmptyCallback) {
   observer_.ResetStatistics();
 
@@ -731,7 +732,7 @@ TEST_F(WebStateListTest, PerformBatchOperation_EmptyCallback) {
   EXPECT_TRUE(observer_.batch_operation_ended());
 }
 
-// Test that batch operation WebStateList is the correct one.
+// Tests that batch operation WebStateList is the correct one.
 TEST_F(WebStateListTest, PerformBatchOperation_CorrectWebStateList) {
   WebStateList* captured_web_state_list = nullptr;
   web_state_list_.PerformBatchOperation(base::BindOnce(
@@ -742,3 +743,18 @@ TEST_F(WebStateListTest, PerformBatchOperation_CorrectWebStateList) {
 
   EXPECT_EQ(captured_web_state_list, &web_state_list_);
 }
+
+// Tests that IsBatchInProgress() returns the correct value.
+TEST_F(WebStateListTest, PerformBatchOperation_IsBatchInProgress) {
+  EXPECT_FALSE(web_state_list_.IsBatchInProgress());
+
+  bool captured_batch_in_progress = false;
+  web_state_list_.PerformBatchOperation(base::BindOnce(
+      [](bool* captured_batch_in_progress, WebStateList* web_state_list) {
+        *captured_batch_in_progress = web_state_list->IsBatchInProgress();
+      },
+      &captured_batch_in_progress));
+
+  EXPECT_FALSE(web_state_list_.IsBatchInProgress());
+  EXPECT_TRUE(captured_batch_in_progress);
+}