[unified_consent] Fix ConsentThrottle crash from mutate during iterate

We have evidence that this crash is caused by enqueuing new requests
within the callback itself.

This CL makes the throttle robust to callbacks that enqueue new
requests, and adds a unit test for that case.

Bug: 1483454
Change-Id: I452370ea0a90b2586c67c161220dc36f40aaede8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5030755
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Tommy Li <tommycli@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Sky Malice <skym@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1225601}

components/unified_consent/consent_throttle.cc

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
+
 #include "components/unified_consent/consent_throttle.h"
 #include "base/functional/bind.h"
 #include "base/time/time.h"
@@ -43,12 +45,15 @@ void ConsentThrottle::OnUrlKeyedDataCollectionConsentStateChanged(
     return;
   }
 
-  for (auto& request_callback : enqueued_request_callbacks_) {
+  request_processing_timer_.Stop();
+
+  // The request callbacks can modify the vector while running. Swap the vector
+  // onto the stack to prevent crashing. https://crbug.com/1483454.
+  std::vector<RequestCallback> callbacks;
+  std::swap(callbacks, enqueued_request_callbacks_);
+  for (auto& request_callback : callbacks) {
     FulfillRequestCallback(consent_state, std::move(request_callback));
   }
-
-  enqueued_request_callbacks_.clear();
-  request_processing_timer_.Stop();
 }
 
 void ConsentThrottle::EnqueueRequest(RequestCallback callback) {
@@ -71,12 +76,16 @@ void ConsentThrottle::EnqueueRequest(RequestCallback callback) {
 }
 
 void ConsentThrottle::OnTimeoutExpired() {
-  for (auto& request_callback : enqueued_request_callbacks_) {
-    CHECK_EQ(consent_helper_->GetConsentState(),
-             UrlKeyedDataCollectionConsentHelper::State::kInitializing);
+  CHECK_EQ(consent_helper_->GetConsentState(),
+           UrlKeyedDataCollectionConsentHelper::State::kInitializing);
+
+  // The request callbacks can modify the vector while running. Swap the vector
+  // onto the stack to prevent crashing. https://crbug.com/1483454.
+  std::vector<RequestCallback> callbacks;
+  std::swap(callbacks, enqueued_request_callbacks_);
+  for (auto& request_callback : callbacks) {
     std::move(request_callback).Run(false);
   }
-  enqueued_request_callbacks_.clear();
 }
 
 }  // namespace unified_consent

components/unified_consent/consent_throttle.h

@@ -10,7 +10,6 @@
 #include "base/functional/callback.h"
 #include "base/scoped_observation.h"
 #include "base/time/time.h"
-#include "base/timer/elapsed_timer.h"
 #include "base/timer/timer.h"
 #include "components/unified_consent/url_keyed_data_collection_consent_helper.h"
 

components/unified_consent/consent_throttle_unittest.cc

@@ -139,5 +139,36 @@ TEST_F(ConsentThrottleTest, InitializationDisabledCase) {
   EXPECT_FALSE(results[0]);
 }
 
+// In production, sometimes the callback to a request enqueues a new request.
+// This tests this case and fixes the crash in https://crbug.com/1483454.
+TEST_F(ConsentThrottleTest, CallbacksMakingNewRequests) {
+  auto helper = std::make_unique<TestUrlKeyedDataCollectionConsentHelper>();
+  ASSERT_EQ(helper->GetConsentState(),
+            UrlKeyedDataCollectionConsentHelper::State::kInitializing);
+
+  auto consent_throttle = ConsentThrottle(std::move(helper));
+  std::vector<bool> results;
+
+  // These two blocks are identical. The crash is reliably triggered when
+  // adding two of these. Probably having two pushes the vector to reallocate
+  // while iterating.
+  consent_throttle.EnqueueRequest(base::BindLambdaForTesting([&](bool result) {
+    results.push_back(result);
+    consent_throttle.EnqueueRequest(base::BindLambdaForTesting(
+        [&](bool result2) { results.push_back(result2); }));
+  }));
+  consent_throttle.EnqueueRequest(base::BindLambdaForTesting([&](bool result) {
+    results.push_back(result);
+    consent_throttle.EnqueueRequest(base::BindLambdaForTesting(
+        [&](bool result2) { results.push_back(result2); }));
+  }));
+
+  // New requests added during iteration live as long as the NEXT timeout.
+  task_environment_.FastForwardBy(base::Seconds(6));
+  EXPECT_EQ(results.size(), 2U);
+  task_environment_.FastForwardBy(base::Seconds(6));
+  EXPECT_EQ(results.size(), 4U);
+}
+
 }  // namespace
 }  // namespace unified_consent