High chrome Input Validation 🔧 Commit mapped

Overview

High
Severity
—
CVSS
No
Exploited ITW
Fixed
Fix Status
ImpactImproper input validation in Proxy
DescriptionImproper input validation in Proxy
ComponentProxy
Sub-componentNet
Bug ClassInput Validation
Tracker568906040
Fix commit2ed13d0826b0 (chromium/src) +178/-125
CISA KEVNot listed
Creditedo
Disclosed2026-10-20

Changed Functions

FunctionChangeNotes
if
net/proxy_resolution/configured_proxy_resolution_request.cc
modified
if
net/proxy_resolution/configured_proxy_resolution_service.cc
modified
SyncFailProxyResolverFactory
net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
modified
async_factory_
net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
modified
if
net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
modified

Files Changed

  • net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
  • net/proxy_resolution/configured_proxy_resolution_request.cc
  • net/proxy_resolution/configured_proxy_resolution_service.cc
From 2ed13d0826b0941eb5fdf6b469cb172b9b8dd2af Mon Sep 17 00:00:00 2001
From: Zonghan Xu <xzonghan@google.com>
Date: Tue, 13 Oct 2026 03:58:26 -0700
Subject: [PATCH] [Net][CPRS] Improve lifetime management in CPRRequest

When an in-flight proxy resolution request fails with
ERR_PAC_SCRIPT_TERMINATED, ConfiguredProxyResolutionService resets its
config and may synchronously re-initialize other pending requests.
During this synchronous re-initialization, a sibling request's callback
can cancel and destroy the completing request while its completion stack
frames are still active.

This change improves request lifetime management:
- End the PROXY_RESOLUTION_SERVICE NetLog event before resetting the
  proxy config in DidFinishResolvingProxy().
- Clear `service_` and `results_` in ConfiguredProxyResolutionRequest
  before calling DidFinishResolvingProxy().
- Guard completion paths with base::WeakPtr to return early if the
  request is destroyed during re-entrant callbacks.

Bug: 568906040
Change-Id: I270e5efe43ebaabc29c8ceead1bae85f3462082d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8560657
Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Zonghan Xu <xzonghan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1717484}
---
diff --git a/net/proxy_resolution/configured_proxy_resolution_request.cc b/net/proxy_resolution/configured_proxy_resolution_request.cc
index e42676f..460f0c4 100644
--- a/net/proxy_resolution/configured_proxy_resolution_request.cc
+++ b/net/proxy_resolution/configured_proxy_resolution_request.cc
@@ -169,26 +169,38 @@
     Reset();
   }
 
-  // Note that DidFinishResolvingProxy might modify `results_`.
-  int rv = service_->DidFinishResolvingProxy(url_, network_anonymization_key_,
-                                             method_, results_, result_code,
-                                             net_log_);
+  // Detach `service_` and `results_` before `DidFinishResolvingProxy()` in case
+  // a sibling callback synchronously destroys `this` or `results` (avoids
+  // double-removal in our destructor and dangling raw_ptrs).
+  ConfiguredProxyResolutionService* service = std::exchange(service_, nullptr);
+  ProxyInfo* results = std::exchange(results_, nullptr);
+  base::WeakPtr<ConfiguredProxyResolutionRequest> weak_this =
+      weak_factory_.GetWeakPtr();
+
+  // Note that DidFinishResolvingProxy might modify `results`.
+  int rv =
+      service->DidFinishResolvingProxy(url_, network_anonymization_key_,
+                                       method_, results, result_code, net_log_);
+  if (!weak_this) {
+    return ERR_ABORTED;
+  }
 
   // Make a note in the results which configuration was in use at the
   // time of the resolve.
-  results_->set_proxy_resolve_start_time(creation_time_);
-  results_->set_proxy_resolve_end_time(base::TimeTicks::Now());
+  results->set_proxy_resolve_start_time(creation_time_);
+  results->set_proxy_resolve_end_time(base::TimeTicks::Now());
 
   // If annotation is not already set, e.g. through TryToCompleteSynchronously
   // function, use in-progress-resolve annotation.
-  if (!results_->traffic_annotation().is_valid())
-    results_->set_traffic_annotation(traffic_annotation_);
+  if (!results->traffic_annotation().is_valid()) {
+    results->set_traffic_annotation(traffic_annotation_);
+  }
 
   // If proxy is set without error, ensure that an annotation is provided.
   if (result_code != ERR_ABORTED && !rv) {
     // TODO(crbug.com/469006851): Turn this back into a CHECK once crashes were
     // fully fixed.
-    DCHECK(results_->traffic_annotation().is_valid());
+    DCHECK(results->traffic_annotation().is_valid());
   }
 
   // Reset the state associated with in-progress-resolve.
@@ -199,9 +211,7 @@
 
 int ConfiguredProxyResolutionRequest::QueryDidCompleteSynchronously(
     int result_code) {
-  int rv = QueryDidComplete(result_code);
-  service_ = nullptr;
-  return rv;
+  return QueryDidComplete(result_code);
 }
 
 LoadState ConfiguredProxyResolutionRequest::GetLoadState() const {
@@ -222,8 +232,12 @@
   // prevent re-entrant dispatch if the service re-initializes synchronously.
   service_->RemovePendingRequest(this);
 
+  base::WeakPtr<ConfiguredProxyResolutionRequest> weak_this =
+      weak_factory_.GetWeakPtr();
   result_code = QueryDidComplete(result_code);
-  service_ = nullptr;
+  if (!weak_this) {
+    return;
+  }
 
   CompletionOnceCallback callback = std::move(user_callback_);
   user_callback_.Reset();
diff --git a/net/proxy_resolution/configured_proxy_resolution_service.cc b/net/proxy_resolution/configured_proxy_resolution_service.cc
index 600274c9..71de633e 100644
--- a/net/proxy_resolution/configured_proxy_resolution_service.cc
+++ b/net/proxy_resolution/configured_proxy_resolution_service.cc
@@ -1293,6 +1293,7 @@
         !pac_retry_throttler_.HasFailedDuringCurrentLoad()) {
       ResetPacRetryThrottler();
     }
+    net_log.EndEvent(NetLogEventType::PROXY_RESOLUTION_SERVICE);
   } else {
     net_log.AddEventWithNetErrorCode(
         NetLogEventType::PROXY_RESOLUTION_SERVICE_RESOLVED_PROXY_LIST,
@@ -1335,6 +1336,10 @@
       }
     }
 
+    // End the NetLog event before resetting the config, as synchronous
+    // re-initialization can destroy the completing request (and `net_log`).
+    net_log.EndEvent(NetLogEventType::PROXY_RESOLUTION_SERVICE);
+
     if (reset_config) {
       ResetProxyConfig(false);
       if (!pending_requests_.empty()) {
@@ -1346,7 +1351,6 @@
     }
   }
 
-  net_log.EndEvent(NetLogEventType::PROXY_RESOLUTION_SERVICE);
   return result_code;
 }
 
diff --git a/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc b/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
index 7e9d649..46b80616 100644
--- a/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
+++ b/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
@@ -531,6 +531,105 @@
   return proxy_list;
 }
 
+// ProxyResolverFactory that delegates to MockAsyncProxyResolverFactory by
+// default, or fails synchronously when `synchronous_fail_` is set.
+class SyncFailProxyResolverFactory : public ProxyResolverFactory {
+ public:
+  explicit SyncFailProxyResolverFactory(bool resolvers_expect_pac_bytes)
+      : ProxyResolverFactory(resolvers_expect_pac_bytes),
+        async_factory_(resolvers_expect_pac_bytes) {}
+
+  // Returns ERR_PAC_SCRIPT_FAILED synchronously if `synchronous_fail_` is set,
+  // or forwards to `async_factory_` otherwise.
+  int CreateProxyResolver(
+      const scoped_refptr<PacFileData>& pac_script,
+      std::unique_ptr<ProxyResolver>* resolver,
+      CompletionOnceCallback callback,
+      std::unique_ptr<ProxyResolverFactory::Request>* request_handle) override {
+    if (synchronous_fail_) {
+      return ERR_PAC_SCRIPT_FAILED;
+    }
+    return async_factory_.CreateProxyResolver(
+        pac_script, resolver, std::move(callback), request_handle);
+  }
+
+  void set_synchronous_fail(bool synchronous_fail) {
+    synchronous_fail_ = synchronous_fail;
+  }
+
+  const MockAsyncProxyResolverFactory::RequestsList& pending_requests() const {
+    return async_factory_.pending_requests();
+  }
+
+ private:
+  bool synchronous_fail_ = false;
+  MockAsyncProxyResolverFactory async_factory_;
+};
+
+// Bundles the caller-owned state for a single proxy resolution request in
+// tests.
+struct TestResolveRequest {
+  std::unique_ptr<ProxyInfo> info = std::make_unique<ProxyInfo>();
+  std::unique_ptr<ProxyResolutionRequest> request;
+  NetLogWithSource net_log = NetLogWithSource::Make(NetLogSourceType::NONE);
+  CompletionOnceCallback callback;
+};
+
+// Starts two concurrent PAC requests and fails the first with
+// ERR_PAC_SCRIPT_TERMINATED while forcing synchronous PAC re-initialization.
+void SimulatePacTerminatedWithSyncReinit(TestResolveRequest& req1,
+                                         TestResolveRequest& req2) {
+  auto config_service =
+      std::make_unique<MockProxyConfigService>("http://foopy/proxy.pac");
+
+  MockAsyncProxyResolver resolver;
+  auto resolver_factory = std::make_unique<SyncFailProxyResolverFactory>(false);
+  auto* resolver_factory_ptr = resolver_factory.get();
+
+  ConfiguredProxyResolutionService service(
+      std::move(config_service), std::move(resolver_factory),
+      /*host_resolver_for_override_rules=*/nullptr, /*net_log=*/nullptr,
+      /*quick_check_enabled=*/true);
+
+  GURL url1("http://www.google.com/");
+  GURL url2("https://www.google.com/");
+
+  // Request 1: In-flight resolve request that will encounter
+  // ERR_PAC_SCRIPT_TERMINATED and trigger proxy re-initialization.
+  int rv1 = service.ResolveProxy(url1, std::string(), NetworkAnonymizationKey(),
+                                 handles::kInvalidNetworkHandle,
+                                 req1.info.get(), std::move(req1.callback),
+                                 &req1.request, req1.net_log, DEFAULT_PRIORITY);
+
+  // Request 2: Concurrent in-flight resolve request that will be paused in
+  // `pending_requests_` and restarted upon re-initialization.
+  int rv2 = service.ResolveProxy(url2, std::string(), NetworkAnonymizationKey(),
+                                 handles::kInvalidNetworkHandle,
+                                 req2.info.get(), std::move(req2.callback),
+                                 &req2.request, req2.net_log, DEFAULT_PRIORITY);
+
+  // Both requests should be waiting for PAC compilation now.
+  EXPECT_THAT(rv1, IsError(ERR_IO_PENDING));
+  EXPECT_THAT(rv2, IsError(ERR_IO_PENDING));
+
+  // Complete PAC script compilation so the service becomes ready and
+  // dispatches `url1` and `url2` to the resolver.
+  ASSERT_EQ(1u, resolver_factory_ptr->pending_requests().size());
+  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
+            resolver_factory_ptr->pending_requests()[0]->script_data()->url());
+  resolver_factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(
+      OK, &resolver);
+
+  // Both `url1` and `url2` are now running as in-flight jobs in the resolver.
+  JobMap jobs = GetPendingJobsForURLs(resolver, url1, url2);
+
+  // Force synchronous re-initialization failure and crash `url1`.
+  resolver_factory_ptr->set_synchronous_fail(true);
+  jobs[url1]->CompleteNow(ERR_PAC_SCRIPT_TERMINATED);
+  EXPECT_FALSE(req1.request);
+  EXPECT_FALSE(req2.request);
+}
+
 }  // namespace
 
 // When a PAC runtime error occurs (ERR_PAC_SCRIPT_FAILED), subsequent
@@ -1930,135 +2029,71 @@
 // Test what happens when the ProxyResolver fails with
 // ERR_PAC_SCRIPT_TERMINATED while multiple requests are in progress, and the
 // subsequent proxy re-initialization completes synchronously (e.g. fails and
-// falls back to DIRECT). Make sure no use-after-free occurs.
+// falls back to DIRECT). Make sure no use-after-free occurs when each request
+// destroys itself in its completion callback (crbug.com/533534913).
 TEST_F(ConfiguredProxyResolutionServiceTest,
-       ProxyResolverTerminatedDuringRequestWithSyncReinit) {
-  class SyncFailProxyResolverFactory : public ProxyResolverFactory {
-   public:
-    explicit SyncFailProxyResolverFactory(bool resolvers_expect_pac_bytes)
-        : ProxyResolverFactory(resolvers_expect_pac_bytes),
-          async_factory_(resolvers_expect_pac_bytes) {}
-
-    int CreateProxyResolver(const scoped_refptr<PacFileData>& pac_script,
-                            std::unique_ptr<ProxyResolver>* resolver,
-                            CompletionOnceCallback callback,
-                            std::unique_ptr<ProxyResolverFactory::Request>*
-                                request_handle) override {
-      if (synchronous_fail_) {
-        return ERR_PAC_SCRIPT_FAILED;
-      }
-      return async_factory_.CreateProxyResolver(
-          pac_script, resolver, std::move(callback), request_handle);
-    }
-
-    void set_synchronous_fail(bool synchronous_fail) {
-      synchronous_fail_ = synchronous_fail;
-    }
-
-    const MockAsyncProxyResolverFactory::RequestsList& pending_requests()
-        const {
-      return async_factory_.pending_requests();
-    }
-
-   private:
-    bool synchronous_fail_ = false;
-    MockAsyncProxyResolverFactory async_factory_;
-  };
-
-  auto config_service =
-      std::make_unique<MockProxyConfigService>("http://foopy/proxy.pac");
-
-  MockAsyncProxyResolver resolver;
-  auto resolver_factory = std::make_unique<SyncFailProxyResolverFactory>(false);
-  auto* resolver_factory_ptr = resolver_factory.get();
-
-  ConfiguredProxyResolutionService service(std::move(config_service),
-                                           std::move(resolver_factory),
-                                           mock_host_resolver_.get(), nullptr,
-                                           /*quick_check_enabled=*/true);
-
-  // Start two resolve requests.
-  GURL url1("http://www.google.com/");
-  GURL url2("https://www.google.com/");
-  ProxyInfo info1, info2;
-  std::unique_ptr<ProxyResolutionRequest> request1, request2;
+       ProxyResolverTerminatedSyncReinitSelfDeletion) {
+  TestResolveRequest req1, req2;
 
   int callback1_result = ERR_IO_PENDING;
-  base::RunLoop run_loop1;
-  auto callback1 = base::BindLambdaForTesting([&](int rv) {
+  req1.callback = base::BindLambdaForTesting([&](int rv) {
     callback1_result = rv;
     // Destroy the request inside the completion callback, simulating the
     // behavior of consumers like HttpStreamFactory::JobController.
-    request1.reset();
-    run_loop1.Quit();
+    req1.request.reset();
   });
 
   int callback2_result = ERR_IO_PENDING;
-  base::RunLoop run_loop2;
-  auto callback2 = base::BindLambdaForTesting([&](int rv) {
+  req2.callback = base::BindLambdaForTesting([&](int rv) {
     callback2_result = rv;
-    request2.reset();
-    run_loop2.Quit();
+    req2.request.reset();
   });
 
-  // Request 1: In-flight resolve request that will encounter
-  // ERR_PAC_SCRIPT_TERMINATED and trigger proxy re-initialization.
-  int rv1 = service.ResolveProxy(url1, std::string(), NetworkAnonymizationKey(),
-                                 handles::kInvalidNetworkHandle, &info1,
-                                 std::move(callback1), &request1,
-                                 NetLogWithSource(), DEFAULT_PRIORITY);
+  SimulatePacTerminatedWithSyncReinit(req1, req2);
 
-  // Request 2: Concurrent in-flight resolve request that will be paused in
-  // `pending_requests_` and restarted upon re-initialization.
-  int rv2 = service.ResolveProxy(url2, std::string(), NetworkAnonymizationKey(),
-                                 handles::kInvalidNetworkHandle, &info2,
-                                 std::move(callback2), &request2,
-                                 NetLogWithSource(), DEFAULT_PRIORITY);
-
-  // Both requests should be waiting for PAC compilation now.
-  EXPECT_THAT(rv1, IsError(ERR_IO_PENDING));
-  EXPECT_THAT(rv2, IsError(ERR_IO_PENDING));
-
-  // The service must first create a ProxyResolver by fetching and compiling
-  // the PAC script. We should have one PAC factory creation request for
-  // "http://foopy/proxy.pac".
-  ASSERT_EQ(1u, resolver_factory_ptr->pending_requests().size());
-  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
-            resolver_factory_ptr->pending_requests()[0]->script_data()->url());
-
-  // Complete PAC script compilation now so the service becomes ready and
-  // dispatches `url1` and `url2` to the resolver.
-  resolver_factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(
-      OK, &resolver);
-
-  // Both `url1` and `url2` are now running as in-flight jobs in the resolver.
-  JobMap jobs = GetPendingJobsForURLs(resolver, url1, url2);
-
-  // Configure the next CreateProxyResolver call (when the service attempts to
-  // reset and re-initialize the PAC resolver after `url1` crashes) to fail
-  // synchronously.
-  resolver_factory_ptr->set_synchronous_fail(true);
-
-  // Simulate a PAC script crash on `url1`. DidFinishResolvingProxy() will then
-  // reset the configuration, attempt re-initialization (which fails
-  // synchronously), and fall back to DIRECT.
-  //
-  // Note: this covers the edge case reported in crbug.com/533534913 and
-  // confirms that destroying `request1` inside its completion callback does not
-  // cause a use-after-free.
-  jobs[url1]->CompleteNow(ERR_PAC_SCRIPT_TERMINATED);
-
-  // `request1` fell back to DIRECT per-request as the error fallback for
+  // `req1` fell back to DIRECT per-request as the error fallback for
   // PAC runtime script crash (ERR_PAC_SCRIPT_TERMINATED).
-  run_loop1.Run();
   EXPECT_THAT(callback1_result, IsOk());
-  EXPECT_TRUE(info1.is_direct());
+  EXPECT_TRUE(req1.info->is_direct());
 
   // PAC re-initialization failed, so the entire service entered DIRECT fallback
-  // mode and restarted `request2` against this new DIRECT configuration.
-  run_loop2.Run();
+  // mode and restarted `req2` against this new DIRECT configuration.
   EXPECT_THAT(callback2_result, IsOk());
-  EXPECT_TRUE(info2.is_direct());
+  EXPECT_TRUE(req2.info->is_direct());
+}
+
+// Verifies that cancelling the completing request from a sibling request's
+// callback during synchronous PAC re-initialization is handled safely.
+TEST_F(ConfiguredProxyResolutionServiceTest,
+       ProxyResolverTerminatedSyncReinitSiblingDeletion) {
+  TestResolveRequest req1, req2;
+  RecordingNetLogObserver net_log_observer;
+
... (file diff truncated)
Loading diff…

Regression Test / PoC

shipped with the fix
diff --git a/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc b/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
index 7e9d649..46b80616 100644
--- a/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
+++ b/net/proxy_resolution/configured_proxy_resolution_service_unittest.cc
@@ -531,6 +531,105 @@
   return proxy_list;
 }
 
+// ProxyResolverFactory that delegates to MockAsyncProxyResolverFactory by
+// default, or fails synchronously when `synchronous_fail_` is set.
+class SyncFailProxyResolverFactory : public ProxyResolverFactory {
+ public:
+  explicit SyncFailProxyResolverFactory(bool resolvers_expect_pac_bytes)
+      : ProxyResolverFactory(resolvers_expect_pac_bytes),
+        async_factory_(resolvers_expect_pac_bytes) {}
+
+  // Returns ERR_PAC_SCRIPT_FAILED synchronously if `synchronous_fail_` is set,
+  // or forwards to `async_factory_` otherwise.
+  int CreateProxyResolver(
+      const scoped_refptr<PacFileData>& pac_script,
+      std::unique_ptr<ProxyResolver>* resolver,
+      CompletionOnceCallback callback,
+      std::unique_ptr<ProxyResolverFactory::Request>* request_handle) override {
+    if (synchronous_fail_) {
+      return ERR_PAC_SCRIPT_FAILED;
+    }
+    return async_factory_.CreateProxyResolver(
+        pac_script, resolver, std::move(callback), request_handle);
+  }
+
+  void set_synchronous_fail(bool synchronous_fail) {
+    synchronous_fail_ = synchronous_fail;
+  }
+
+  const MockAsyncProxyResolverFactory::RequestsList& pending_requests() const {
+    return async_factory_.pending_requests();
+  }
+
+ private:
+  bool synchronous_fail_ = false;
+  MockAsyncProxyResolverFactory async_factory_;
+};
+
+// Bundles the caller-owned state for a single proxy resolution request in
+// tests.
+struct TestResolveRequest {
+  std::unique_ptr<ProxyInfo> info = std::make_unique<ProxyInfo>();
+  std::unique_ptr<ProxyResolutionRequest> request;
+  NetLogWithSource net_log = NetLogWithSource::Make(NetLogSourceType::NONE);
+  CompletionOnceCallback callback;
+};
+
+// Starts two concurrent PAC requests and fails the first with
+// ERR_PAC_SCRIPT_TERMINATED while forcing synchronous PAC re-initialization.
+void SimulatePacTerminatedWithSyncReinit(TestResolveRequest& req1,
+                                         TestResolveRequest& req2) {
+  auto config_service =
+      std::make_unique<MockProxyConfigService>("http://foopy/proxy.pac");
+
+  MockAsyncProxyResolver resolver;
+  auto resolver_factory = std::make_unique<SyncFailProxyResolverFactory>(false);
+  auto* resolver_factory_ptr = resolver_factory.get();
+
+  ConfiguredProxyResolutionService service(
+      std::move(config_service), std::move(resolver_factory),
+      /*host_resolver_for_override_rules=*/nullptr, /*net_log=*/nullptr,
+      /*quick_check_enabled=*/true);
+
+  GURL url1("http://www.google.com/");
+  GURL url2("https://www.google.com/");
+
+  // Request 1: In-flight resolve request that will encounter
+  // ERR_PAC_SCRIPT_TERMINATED and trigger proxy re-initialization.
+  int rv1 = service.ResolveProxy(url1, std::string(), NetworkAnonymizationKey(),
+                                 handles::kInvalidNetworkHandle,
+                                 req1.info.get(), std::move(req1.callback),
+                                 &req1.request, req1.net_log, DEFAULT_PRIORITY);
+
+  // Request 2: Concurrent in-flight resolve request that will be paused in
+  // `pending_requests_` and restarted upon re-initialization.
+  int rv2 = service.ResolveProxy(url2, std::string(), NetworkAnonymizationKey(),
+                                 handles::kInvalidNetworkHandle,
+                                 req2.info.get(), std::move(req2.callback),
+                                 &req2.request, req2.net_log, DEFAULT_PRIORITY);
+
+  // Both requests should be waiting for PAC compilation now.
+  EXPECT_THAT(rv1, IsError(ERR_IO_PENDING));
+  EXPECT_THAT(rv2, IsError(ERR_IO_PENDING));
+
+  // Complete PAC script compilation so the service becomes ready and
+  // dispatches `url1` and `url2` to the resolver.
+  ASSERT_EQ(1u, resolver_factory_ptr->pending_requests().size());
+  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
+            resolver_factory_ptr->pending_requests()[0]->script_data()->url());
+  resolver_factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(
+      OK, &resolver);
+
+  // Both `url1` and `url2` are now running as in-flight jobs in the resolver.
+  JobMap jobs = GetPendingJobsForURLs(resolver, url1, url2);
+
+  // Force synchronous re-initialization failure and crash `url1`.
+  resolver_factory_ptr->set_synchronous_fail(true);
+  jobs[url1]->CompleteNow(ERR_PAC_SCRIPT_TERMINATED);
+  EXPECT_FALSE(req1.request);
+  EXPECT_FALSE(req2.request);
+}
+
 }  // namespace
 
 // When a PAC runtime error occurs (ERR_PAC_SCRIPT_FAILED), subsequent
@@ -1930,135 +2029,71 @@
 // Test what happens when the ProxyResolver fails with
 // ERR_PAC_SCRIPT_TERMINATED while multiple requests are in progress, and the
 // subsequent proxy re-initialization completes synchronously (e.g. fails and
-// falls back to DIRECT). Make sure no use-after-free occurs.
+// falls back to DIRECT). Make sure no use-after-free occurs when each request
+// destroys itself in its completion callback (crbug.com/533534913).
 TEST_F(ConfiguredProxyResolutionServiceTest,
-       ProxyResolverTerminatedDuringRequestWithSyncReinit) {
-  class SyncFailProxyResolverFactory : public ProxyResolverFactory {
-   public:
-    explicit SyncFailProxyResolverFactory(bool resolvers_expect_pac_bytes)
-        : ProxyResolverFactory(resolvers_expect_pac_bytes),
-          async_factory_(resolvers_expect_pac_bytes) {}
-
-    int CreateProxyResolver(const scoped_refptr<PacFileData>& pac_script,
-                            std::unique_ptr<ProxyResolver>* resolver,
-                            CompletionOnceCallback callback,
-                            std::unique_ptr<ProxyResolverFactory::Request>*
-                                request_handle) override {
-      if (synchronous_fail_) {
-        return ERR_PAC_SCRIPT_FAILED;
-      }
-      return async_factory_.CreateProxyResolver(
-          pac_script, resolver, std::move(callback), request_handle);
-    }
-
-    void set_synchronous_fail(bool synchronous_fail) {
-      synchronous_fail_ = synchronous_fail;
-    }
-
-    const MockAsyncProxyResolverFactory::RequestsList& pending_requests()
-        const {
-      return async_factory_.pending_requests();
-    }
-
-   private:
-    bool synchronous_fail_ = false;
-    MockAsyncProxyResolverFactory async_factory_;
-  };
-
-  auto config_service =
-      std::make_unique<MockProxyConfigService>("http://foopy/proxy.pac");
-
-  MockAsyncProxyResolver resolver;
-  auto resolver_factory = std::make_unique<SyncFailProxyResolverFactory>(false);
-  auto* resolver_factory_ptr = resolver_factory.get();
-
-  ConfiguredProxyResolutionService service(std::move(config_service),
-                                           std::move(resolver_factory),
-                                           mock_host_resolver_.get(), nullptr,
-                                           /*quick_check_enabled=*/true);
-
-  // Start two resolve requests.
-  GURL url1("http://www.google.com/");
-  GURL url2("https://www.google.com/");
-  ProxyInfo info1, info2;
-  std::unique_ptr<ProxyResolutionRequest> request1, request2;
+       ProxyResolverTerminatedSyncReinitSelfDeletion) {
+  TestResolveRequest req1, req2;
 
   int callback1_result = ERR_IO_PENDING;
-  base::RunLoop run_loop1;
-  auto callback1 = base::BindLambdaForTesting([&](int rv) {
+  req1.callback = base::BindLambdaForTesting([&](int rv) {
     callback1_result = rv;
     // Destroy the request inside the completion callback, simulating the
     // behavior of consumers like HttpStreamFactory::JobController.
-    request1.reset();
-    run_loop1.Quit();
+    req1.request.reset();
   });
 
   int callback2_result = ERR_IO_PENDING;
-  base::RunLoop run_loop2;
-  auto callback2 = base::BindLambdaForTesting([&](int rv) {
+  req2.callback = base::BindLambdaForTesting([&](int rv) {
     callback2_result = rv;
-    request2.reset();
-    run_loop2.Quit();
+    req2.request.reset();
   });
 
-  // Request 1: In-flight resolve request that will encounter
-  // ERR_PAC_SCRIPT_TERMINATED and trigger proxy re-initialization.
-  int rv1 = service.ResolveProxy(url1, std::string(), NetworkAnonymizationKey(),
-                                 handles::kInvalidNetworkHandle, &info1,
-                                 std::move(callback1), &request1,
-                                 NetLogWithSource(), DEFAULT_PRIORITY);
+  SimulatePacTerminatedWithSyncReinit(req1, req2);
 
-  // Request 2: Concurrent in-flight resolve request that will be paused in
-  // `pending_requests_` and restarted upon re-initialization.
-  int rv2 = service.ResolveProxy(url2, std::string(), NetworkAnonymizationKey(),
-                                 handles::kInvalidNetworkHandle, &info2,
-                                 std::move(callback2), &request2,
-                                 NetLogWithSource(), DEFAULT_PRIORITY);
-
-  // Both requests should be waiting for PAC compilation now.
-  EXPECT_THAT(rv1, IsError(ERR_IO_PENDING));
-  EXPECT_THAT(rv2, IsError(ERR_IO_PENDING));
-
-  // The service must first create a ProxyResolver by fetching and compiling
-  // the PAC script. We should have one PAC factory creation request for
-  // "http://foopy/proxy.pac".
-  ASSERT_EQ(1u, resolver_factory_ptr->pending_requests().size());
-  EXPECT_EQ(GURL("http://foopy/proxy.pac"),
-            resolver_factory_ptr->pending_requests()[0]->script_data()->url());
-
-  // Complete PAC script compilation now so the service becomes ready and
-  // dispatches `url1` and `url2` to the resolver.
-  resolver_factory_ptr->pending_requests()[0]->CompleteNowWithForwarder(
-      OK, &resolver);
-
-  // Both `url1` and `url2` are now running as in-flight jobs in the resolver.
-  JobMap jobs = GetPendingJobsForURLs(resolver, url1, url2);
-
-  // Configure the next CreateProxyResolver call (when the service attempts to
-  // reset and re-initialize the PAC resolver after `url1` crashes) to fail
-  // synchronously.
-  resolver_factory_ptr->set_synchronous_fail(true);
-
-  // Simulate a PAC script crash on `url1`. DidFinishResolvingProxy() will then
-  // reset the configuration, attempt re-initialization (which fails
-  // synchronously), and fall back to DIRECT.
-  //
-  // Note: this covers the edge case reported in crbug.com/533534913 and
-  // confirms that destroying `request1` inside its completion callback does not
-  // cause a use-after-free.
-  jobs[url1]->CompleteNow(ERR_PAC_SCRIPT_TERMINATED);
-
-  // `request1` fell back to DIRECT per-request as the error fallback for
+  // `req1` fell back to DIRECT per-request as the error fallback for
   // PAC runtime script crash (ERR_PAC_SCRIPT_TERMINATED).
-  run_loop1.Run();
   EXPECT_THAT(callback1_result, IsOk());
-  EXPECT_TRUE(info1.is_direct());
+  EXPECT_TRUE(req1.info->is_direct());
 
   // PAC re-initialization failed, so the entire service entered DIRECT fallback
-  // mode and restarted `request2` against this new DIRECT configuration.
-  run_loop2.Run();
+  // mode and restarted `req2` against this new DIRECT configuration.
   EXPECT_THAT(callback2_result, IsOk());
-  EXPECT_TRUE(info2.is_direct());
+  EXPECT_TRUE(req2.info->is_direct());
+}
+
+// Verifies that cancelling the completing request from a sibling request's
+// callback during synchronous PAC re-initialization is handled safely.
+TEST_F(ConfiguredProxyResolutionServiceTest,
+       ProxyResolverTerminatedSyncReinitSiblingDeletion) {
+  TestResolveRequest req1, req2;
+  RecordingNetLogObserver net_log_observer;
+
+  bool callback1_called = false;
+  req1.callback =
+      base::BindLambdaForTesting([&](int rv) { callback1_called = true; });
+
+  int callback2_result = ERR_IO_PENDING;
+  req2.callback = base::BindLambdaForTesting([&](int rv) {
+    callback2_result = rv;
+    // Cancel `req1` while its completion frames are still on the stack.
+    req1.info.reset();
+    req1.request.reset();
+    req2.request.reset();
+  });
+
+  SimulatePacTerminatedWithSyncReinit(req1, req2);
+
+  EXPECT_THAT(callback2_result, IsOk());
+  EXPECT_TRUE(req2.info->is_direct());
+  EXPECT_FALSE(callback1_called);
+
+  auto entries1 = net_log_observer.GetEntriesForSource(req1.net_log.source());
+  ASSERT_EQ(5u, entries1.size());
+  EXPECT_TRUE(LogContainsBeginEvent(entries1, 0,
+                                    NetLogEventType::PROXY_RESOLUTION_SERVICE));
+  EXPECT_TRUE(LogContainsEndEvent(entries1, 4,
+                                  NetLogEventType::PROXY_RESOLUTION_SERVICE));
 }
 
 TEST_F(ConfiguredProxyResolutionServiceTest,
Loading diff…

Original Bug Report

The reporter's bug is still restricted on the tracker. Chrome de-restricts security bugs ~30–90 days after the fix ships; a later run will backfill it here.