Don't access this in BrandcodeConfigFetcher after running the callback

The callback in BrandcodeConfigFetcher::OnSimpleLoaderComplete can be
run synchronously, which may delete `this`. Avoid accessing member
variables after running the callback to avoid a use-after-free.

(cherry picked from commit 6f476b9)

Fixed: 1491296
Change-Id: Id8ce7ce5cde13a81f213f6c54d62de62b566e993
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4931200
Commit-Queue: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1208889}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4949910
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5993@{#1338}
Cr-Branched-From: 5113507-refs/heads/main@{#1192594}

chrome/browser/profile_resetter/brandcode_config_fetcher.cc

@@ -107,8 +107,15 @@ void BrandcodeConfigFetcher::SetCallback(FetchCallback callback) {
 
 void BrandcodeConfigFetcher::OnSimpleLoaderComplete(
     std::unique_ptr<std::string> response_body) {
-  if (response_body && simple_url_loader_->ResponseInfo() &&
-      simple_url_loader_->ResponseInfo()->mime_type == "text/xml") {
+  const bool is_valid_response =
+      response_body && simple_url_loader_->ResponseInfo() &&
+      simple_url_loader_->ResponseInfo()->mime_type == "text/xml";
+
+  // Release resources before potentially running the callback.
+  simple_url_loader_.reset();
+  download_timer_.Stop();
+
+  if (is_valid_response) {
     data_decoder::DataDecoder::ParseXmlIsolated(
         *response_body,
         data_decoder::mojom::XmlParser::WhitespaceBehavior::kIgnore,
@@ -117,8 +124,8 @@ void BrandcodeConfigFetcher::OnSimpleLoaderComplete(
   } else {
     std::move(fetch_callback_).Run();
   }
-  simple_url_loader_.reset();
-  download_timer_.Stop();
+
+  // `this` may now be deleted from `fetch_callback_`.
 }
 
 void BrandcodeConfigFetcher::OnXmlConfigParsed(

chrome/browser/profile_resetter/profile_resetter_unittest.cc

@@ -748,7 +748,7 @@ TEST_F(ConfigParserTest, NoConnectivity) {
       url, network::mojom::URLResponseHead::New(), "",
       network::URLLoaderCompletionStatus(net::HTTP_INTERNAL_SERVER_ERROR));
 
-  std::unique_ptr<BrandcodeConfigFetcher> fetcher = WaitForRequest(GURL(url));
+  std::unique_ptr<BrandcodeConfigFetcher> fetcher = WaitForRequest(url);
   EXPECT_FALSE(fetcher->GetSettings());
 }
 
@@ -770,7 +770,7 @@ TEST_F(ConfigParserTest, ParseConfig) {
   test_url_loader_factory().AddResponse(url, std::move(head), xml_config,
                                         status);
 
-  std::unique_ptr<BrandcodeConfigFetcher> fetcher = WaitForRequest(GURL(url));
+  std::unique_ptr<BrandcodeConfigFetcher> fetcher = WaitForRequest(url);
   std::unique_ptr<BrandcodedDefaultSettings> settings = fetcher->GetSettings();
   ASSERT_TRUE(settings);
 
@@ -796,6 +796,32 @@ TEST_F(ConfigParserTest, ParseConfig) {
   EXPECT_EQ("http://foo.de", startup_pages[1]);
 }
 
+// Return an invalid response from the fetch request and delete the
+// Fetcher object in the callback, which mimics how ResetSettingsHandler uses
+// the class. See https://crbug.com/1491296.
+TEST_F(ConfigParserTest, InvalidResponseDeleteFromCallback) {
+  const GURL url("http://test");
+  auto head = network::mojom::URLResponseHead::New();
+  head->headers = base::MakeRefCounted<net::HttpResponseHeaders>(
+      net::HttpUtil::AssembleRawHeaders(
+          "HTTP/1.1 200 OK\nContent-type: application/custom\n\n"));
+  head->mime_type = "application/custom";
+  test_url_loader_factory().AddResponse(url, std::move(head),
+                                        "Custom app data, not XML",
+                                        network::URLLoaderCompletionStatus());
+
+  base::RunLoop run_loop;
+  std::unique_ptr<BrandcodeConfigFetcher> fetcher;
+  auto callback = base::BindLambdaForTesting([&fetcher, &run_loop] {
+    EXPECT_FALSE(fetcher->GetSettings());
+    fetcher.reset();
+    run_loop.Quit();
+  });
+  fetcher = std::make_unique<BrandcodeConfigFetcher>(&test_url_loader_factory(),
+                                           std::move(callback), url, "ABCD");
+  run_loop.Run();
+}
+
 TEST_F(ProfileResetterTest, CheckSnapshots) {
   ResettableSettingsSnapshot empty_snap(profile());
   EXPECT_EQ(0, empty_snap.FindDifferentFields(empty_snap));