From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 From: Matt Menke Date: Thu, 10 Jun 2021 07:05:07 +0000 Subject: Fix URLLoader cleanup on CorsURLLoaderFactory destruction. Destroying one URLLoader can result in other URLLoaders getting errors, due to to cache interconnectedness. CorsURLLoaderFactory's destructor was not taking that into account. Also fix a bonus bug: HttpCache::Transaction::response_ wasn't being cleared in HttpCache::Transaction::DoHeadersPhaseCannotProceed(), which could result in DCHECKs when calling GetResponseInfo() when a transaction that was waiting on a cached response from another transaction ended up failing. [M90]: Fixed trivial conflict (cherry picked from commit 2f49a3c69a2184c95f43a395e4f33a3959cb8dbc) (cherry picked from commit baf23e3c5b1394982cff718a0e055d4f239245ad) Bug: 1209769 Change-Id: I2c18caa488767a29011aca1e1b0bace24c1ba8fc Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2922826 Reviewed-by: Maksim Orlovich Commit-Queue: Matt Menke Cr-Original-Original-Commit-Position: refs/heads/master@{#887522} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2935241 Auto-Submit: Matt Menke Cr-Original-Commit-Position: refs/branch-heads/4472@{#1433} Cr-Original-Branched-From: 3d60439cfb36485e76a1c5bb7f513d3721b20da1-refs/heads/master@{#870763} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2948654 Owners-Override: Victor-Gabriel Savu Reviewed-by: Artem Sumaneev Reviewed-by: Matt Menke Commit-Queue: Victor-Gabriel Savu Cr-Commit-Position: refs/branch-heads/4430@{#1513} Cr-Branched-From: e5ce7dc4f7518237b3d9bb93cccca35d25216cbe-refs/heads/master@{#857950} diff --git a/net/http/http_cache_transaction.cc b/net/http/http_cache_transaction.cc index bb0b938e71f92c2743df2c12824e8f634f6ed5e2..6a1ccb4657f8de1a14396366b560bc11f4cee829 100644 --- a/net/http/http_cache_transaction.cc +++ b/net/http/http_cache_transaction.cc @@ -2115,6 +2115,8 @@ int HttpCache::Transaction::DoHeadersPhaseCannotProceed(int result) { entry_ = nullptr; new_entry_ = nullptr; + SetResponse(HttpResponseInfo()); + // Bypass the cache for timeout scenario. if (result == ERR_CACHE_LOCK_TIMEOUT) effective_load_flags_ |= LOAD_DISABLE_CACHE; diff --git a/services/network/cors/cors_url_loader_factory.cc b/services/network/cors/cors_url_loader_factory.cc index 7e4d57cf7f2f8e4e0aa208f53f83833fc0c68112..bf744d52473e31fb100ccf1644abd5e9122ba669 100644 --- a/services/network/cors/cors_url_loader_factory.cc +++ b/services/network/cors/cors_url_loader_factory.cc @@ -228,7 +228,17 @@ CorsURLLoaderFactory::CorsURLLoaderFactory( &CorsURLLoaderFactory::DeleteIfNeeded, base::Unretained(this))); } -CorsURLLoaderFactory::~CorsURLLoaderFactory() = default; +CorsURLLoaderFactory::~CorsURLLoaderFactory() { + // Delete loaders one at a time, since deleting one loader can cause another + // loader waiting on it to fail synchronously, which could result in the other + // loader calling DestroyURLLoader(). + while (!loaders_.empty()) { + // No need to call context_->LoaderDestroyed(), since this method is only + // called from the NetworkContext's destructor, or when there are no + // remaining URLLoaders. + loaders_.erase(loaders_.begin()); + } +} void CorsURLLoaderFactory::OnLoaderCreated( std::unique_ptr loader) { diff --git a/services/network/cors/cors_url_loader_factory_unittest.cc b/services/network/cors/cors_url_loader_factory_unittest.cc index 13811282f5bc922cce50a2d0c0c041d1cdd586c1..dd3361f818590031b8a843595bea1223e012bf3e 100644 --- a/services/network/cors/cors_url_loader_factory_unittest.cc +++ b/services/network/cors/cors_url_loader_factory_unittest.cc @@ -7,7 +7,9 @@ #include "base/macros.h" #include "base/test/task_environment.h" #include "mojo/public/cpp/bindings/remote.h" +#include "net/base/load_flags.h" #include "net/proxy_resolution/configured_proxy_resolution_service.h" +#include "net/test/embedded_test_server/embedded_test_server.h" #include "net/traffic_annotation/network_traffic_annotation_test_helper.h" #include "net/url_request/url_request_context.h" #include "net/url_request/url_request_context_builder.h" @@ -49,6 +51,9 @@ class CorsURLLoaderFactoryTest : public testing::Test { protected: // testing::Test implementation. void SetUp() override { + test_server_.AddDefaultHandlers(); + ASSERT_TRUE(test_server_.Start()); + network_service_ = NetworkService::CreateForTesting(); auto context_params = mojom::NetworkContextParams::New(); @@ -68,7 +73,7 @@ class CorsURLLoaderFactoryTest : public testing::Test { auto factory_params = network::mojom::URLLoaderFactoryParams::New(); factory_params->process_id = kProcessId; factory_params->request_initiator_origin_lock = - url::Origin::Create(GURL("http://localhost")); + url::Origin::Create(test_server_.base_url()); auto resource_scheduler_client = base::MakeRefCounted( kProcessId, kRouteId, &resource_scheduler_, @@ -81,15 +86,25 @@ class CorsURLLoaderFactoryTest : public testing::Test { } void CreateLoaderAndStart(const ResourceRequest& request) { + url_loaders_.emplace_back(mojo::Remote()); + test_cors_loader_clients_.emplace_back( + std::make_unique()); cors_url_loader_factory_->CreateLoaderAndStart( - url_loader_.BindNewPipeAndPassReceiver(), kRouteId, kRequestId, + url_loaders_.back().BindNewPipeAndPassReceiver(), kRouteId, kRequestId, mojom::kURLLoadOptionNone, request, - test_cors_loader_client_.CreateRemote(), + test_cors_loader_clients_.back()->CreateRemote(), net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS)); } void ResetFactory() { cors_url_loader_factory_.reset(); } + net::test_server::EmbeddedTestServer* test_server() { return &test_server_; } + + std::vector>& + test_cors_loader_clients() { + return test_cors_loader_clients_; + } + private: // Test environment. base::test::TaskEnvironment task_environment_; @@ -99,15 +114,17 @@ class CorsURLLoaderFactoryTest : public testing::Test { std::unique_ptr network_context_; mojo::Remote network_context_remote_; + net::test_server::EmbeddedTestServer test_server_; + // CorsURLLoaderFactory instance under tests. std::unique_ptr cors_url_loader_factory_; mojo::Remote cors_url_loader_factory_remote_; - // Holds URLLoader that CreateLoaderAndStart() creates. - mojo::Remote url_loader_; + // Holds the URLLoaders that CreateLoaderAndStart() creates. + std::vector> url_loaders_; - // TestURLLoaderClient that records callback activities. - TestURLLoaderClient test_cors_loader_client_; + // TestURLLoaderClients that record callback activities. + std::vector> test_cors_loader_clients_; // Holds for allowed origin access lists. OriginAccessList origin_access_list_; @@ -118,7 +135,7 @@ class CorsURLLoaderFactoryTest : public testing::Test { // Regression test for https://crbug.com/906305. TEST_F(CorsURLLoaderFactoryTest, DestructionOrder) { ResourceRequest request; - GURL url("http://localhost"); + GURL url = test_server()->GetURL("/hung"); request.mode = mojom::RequestMode::kNoCors; request.credentials_mode = mojom::CredentialsMode::kOmit; request.method = net::HttpRequestHeaders::kGetMethod; @@ -141,5 +158,36 @@ TEST_F(CorsURLLoaderFactoryTest, DestructionOrder) { ResetFactory(); } +TEST_F(CorsURLLoaderFactoryTest, CleanupWithSharedCacheObjectInUse) { + // Create a loader for a response that hangs after receiving headers, and run + // it until headers are received. + ResourceRequest request; + GURL url = test_server()->GetURL("/hung-after-headers"); + request.mode = mojom::RequestMode::kNoCors; + request.credentials_mode = mojom::CredentialsMode::kOmit; + request.method = net::HttpRequestHeaders::kGetMethod; + request.url = url; + request.request_initiator = url::Origin::Create(url); + CreateLoaderAndStart(request); + test_cors_loader_clients().back()->RunUntilResponseReceived(); + + // Read only requests will fail synchonously on destruction of the request + // they're waiting on if they're in the |done_headers_queue| when the other + // request fails. Make a large number of such requests, spin the message loop + // so they end up blocked on the hung request, and then destroy all loads. A + // large number of loaders is needed because they're stored in a set, indexed + // by address, so teardown order is random. + request.load_flags = + net::LOAD_ONLY_FROM_CACHE | net::LOAD_SKIP_CACHE_VALIDATION; + for (int i = 0; i < 10; ++i) + CreateLoaderAndStart(request); + base::RunLoop().RunUntilIdle(); + + // This should result in a crash if tearing down one URLLoaderFactory + // resulting in a another one failing causes a crash during teardown. See + // https://crbug.com/1209769. + ResetFactory(); +} + } // namespace cors } // namespace network