Revert "Migrate ServicesCustomizationDocument to SimpleURLLoader"

This reverts commit 23e967ba.

Reason for revert: Chrome kept crashing when trying to log in on the login screen. See crbug.com/820302 for details.

Bug: 820302

Original change's description:
> Migrate ServicesCustomizationDocument to SimpleURLLoader
> 
> Bug: 773295
> Change-Id: Id129cea8341c14bd8da4b5374034423223f03859
> Reviewed-on: https://chromium-review.googlesource.com/884063
> Commit-Queue: Mark Pilgrim <pilgrim@chromium.org>
> Reviewed-by: James Cook <jamescook@chromium.org>
> Reviewed-by: Matt Menke <mmenke@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#541884}

TBR=jamescook@chromium.org,mmenke@chromium.org,pilgrim@chromium.org

Change-Id: Ibaf3b0e0f98edf34f79374e85101b705418e7588
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 773295
Reviewed-on: https://chromium-review.googlesource.com/956763Reviewed-by: Xiaoqian Dai <xdai@chromium.org>
Commit-Queue: Xiaoqian Dai <xdai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541964}

chrome/browser/chromeos/BUILD.gn

@@ -130,7 +130,6 @@ source_set("chromeos") {
     "//components/viz/host",
     "//services/identity/public/cpp",
     "//services/metrics/public/cpp:ukm_builders",
-    "//services/network:network_service",
     "//services/resource_coordinator/public/cpp:resource_coordinator_cpp",
     "//third_party/fontconfig",
     "//third_party/metrics_proto",

chrome/browser/chromeos/DEPS

@@ -27,7 +27,6 @@ include_rules = [
   "+mojo/edk/embedder",
   "+services/device/public",
   "+services/metrics/public",
-  "+services/network",
   "+services/tracing/public",
   "+services/viz/public/interfaces",
   # Chromeos should not use ozone directly, it must go through mojo as ozone

chrome/browser/chromeos/customization/customization_document.cc

@@ -34,7 +34,6 @@
 #include "chrome/browser/chromeos/net/delay_network_call.h"
 #include "chrome/browser/extensions/external_loader.h"
 #include "chrome/browser/extensions/external_provider_impl.h"
-#include "chrome/browser/net/system_network_context_manager.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service.h"
 #include "chrome/browser/ui/app_list/app_list_syncable_service_factory.h"
@@ -45,14 +44,11 @@
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "content/public/browser/browser_thread.h"
-#include "content/public/browser/storage_partition.h"
 #include "extensions/common/extension_urls.h"
 #include "net/base/load_flags.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
-#include "services/network/public/cpp/resource_request.h"
-#include "services/network/public/cpp/simple_url_loader.h"
-#include "services/network/url_loader_factory.h"
+#include "net/url_request/url_fetcher.h"
 
 namespace chromeos {
 namespace {
@@ -574,40 +570,14 @@ void ServicesCustomizationDocument::StartFileFetch() {
 }
 
 void ServicesCustomizationDocument::DoStartFileFetch() {
-  net::NetworkTrafficAnnotationTag traffic_annotation =
-      net::DefineNetworkTrafficAnnotation("chromeos_customization_document", R"(
-          semantics {
-            sender: "Chrome OS Services Customization"
-            description:
-              "Chrome OS downloads the OEM services customization manifest."
-            trigger:
-              "When a public session starts on managed devices and an OEM "
-              "has uploaded a service customization document."
-            data:
-              "URL of the OEM service customization document. "
-              "No user information is sent."
-            destination: WEBSITE
-          }
-          policy {
-            cookies_allowed: NO
-            setting: "Unconditionally enabled on Chrome OS."
-          })");
-  auto resource_request = std::make_unique<network::ResourceRequest>();
-  resource_request->url = url_;
-  resource_request->load_flags =
-      net::LOAD_DO_NOT_SEND_COOKIES | net::LOAD_DO_NOT_SAVE_COOKIES |
-      net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_SEND_AUTH_DATA;
-  resource_request->headers.SetHeader("Accept", "application/json");
-  simple_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
-                                                    traffic_annotation);
-  network::mojom::URLLoaderFactory* loader_factory = url_loader_factory_;
-  if (!loader_factory) {
-    g_browser_process->system_network_context_manager()->GetURLLoaderFactory();
-  }
-  simple_loader_->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
-      loader_factory,
-      base::BindOnce(&ServicesCustomizationDocument::OnSimpleLoaderComplete,
-                     base::Unretained(this)));
+  url_fetcher_ = net::URLFetcher::Create(url_, net::URLFetcher::GET, this);
+  url_fetcher_->SetRequestContext(g_browser_process->system_request_context());
+  url_fetcher_->AddExtraRequestHeader("Accept: application/json");
+  url_fetcher_->SetLoadFlags(net::LOAD_DO_NOT_SEND_COOKIES |
+                             net::LOAD_DO_NOT_SAVE_COOKIES |
+                             net::LOAD_DISABLE_CACHE |
+                             net::LOAD_DO_NOT_SEND_AUTH_DATA);
+  url_fetcher_->Start();
 }
 
 bool ServicesCustomizationDocument::LoadManifestFromString(
@@ -639,30 +609,26 @@ void ServicesCustomizationDocument::OnManifestLoaded() {
   }
 }
 
-void ServicesCustomizationDocument::OnSimpleLoaderComplete(
-    std::unique_ptr<std::string> response_body) {
-  int response_code = 0;
-  bool retry = true;
-  if (simple_loader_->ResponseInfo() &&
-      simple_loader_->ResponseInfo()->headers) {
-    response_code = simple_loader_->ResponseInfo()->headers->response_code();
-    std::string mime_type;
-    simple_loader_->ResponseInfo()->headers->GetMimeType(&mime_type);
-    if (mime_type == "application/json" and response_body) {
-      LoadManifestFromString(*response_body);
-      retry = false;
-    } else if (response_code == net::HTTP_NOT_FOUND) {
-      LOG(ERROR) << "Customization manifest is missing on server: "
-                 << url_.spec();
-      OnCustomizationNotFound();
-      retry = false;
-    }
-  }
-  if (retry) {
+void ServicesCustomizationDocument::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  std::string mime_type;
+  std::string data;
+  if (source->GetStatus().is_success() &&
+      source->GetResponseCode() == net::HTTP_OK &&
+      source->GetResponseHeaders()->GetMimeType(&mime_type) &&
+      mime_type == "application/json" &&
+      source->GetResponseAsString(&data)) {
+    LoadManifestFromString(data);
+  } else if (source->GetResponseCode() == net::HTTP_NOT_FOUND) {
+    LOG(ERROR) << "Customization manifest is missing on server: "
+               << source->GetURL().spec();
+    OnCustomizationNotFound();
+  } else {
     if (num_retries_ < kMaxFetchRetries) {
       num_retries_++;
       content::BrowserThread::PostDelayedTask(
-          content::BrowserThread::UI, FROM_HERE,
+          content::BrowserThread::UI,
+          FROM_HERE,
           base::Bind(&ServicesCustomizationDocument::StartFileFetch,
                      weak_ptr_factory_.GetWeakPtr()),
           base::TimeDelta::FromSeconds(kRetriesDelayInSec));
@@ -670,8 +636,8 @@ void ServicesCustomizationDocument::OnSimpleLoaderComplete(
     }
     // This doesn't stop fetching manifest on next restart.
     LOG(ERROR) << "URL fetch for services customization failed:"
-               << " response code = " << response_code
-               << " URL = " << url_.spec();
+               << " response code = " << source->GetResponseCode()
+               << " URL = " << source->GetURL().spec();
 
     LogManifestLoadResult(HISTOGRAM_LOAD_RESULT_RETRIES_FAIL);
   }
@@ -811,8 +777,8 @@ void ServicesCustomizationDocument::SetOemFolderName(
 std::string ServicesCustomizationDocument::GetOemAppsFolderNameImpl(
     const std::string& locale,
     const base::DictionaryValue& root) const {
-  return GetLocaleSpecificStringImpl(&root, locale, kLocalizedContent,
-                                     kDefaultAppsFolderName);
+  return GetLocaleSpecificStringImpl(
+      &root, locale, kLocalizedContent, kDefaultAppsFolderName);
 }
 
 // static
@@ -885,9 +851,10 @@ void ServicesCustomizationDocument::CheckAndApplyWallpaper() {
 
   std::unique_ptr<bool> exists(new bool(false));
 
-  base::Closure check_file_exists = base::Bind(
-      &CheckWallpaperCacheExists, GetCustomizedWallpaperDownloadedFileName(),
-      base::Unretained(exists.get()));
+  base::Closure check_file_exists =
+      base::Bind(&CheckWallpaperCacheExists,
+                 GetCustomizedWallpaperDownloadedFileName(),
+                 base::Unretained(exists.get()));
   base::Closure on_checked_closure = base::Bind(
       &ServicesCustomizationDocument::OnCheckedWallpaperCacheExists,
       weak_ptr_factory_.GetWeakPtr(), base::Passed(std::move(exists)),

chrome/browser/chromeos/customization/customization_document.h

@@ -17,6 +17,7 @@
 #include "base/memory/singleton.h"
 #include "base/memory/weak_ptr.h"
 #include "base/values.h"
+#include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
 class PrefRegistrySimple;
@@ -31,14 +32,8 @@ namespace extensions {
 class ExternalLoader;
 }
 
-namespace network {
-class SimpleURLLoader;
-}
-
-namespace network {
-namespace mojom {
-class URLLoaderFactory;
-}
+namespace net {
+class URLFetcher;
 }
 
 namespace user_prefs {
@@ -146,7 +141,8 @@ class StartupCustomizationDocument : public CustomizationDocument {
 // outside this class by calling StartFetching() or EnsureCustomizationApplied()
 // methods.
 // User of the file should check IsReady before use it.
-class ServicesCustomizationDocument : public CustomizationDocument {
+class ServicesCustomizationDocument : public CustomizationDocument,
+                                      private net::URLFetcherDelegate {
  public:
   static ServicesCustomizationDocument* GetInstance();
 
@@ -204,11 +200,6 @@ class ServicesCustomizationDocument : public CustomizationDocument {
     return wallpaper_downloader_.get();
   }
 
-  void SetURLLoaderFactoryForTesting(
-      network::mojom::URLLoaderFactory* url_loader_factory) {
-    url_loader_factory_ = url_loader_factory;
-  }
-
  private:
   friend struct base::DefaultSingletonTraits<ServicesCustomizationDocument>;
   FRIEND_TEST_ALL_PREFIXES(CustomizationWallpaperDownloaderBrowserTest,
@@ -236,7 +227,8 @@ class ServicesCustomizationDocument : public CustomizationDocument {
   // Overriden from CustomizationDocument:
   bool LoadManifestFromString(const std::string& manifest) override;
 
-  void OnSimpleLoaderComplete(std::unique_ptr<std::string> response_body);
+  // Overriden from net::URLFetcherDelegate:
+  void OnURLFetchComplete(const net::URLFetcher* source) override;
 
   // Initiate file fetching. Wait for online status.
   void StartFileFetch();
@@ -301,6 +293,9 @@ class ServicesCustomizationDocument : public CustomizationDocument {
   // Services customization manifest URL.
   GURL url_;
 
+  // URLFetcher instance.
+  std::unique_ptr<net::URLFetcher> url_fetcher_;
+
   // How many times we already tried to fetch customization manifest file.
   int num_retries_;
 
@@ -314,7 +309,6 @@ class ServicesCustomizationDocument : public CustomizationDocument {
   ExternalLoaders external_loaders_;
 
   std::unique_ptr<CustomizationWallpaperDownloader> wallpaper_downloader_;
-  std::unique_ptr<network::SimpleURLLoader> simple_loader_;
 
   // This is barrier until customization is applied.
   // When number of finished tasks match number of started - customization is
@@ -327,9 +321,6 @@ class ServicesCustomizationDocument : public CustomizationDocument {
   // successfully.
   size_t apply_tasks_success_;
 
-  // Only used for unit tests. Not owned.
-  network::mojom::URLLoaderFactory* url_loader_factory_;
-
   // Weak factory for callbacks.
   base::WeakPtrFactory<ServicesCustomizationDocument> weak_ptr_factory_;
 

chrome/browser/chromeos/customization/customization_document_unittest.cc

@@ -32,10 +32,13 @@
 #include "extensions/common/manifest.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
-#include "services/network/test/test_url_loader_factory.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_request_status.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using ::testing::Exactly;
+using ::testing::Invoke;
 using ::testing::Mock;
 using ::testing::_;
 using extensions::ExternalInstallInfoFile;
@@ -156,6 +159,30 @@ TEST(StartupCustomizationDocumentTest, BadManifest) {
   EXPECT_FALSE(customization.IsReady());
 }
 
+class TestURLFetcherCallback {
+ public:
+  std::unique_ptr<net::FakeURLFetcher> CreateURLFetcher(
+      const GURL& url,
+      net::URLFetcherDelegate* d,
+      const std::string& response_data,
+      net::HttpStatusCode response_code,
+      net::URLRequestStatus::Status status) {
+    std::unique_ptr<net::FakeURLFetcher> fetcher(
+        new net::FakeURLFetcher(url, d, response_data, response_code, status));
+    OnRequestCreate(url, fetcher.get());
+    return fetcher;
+  }
+  MOCK_METHOD2(OnRequestCreate,
+               void(const GURL&, net::FakeURLFetcher*));
+};
+
+void AddMimeHeader(const GURL& url, net::FakeURLFetcher* fetcher) {
+  scoped_refptr<net::HttpResponseHeaders> download_headers =
+      new net::HttpResponseHeaders("");
+  download_headers->AddHeader("Content-Type: application/json");
+  fetcher->set_response_headers(download_headers);
+}
+
 class MockExternalProviderVisitor
     : public extensions::ExternalProviderInterface::VisitorInterface {
  public:
@@ -176,6 +203,11 @@ class MockExternalProviderVisitor
 
 class ServicesCustomizationDocumentTest : public testing::Test {
  protected:
+  ServicesCustomizationDocumentTest()
+      : factory_(nullptr,
+                 base::Bind(&TestURLFetcherCallback::CreateURLFetcher,
+                            base::Unretained(&url_callback_))) {}
+
   // testing::Test:
   void SetUp() override {
     ServicesCustomizationDocument::InitializeForTesting();
@@ -226,24 +258,25 @@ class ServicesCustomizationDocumentTest : public testing::Test {
                            const std::string& manifest) {
     GURL url(base::StringPrintf(ServicesCustomizationDocument::kManifestUrl,
                                 id.c_str()));
-    network::ResourceResponseHead head;
-    head.headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
-    head.headers->AddHeader("Content-Type: application/json");
-    network::URLLoaderCompletionStatus status;
-    status.decoded_body_length = manifest.size();
-    factory_.AddResponse(url, head, manifest, status);
+    factory_.SetFakeResponse(url,
+                             manifest,
+                             net::HTTP_OK,
+                             net::URLRequestStatus::SUCCESS);
+    EXPECT_CALL(url_callback_, OnRequestCreate(url, _))
+      .Times(Exactly(1))
+      .WillRepeatedly(Invoke(AddMimeHeader));
   }
 
   void AddManifestNotFound(const std::string& id) {
     GURL url(base::StringPrintf(ServicesCustomizationDocument::kManifestUrl,
                                 id.c_str()));
-    network::ResourceResponseHead head;
-    head.headers = base::MakeRefCounted<net::HttpResponseHeaders>(
-        "HTTP/1.x 404 Not Found");
-    head.headers->AddHeader("Content-Type: application/json");
-    network::URLLoaderCompletionStatus status;
-    status.decoded_body_length = 0;
-    factory_.AddResponse(url, head, "", status);
+    factory_.SetFakeResponse(url,
+                             std::string(),
+                             net::HTTP_NOT_FOUND,
+                             net::URLRequestStatus::SUCCESS);
+    EXPECT_CALL(url_callback_, OnRequestCreate(url, _))
+      .Times(Exactly(1))
+      .WillRepeatedly(Invoke(AddMimeHeader));
   }
 
   std::unique_ptr<TestingProfile> CreateProfile() {
@@ -258,13 +291,13 @@ class ServicesCustomizationDocumentTest : public testing::Test {
     return profile_builder.Build();
   }
 
-  network::TestURLLoaderFactory factory_;
-
  private:
   content::TestBrowserThreadBundle thread_bundle_;
   system::ScopedFakeStatisticsProvider fake_statistics_provider_;
   ScopedCrosSettingsTestHelper scoped_cros_settings_test_helper_;
   TestingPrefServiceSimple local_state_;
+  TestURLFetcherCallback url_callback_;
+  net::FakeURLFetcherFactory factory_;
   NetworkPortalDetectorTestImpl network_portal_detector_;
 };
 
@@ -276,7 +309,6 @@ TEST_F(ServicesCustomizationDocumentTest, Basic) {
       ServicesCustomizationDocument::GetInstance();
   EXPECT_FALSE(doc->IsReady());
 
-  doc->SetURLLoaderFactoryForTesting(&factory_);
   doc->StartFetching();
   RunUntilIdle();
   EXPECT_TRUE(doc->IsReady());
@@ -314,7 +346,6 @@ TEST_F(ServicesCustomizationDocumentTest, NoCustomizationIdInVpd) {
       ServicesCustomizationDocument::GetInstance();
   EXPECT_FALSE(doc->IsReady());
 
-  doc->SetURLLoaderFactoryForTesting(&factory_);
   std::unique_ptr<TestingProfile> profile = CreateProfile();
   extensions::ExternalLoader* loader = doc->CreateExternalLoader(profile.get());
   EXPECT_TRUE(loader);
@@ -351,7 +382,6 @@ TEST_F(ServicesCustomizationDocumentTest, DefaultApps) {
       ServicesCustomizationDocument::GetInstance();
   EXPECT_FALSE(doc->IsReady());
 
-  doc->SetURLLoaderFactoryForTesting(&factory_);
   std::unique_ptr<TestingProfile> profile = CreateProfile();
   extensions::ExternalLoader* loader = doc->CreateExternalLoader(profile.get());
   EXPECT_TRUE(loader);
@@ -402,7 +432,6 @@ TEST_F(ServicesCustomizationDocumentTest, CustomizationManifestNotFound) {
       ServicesCustomizationDocument::GetInstance();
   EXPECT_FALSE(doc->IsReady());
 
-  doc->SetURLLoaderFactoryForTesting(&factory_);
   std::unique_ptr<TestingProfile> profile = CreateProfile();
   extensions::ExternalLoader* loader = doc->CreateExternalLoader(profile.get());
   EXPECT_TRUE(loader);