[Safe Browsing] Fix crash in token fetching logic

When SafeBrowsingTokenFetchTracker is destroyed, it runs all of its
outstanding client callbacks. However, when an access token is fetched
SafeBrowsingTokenFetchTracker calls the corresponding client callback
(which is a base::OnceCallback) *before* it removes that callback from
the set of outstanding callbacks. The combination of these facts means
that if the SafeBrowsingTokenFetchTracker is destroyed from within a
client callback, a crash will occur. This indeed occurs in production
(see crashes linked in bug, and note that this is behavior that dates
back from the introduction of this code in SafeBrowsingTokenFetcher).

This CL adds a test that exhibits the problematic flow and fixes the
crash by removing the callback from the set of outstanding callbacks
*before* running that callback on access token fetch. The test crashes
without the fix.

Bug: 1168599
Change-Id: I006bde8e48531b9912fa779a5c6c6a27acaf6fa1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2640375Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#846022}

components/safe_browsing/core/browser/safe_browsing_token_fetch_tracker.cc

@@ -57,11 +57,16 @@ void SafeBrowsingTokenFetchTracker::OnTokenFetchTimeout(
 
 void SafeBrowsingTokenFetchTracker::Finish(int request_id,
                                            const std::string& access_token) {
-  if (callbacks_.contains(request_id)) {
-    std::move(callbacks_[request_id]).Run(access_token);
-  }
+  if (!callbacks_.contains(request_id))
+    return;
 
+  // Remove the callback from the map before running it so that this object
+  // doesn't try to run this callback again if deleted from within the
+  // callback.
+  auto callback = std::move(callbacks_[request_id]);
   callbacks_.erase(request_id);
+
+  std::move(callback).Run(access_token);
 }
 
 }  // namespace safe_browsing

components/safe_browsing/core/browser/safe_browsing_token_fetch_tracker_unittest.cc

@@ -87,6 +87,28 @@ TEST_F(SafeBrowsingTokenFetchTrackerTest, FetcherDestruction) {
   EXPECT_EQ(access_token2, "");
 }
 
+// Verifies that destruction of a SafeBrowsingTokenFetchTracker instance from
+// within the client callback that the token was fetched doesn't cause a crash.
+TEST_F(SafeBrowsingTokenFetchTrackerTest,
+       FetcherDestroyedFromWithinOnTokenFetchedCallback) {
+  // Destroyed in the token fetch callback.
+  auto* fetcher = new SafeBrowsingTokenFetchTracker();
+
+  std::string access_token;
+  int request_id = fetcher->StartTrackingTokenFetch(
+      base::BindOnce(
+          [](std::string* target_token, SafeBrowsingTokenFetchTracker* fetcher,
+             const std::string& token) {
+            *target_token = token;
+            delete fetcher;
+          },
+          &access_token, fetcher),
+      base::BindOnce([](int request_id) {}));
+
+  fetcher->OnTokenFetchComplete(request_id, "token");
+  EXPECT_EQ(access_token, "token");
+}
+
 TEST_F(SafeBrowsingTokenFetchTrackerTest, Timeout) {
   SafeBrowsingTokenFetchTracker fetcher;
   std::string access_token1 = "dummy_value1";