Ensure inactive factories are deleted during ResetURLLoaderFactories()

Calling ClearBindings() on a factory with no in progress requests could
have previously led to a leak.

Change-Id: Ia7bba6f5476366d4ffa7912974190c32c0bebd41
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032717Reviewed-by: Matthew Denton <mpdenton@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#737418}

services/network/cors/cors_url_loader_factory.cc

@@ -223,6 +223,7 @@ void CorsURLLoaderFactory::Clone(
 
 void CorsURLLoaderFactory::ClearBindings() {
   receivers_.Clear();
+  DeleteIfNeeded();
 }
 
 void CorsURLLoaderFactory::DeleteIfNeeded() {

services/network/cors/cors_url_loader_factory.h

@@ -49,7 +49,8 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
   void DestroyURLLoader(mojom::URLLoader* loader);
 
   // Clears the bindings for this factory, but does not touch any in-progress
-  // URLLoaders.
+  // URLLoaders. Calling this may delete this factory and remove it from the
+  // network context.
   void ClearBindings();
 
   int32_t process_id() const { return process_id_; }

services/network/network_context.cc

@@ -518,7 +518,13 @@ void NetworkContext::CreateURLLoaderFactory(
 }
 
 void NetworkContext::ResetURLLoaderFactories() {
+  // Move all factories to a temporary vector so ClearBindings() does not
+  // invalidate the iterator if the factory gets deleted.
+  std::vector<cors::CorsURLLoaderFactory*> factories;
+  factories.reserve(url_loader_factories_.size());
   for (const auto& factory : url_loader_factories_)
+    factories.push_back(factory.get());
+  for (auto* factory : factories)
     factory->ClearBindings();
 }
 

services/network/network_context.h

@@ -466,6 +466,10 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) NetworkContext
 
   size_t NumOpenQuicTransports() const;
 
+  size_t num_url_loader_factories_for_testing() const {
+    return url_loader_factories_.size();
+  }
+
  private:
   URLRequestContextOwner MakeURLRequestContext();
 

services/network/network_context_unittest.cc

@@ -6489,6 +6489,28 @@ TEST_F(NetworkContextTest, HSTSPolicyBypassList) {
   EXPECT_TRUE(transport_security_state->ShouldUpgradeToSSL("sub.example"));
 }
 
+TEST_F(NetworkContextTest, FactoriesDeletedWhenBindingsCleared) {
+  std::unique_ptr<NetworkContext> network_context =
+      CreateContextWithParams(CreateContextParams());
+
+  auto loader_params = mojom::URLLoaderFactoryParams::New();
+  loader_params->process_id = 1;
+  mojo::Remote<mojom::URLLoaderFactory> remote1;
+  network_context->CreateURLLoaderFactory(remote1.BindNewPipeAndPassReceiver(),
+                                          std::move(loader_params));
+
+  loader_params = mojom::URLLoaderFactoryParams::New();
+  loader_params->process_id = 1;
+  mojo::Remote<mojom::URLLoaderFactory> remote2;
+  network_context->CreateURLLoaderFactory(remote2.BindNewPipeAndPassReceiver(),
+                                          std::move(loader_params));
+
+  // We should have at least 2 loader factories.
+  EXPECT_GT(network_context->num_url_loader_factories_for_testing(), 1u);
+  network_context->ResetURLLoaderFactories();
+  EXPECT_EQ(network_context->num_url_loader_factories_for_testing(), 0u);
+}
+
 #if BUILDFLAG(BUILTIN_CERT_VERIFIER_FEATURE_SUPPORTED)
 TEST_F(NetworkContextTest, UseCertVerifierBuiltin) {
   net::EmbeddedTestServer test_server(net::EmbeddedTestServer::TYPE_HTTPS);