Domain Reliability: Don't crash on shutdown with uploads pending

chrome/browser/domain_reliability/browsertest.cc

Index: chrome/browser/domain_reliability/browsertest.cc
diff --git a/chrome/browser/domain_reliability/browsertest.cc b/chrome/browser/domain_reliability/browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..d18dee8816dce5c4c1ad37cf28f979db0bb4dcab
--- /dev/null
+++ b/chrome/browser/domain_reliability/browsertest.cc
@@ -0,0 +1,100 @@
+// Copyright 2016 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 "base/command_line.h"
+#include "base/macros.h"
+#include "chrome/browser/domain_reliability/service_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "components/domain_reliability/service.h"
+#include "net/test/url_request/url_request_hanging_read_job.h"
+
+namespace domain_reliability {
+
+class DomainReliabilityBrowserTest : public InProcessBrowserTest {

[mmenke, 2016/12/14 19:12:17]

Should probably call this file domain_reliability_browsertest (browsertest is
more common than browser_test, despite the fact that the "T" is generally
capitalized in test class names)

Or maybe clearer, go with DomainReliabilityServiceBrowserTest /
domain_reliability_service_browsertest .

[Julia Tuttle, 2016/12/15 21:52:59]

It's not a test of *just* the service, and Domain Reliability has the odd
convention of not including the domain_reliability_ prefix on *most* files. Is
there a reason for this one in particular to include it?

+ protected:

[mmenke, 2016/12/14 19:12:17]

optional:  Suggest just making these public, as protected, while more accurate
about how their used, is less intuitive.

[Julia Tuttle, 2016/12/15 21:52:59]

Done.

+  DomainReliabilityBrowserTest() {
+    // Note: This test's use of URLRequestHangingReadJob only works because the
+    // Uploader currently uses a URLFetcher, and therefore doesn't return from
+    // uploads until the body is completely downloaded.
+    //
+    // If some day it switches to returning after the headers and canceling the
+    // request after it has a response code, this test will need to use the
+    // (non-existent) URLRequestHangingStartJob.
+    net::URLRequestHangingReadJob::AddUrlHandler();

[mmenke, 2016/12/14 19:12:17]

See URLRequestFailedJob::GetMockHttp[s]Url(net::ERR_IO_PENDING);  (Oops, I
referred to this comment in an IM, but I hadn't published it yet)

[Julia Tuttle, 2016/12/15 21:53:00]

Oh! Good call. Done.

+  }
+
+  ~DomainReliabilityBrowserTest() override {}
+
+  // Note: In an ideal world, instead of appending the command-line switch and
+  // manually setting discard_uploads to false, Domain Reliability would
+  // continuously monitor the metrics reporting pref, and the test could just
+  // set the pref.
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kEnableDomainReliability);
+  }
+
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+
+    DomainReliabilityService* service = GetService();
+    if (service)
+      service->SetDiscardUploadsForTesting(false);
+  }
+
+  DomainReliabilityService* GetService() {
+    return DomainReliabilityServiceFactory::GetForBrowserContext(
+        browser()->profile());
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DomainReliabilityBrowserTest);
+};
+
+class DomainReliabilityDisabledBrowserTest
+    : public DomainReliabilityBrowserTest {
+ protected:
+  DomainReliabilityDisabledBrowserTest() {}
+
+  ~DomainReliabilityDisabledBrowserTest() override {}
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    command_line->AppendSwitch(switches::kDisableDomainReliability);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(DomainReliabilityDisabledBrowserTest);
+};
+
+IN_PROC_BROWSER_TEST_F(DomainReliabilityDisabledBrowserTest,
+                       ServiceNotCreated) {
+  EXPECT_FALSE(GetService());
+}
+
+IN_PROC_BROWSER_TEST_F(DomainReliabilityBrowserTest, ServiceCreated) {
+  EXPECT_TRUE(GetService());
+}
+
+IN_PROC_BROWSER_TEST_F(DomainReliabilityBrowserTest, UploadAtShutdown) {
+  DomainReliabilityService* service = GetService();
+
+  auto config = base::MakeUnique<DomainReliabilityConfig>();

[mmenke, 2016/12/14 19:12:17]

include base/memory/ptr_util.h

[Julia Tuttle, 2016/12/15 21:52:59]

Done.

+  config->origin = GURL("https://localhost/");

[mmenke, 2016/12/14 19:12:17]

include gurl.h

[Julia Tuttle, 2016/12/15 21:53:00]

Done.

+  config->include_subdomains = false;
+  config->collectors.push_back(
+      new GURL(net::URLRequestHangingReadJob::GetMockHttpsUrl()));
+  config->success_sample_rate = 1.0;
+  config->failure_sample_rate = 1.0;
+  service->AddContextForTesting(std::move(config));
+
+  ui_test_utils::NavigateToURL(browser(), GURL("https://localhost/"));
+
+  service->ForceUploadsForTesting();

[mmenke, 2016/12/14 19:12:17]

Think you need a comment about what this actually tests.

[Julia Tuttle, 2016/12/15 21:52:59]

Done.

+}
+
+}  // namespace domain_reliability