Fix relative lifetime of URLLoaderFactory vs URLLoaders.

The code assumes that URLLoaderFactory is guaranteed to outlive
URLLoader (i.e. this is why URLLoader::factory_params_ is a raw
pointer).  This CL makes sure this assumption holds for the factory and
loaders owned by CORSURLLoaderFactory: |loaders_| need to be destroyed
*before* destroying |network_loader_factory_|.

Bug: 906305
Change-Id: I61195be425c8b21a725676de4d44b1611f443410
Reviewed-on: https://chromium-review.googlesource.com/c/1343282Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610459}

services/network/BUILD.gn

@@ -271,6 +271,7 @@ source_set("tests") {
     "chunked_data_pipe_upload_data_stream_unittest.cc",
     "cookie_manager_unittest.cc",
     "cookie_settings_unittest.cc",
+    "cors/cors_url_loader_factory_unittest.cc",
     "cors/cors_url_loader_unittest.cc",
     "cors/preflight_controller_unittest.cc",
     "cross_origin_read_blocking_unittest.cc",

services/network/cors/cors_url_loader_factory.h

@@ -79,11 +79,15 @@ class COMPONENT_EXPORT(NETWORK_SERVICE) CorsURLLoaderFactory final
   // The NetworkContext owns |this|.
   NetworkContext* const context_ = nullptr;
   scoped_refptr<ResourceSchedulerClient> resource_scheduler_client_;
-  std::set<std::unique_ptr<mojom::URLLoader>, base::UniquePtrComparator>
-      loaders_;
 
   const bool disable_web_security_;
+
+  // Relative order of |network_loader_factory_| and |loaders_| matters -
+  // URLLoaderFactory needs to live longer than URLLoaders created using the
+  // factory.  See also https://crbug.com/906305.
   std::unique_ptr<mojom::URLLoaderFactory> network_loader_factory_;
+  std::set<std::unique_ptr<mojom::URLLoader>, base::UniquePtrComparator>
+      loaders_;
 
   // Used when constructed by ResourceMessageFilter.
   base::RepeatingCallback<void(int)> preflight_finalizer_;

services/network/cors/cors_url_loader_factory_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#include "base/macros.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/scoped_task_environment.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"
+#include "services/network/cors/cors_url_loader_factory.h"
+#include "services/network/network_context.h"
+#include "services/network/network_service.h"
+#include "services/network/public/cpp/features.h"
+#include "services/network/public/cpp/resource_request.h"
+#include "services/network/public/mojom/url_loader.mojom.h"
+#include "services/network/public/mojom/url_loader_factory.mojom.h"
+#include "services/network/resource_scheduler.h"
+#include "services/network/resource_scheduler_client.h"
+#include "services/network/test/test_url_loader_client.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "url/gurl.h"
+
+namespace network {
+namespace cors {
+
+namespace {
+
+constexpr int kProcessId = 123;
+constexpr int kRequestId = 456;
+constexpr int kRouteId = 789;
+
+}  // namespace
+
+class CorsURLLoaderFactoryTest : public testing::Test {
+ public:
+  CorsURLLoaderFactoryTest()
+      : scoped_task_environment_(
+            base::test::ScopedTaskEnvironment::MainThreadType::IO),
+        resource_scheduler_(true) {
+    net::URLRequestContextBuilder context_builder;
+    context_builder.set_proxy_resolution_service(
+        net::ProxyResolutionService::CreateDirect());
+    url_request_context_ = context_builder.Build();
+  }
+
+ protected:
+  // testing::Test implementation.
+  void SetUp() override {
+    feature_list_.InitAndEnableFeature(features::kOutOfBlinkCors);
+
+    network_service_ = NetworkService::CreateForTesting();
+
+    auto context_params = mojom::NetworkContextParams::New();
+    // Use a fixed proxy config, to avoid dependencies on local network
+    // configuration.
+    context_params->initial_proxy_config =
+        net::ProxyConfigWithAnnotation::CreateDirect();
+    network_context_ = std::make_unique<NetworkContext>(
+        network_service_.get(), mojo::MakeRequest(&network_context_ptr_),
+        std::move(context_params));
+
+    auto factory_params = network::mojom::URLLoaderFactoryParams::New();
+    factory_params->process_id = kProcessId;
+    auto resource_scheduler_client =
+        base::MakeRefCounted<ResourceSchedulerClient>(
+            kProcessId, kRouteId, &resource_scheduler_,
+            url_request_context_->network_quality_estimator());
+    cors_url_loader_factory_ = std::make_unique<CorsURLLoaderFactory>(
+        network_context_.get(), std::move(factory_params),
+        resource_scheduler_client,
+        mojo::MakeRequest(&cors_url_loader_factory_ptr_), &origin_access_list_);
+  }
+
+  void CreateLoaderAndStart(const ResourceRequest& request) {
+    cors_url_loader_factory_->CreateLoaderAndStart(
+        mojo::MakeRequest(&url_loader_), kRouteId, kRequestId,
+        mojom::kURLLoadOptionNone, request,
+        test_cors_loader_client_.CreateInterfacePtr(),
+        net::MutableNetworkTrafficAnnotationTag(TRAFFIC_ANNOTATION_FOR_TESTS));
+  }
+
+  void ResetFactory() { cors_url_loader_factory_.reset(); }
+
+ private:
+  // Testing instance to enable kOutOfBlinkCors feature.
+  base::test::ScopedFeatureList feature_list_;
+
+  // Test environment.
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::unique_ptr<net::URLRequestContext> url_request_context_;
+  ResourceScheduler resource_scheduler_;
+  std::unique_ptr<NetworkService> network_service_;
+  std::unique_ptr<NetworkContext> network_context_;
+  mojom::NetworkContextPtr network_context_ptr_;
+
+  // CorsURLLoaderFactory instance under tests.
+  std::unique_ptr<mojom::URLLoaderFactory> cors_url_loader_factory_;
+  mojom::URLLoaderFactoryPtr cors_url_loader_factory_ptr_;
+
+  // Holds URLLoaderPtr that CreateLoaderAndStart() creates.
+  mojom::URLLoaderPtr url_loader_;
+
+  // TestURLLoaderClient that records callback activities.
+  TestURLLoaderClient test_cors_loader_client_;
+
+  // Holds for allowed origin access lists.
+  OriginAccessList origin_access_list_;
+
+  DISALLOW_COPY_AND_ASSIGN(CorsURLLoaderFactoryTest);
+};
+
+// Regression test for https://crbug.com/906305.
+TEST_F(CorsURLLoaderFactoryTest, DestructionOrder) {
+  ResourceRequest request;
+  GURL url("http://localhost");
+  request.fetch_request_mode = mojom::FetchRequestMode::kNoCors;
+  request.fetch_credentials_mode = mojom::FetchCredentialsMode::kOmit;
+  request.load_flags |= net::LOAD_DO_NOT_SAVE_COOKIES;
+  request.load_flags |= net::LOAD_DO_NOT_SEND_COOKIES;
+  request.load_flags |= net::LOAD_DO_NOT_SEND_AUTH_DATA;
+  request.method = net::HttpRequestHeaders::kGetMethod;
+  request.url = url;
+  request.request_initiator = url::Origin::Create(url);
+
+  // As of r609458 setting |keepalive| to true was triggerring a dereference of
+  // |factory_params_| in the destructor of network::URLLoader.  This
+  // dereference assumes that the network::URLLoaderFactory (which keeps
+  // |factory_params_| alive) lives longer than the network::URLLoaders created
+  // via the factory (which necessitates being careful with the destruction
+  // order of fields of network::cors::CorsURLLoaderFactory which owns both
+  // network::URLLoaderFactory and the network::URLLoaders it creates).
+  request.keepalive = true;
+
+  // Create a loader and immediately (while the loader is still stored in
+  // CorsURLLoaderFactory::loaders_ / not released via test_cors_loader_client_)
+  // destroy the factory.  If ASAN doesn't complain then the test passes.
+  CreateLoaderAndStart(request);
+  ResetFactory();
+}
+
+}  // namespace cors
+}  // namespace network