Display DNS probe results.

chrome/browser/net/dns_probe_browsertest.cc

Index: chrome/browser/net/dns_probe_browsertest.cc
diff --git a/chrome/browser/net/dns_probe_browsertest.cc b/chrome/browser/net/dns_probe_browsertest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..18877c097ebbc97b813af04c7e9b669c5f384f26
--- /dev/null
+++ b/chrome/browser/net/dns_probe_browsertest.cc
@@ -0,0 +1,410 @@
+// Copyright (c) 2013 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/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/message_loop.h"
+#include "base/path_service.h"
+#include "base/threading/thread_restrictions.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/google/google_util.h"
+#include "chrome/browser/io_thread.h"
+#include "chrome/browser/net/dns_probe_test_util.h"
+#include "chrome/browser/net/net_error_tab_helper.h"
+#include "chrome/browser/net/url_request_mock_util.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/chrome_paths.h"
+#include "chrome/common/net/net_error_info.h"
+#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/test/browser_test_utils.h"
+#include "content/test/net/url_request_failed_job.h"
+#include "content/test/net/url_request_mock_http_job.h"
+#include "net/base/net_errors.h"
+#include "net/dns/dns_test_util.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_job.h"
+#include "net/url_request/url_request_job_factory.h"
+
+using base::Bind;
+using base::Callback;
+using base::Closure;
+using base::ConstRef;
+using base::FilePath;
+using base::MessageLoop;
+using base::Unretained;
+using chrome_common_net::DnsProbeStatus;
+using content::BrowserThread;
+using content::URLRequestFailedJob;
+using content::URLRequestMockHTTPJob;
+using content::WebContents;
+using google_util::LinkDoctorBaseURL;
+using net::MockDnsClientRule;
+using net::NetworkDelegate;
+using net::URLRequest;
+using net::URLRequestFilter;
+using net::URLRequestJob;
+using net::URLRequestJobFactory;
+using ui_test_utils::NavigateToURL;
+using ui_test_utils::NavigateToURLBlockUntilNavigationsComplete;
+
+namespace chrome_browser_net {
+
+namespace {
+
+FilePath GetMockLinkDoctorFilePath() {
+  FilePath root_http;
+  PathService::Get(chrome::DIR_TEST_DATA, &root_http);
+  return root_http.AppendASCII("mock-link-doctor.html");
+}
+
+class BrokenLinkDoctorProtocolHandlerDelegate {
+ public:
+  virtual int link_doctor_net_error() const = 0;

[mmenke, 2013/06/24 16:20:00]

nit:  Virtual functions shouldn't be named this way.  Also, do we really need an
abstract class for this?  Can just give the BrokenLinkDoctorProtocolHandler a
function to set the result, and have the DnsProbeBrowserTestIOThreadHelper call
that directly, so don't need to have the two classes talk to each other.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Done.

+};
+
+class BrokenLinkDoctorProtocolHandler
+    : public URLRequestJobFactory::ProtocolHandler {
+ public:
+  explicit BrokenLinkDoctorProtocolHandler(

[mmenke, 2013/06/24 16:20:00]

nit:  Explicit not needed (Unless you get rid of the second argument, per my
suggestion above).

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Done.

+      FilePath mock_link_doctor_file_path,
+      BrokenLinkDoctorProtocolHandlerDelegate* delegate)
+      : mock_link_doctor_file_path_(mock_link_doctor_file_path),
+        delegate_(delegate) {}
+
+  virtual URLRequestJob* MaybeCreateJob(
+      URLRequest* request, NetworkDelegate* network_delegate) const OVERRIDE {
+    int net_error = delegate_->link_doctor_net_error();
+    if (net_error != net::OK) {
+      return new URLRequestFailedJob(request, network_delegate, net_error);
+    } else {
+      return new URLRequestMockHTTPJob(
+          request, network_delegate, mock_link_doctor_file_path_);
+    }
+  }
+
+ private:
+  const FilePath mock_link_doctor_file_path_;
+  const BrokenLinkDoctorProtocolHandlerDelegate* delegate_;
+};
+
+class DnsProbeBrowserTestIOThreadHelper
+    : public BrokenLinkDoctorProtocolHandlerDelegate {
+ public:
+  DnsProbeBrowserTestIOThreadHelper();
+  virtual ~DnsProbeBrowserTestIOThreadHelper() {}
+
+  void SetUpOnIOThread(IOThread* io_thread);
+  void CleanUpOnIOThreadAndDeleteHelper();
+
+  void SetMockDnsClientRules(MockDnsClientRule::Result system_good_result,
+                             MockDnsClientRule::Result public_good_result);
+
+  virtual int link_doctor_net_error() const OVERRIDE {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    return link_doctor_net_error_;
+  }
+
+  void set_link_doctor_net_error(int link_doctor_net_error) {
+    DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    link_doctor_net_error_ = link_doctor_net_error;
+  }
+
+ private:
+  IOThread* io_thread_;
+  FilePath mock_link_doctor_file_path_;
+  int link_doctor_net_error_;
+};
+
+DnsProbeBrowserTestIOThreadHelper::DnsProbeBrowserTestIOThreadHelper()
+    : io_thread_(NULL),
+      mock_link_doctor_file_path_(GetMockLinkDoctorFilePath()),
+      link_doctor_net_error_(net::OK) {}
+
+void DnsProbeBrowserTestIOThreadHelper::SetUpOnIOThread(IOThread* io_thread) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));

[mmenke, 2013/06/24 16:20:00]

Should use CHECKS (Or asserts) in tests, since we want the release build to fail
with them, too.  The CQ, in particular, only runs tests with release builds.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Done.  Do any of the DCHECKs in the non-test code want to change, also?

[mmenke, 2013/06/25 17:40:26]

My general feeling is that production code should avoid CHECKs unless there's
something we're particularly concerned about failing / regressing, just to avoid
bloating up the binary.

+  DCHECK(io_thread);
+  DCHECK(!io_thread_);
+
+  io_thread_ = io_thread;
+
+  URLRequestFailedJob::AddUrlHandler();
+
+  const GURL link_doctor_base_url = LinkDoctorBaseURL();
+  const std::string link_doctor_host = link_doctor_base_url.host();
+  scoped_ptr<URLRequestJobFactory::ProtocolHandler> link_doctor_handler(
+      new BrokenLinkDoctorProtocolHandler(mock_link_doctor_file_path_, this));
+  URLRequestFilter::GetInstance()->AddHostnameProtocolHandler(
+      "http", link_doctor_host, link_doctor_handler.Pass());
+}
+
+void DnsProbeBrowserTestIOThreadHelper::CleanUpOnIOThreadAndDeleteHelper() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  DCHECK(io_thread_);
+
+  URLRequestFilter::GetInstance()->ClearHandlers();
+
+  delete this;
+}
+
+void DnsProbeBrowserTestIOThreadHelper::SetMockDnsClientRules(
+    MockDnsClientRule::Result system_good_result,
+    MockDnsClientRule::Result public_good_result) {
+  DnsProbeService* service = io_thread_->globals()->dns_probe_service.get();
+  service->SetSystemClientForTesting(
+      CreateMockDnsClientForProbes(system_good_result));
+  service->SetPublicClientForTesting(
+      CreateMockDnsClientForProbes(public_good_result));
+}
+
+class DnsProbeBrowserTest : public InProcessBrowserTest {
+ public:
+  DnsProbeBrowserTest();
+
+  virtual void SetUpOnMainThread() OVERRIDE;
+  virtual void CleanUpOnMainThread() OVERRIDE;
+
+ protected:
+  void SetLinkDoctorBroken(bool broken);
+  void SetMockDnsClientRules(MockDnsClientRule::Result system_good_result,
+                MockDnsClientRule::Result public_good_result);
+  void NavigateToDnsError();
+  void NavigateToOtherError();
+
+  void WaitForNetErrorInfo();
+
+  bool TitleIs(const std::string& expected);
+  bool PageContains(const std::string& expected);
+
+  DnsProbeStatus last_dns_probe_status() const {
+    return last_dns_probe_status_;
+  }
+  int net_error_info_count() const { return net_error_info_count_; }
+
+ private:
+  void OnNetErrorInfoSent(DnsProbeStatus dns_probe_status);
+
+  DnsProbeBrowserTestIOThreadHelper* helper_;
+
+  bool awaiting_net_error_info_;
+  bool received_net_error_info_;
+  int net_error_info_count_;

[mmenke, 2013/06/24 16:20:00]

We're actually waiting / monitoring dns_probe_status, not net_error_info (x4).

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Yeah, but what we receive is a NetErrorInfo message containing the
DnsProbeStatus.  (I can change the name if you want, but I named it for the
message it's waiting for.)

+  DnsProbeStatus last_dns_probe_status_;
+};
+
+DnsProbeBrowserTest::DnsProbeBrowserTest()
+    : helper_(new DnsProbeBrowserTestIOThreadHelper()),
+      awaiting_net_error_info_(false),
+      received_net_error_info_(false),
+      net_error_info_count_(0) {
+}
+
+void DnsProbeBrowserTest::SetUpOnMainThread() {
+  NetErrorTabHelper::set_state_for_testing(
+      NetErrorTabHelper::TESTING_FORCE_ENABLED);
+
+  DCHECK(helper_);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::SetUpOnIOThread,
+           Unretained(helper_),
+           g_browser_process->io_thread()));
+
+  NetErrorTabHelper* tab_helper = NetErrorTabHelper::FromWebContents(
+      browser()->tab_strip_model()->GetActiveWebContents());
+  tab_helper->set_net_error_info_snoop_callback_for_testing(Bind(
+      &DnsProbeBrowserTest::OnNetErrorInfoSent,
+      Unretained(this)));
+}
+
+void DnsProbeBrowserTest::CleanUpOnMainThread() {
+  DCHECK(helper_);
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::CleanUpOnIOThreadAndDeleteHelper,
+           Unretained(helper_)));
+
+  NetErrorTabHelper::set_state_for_testing(
+      NetErrorTabHelper::TESTING_DEFAULT);
+}
+
+void DnsProbeBrowserTest::SetLinkDoctorBroken(bool broken) {
+  int net_error = broken ? net::ERR_NAME_NOT_RESOLVED : net::OK;
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::set_link_doctor_net_error,
+           Unretained(helper_),
+           net_error));
+}
+
+// These two functions wait for two navigations because Link Doctor loads two
+// pages: a blank page, so the user stops seeing the previous page, and then
+// either the Link Doctor page or a regular error page.  We want to wait for
+// the error page, so we wait for both loads to finish.
+
+void DnsProbeBrowserTest::NavigateToDnsError() {
+  NavigateToURLBlockUntilNavigationsComplete(
+      browser(),
+      URLRequestFailedJob::GetMockHttpUrl(net::ERR_NAME_NOT_RESOLVED),
+      2);
+}
+
+void DnsProbeBrowserTest::NavigateToOtherError() {
+  NavigateToURLBlockUntilNavigationsComplete(
+      browser(),
+      URLRequestFailedJob::GetMockHttpUrl(net::ERR_CONNECTION_REFUSED),
+      2);
+}
+
+void DnsProbeBrowserTest::SetMockDnsClientRules(
+    MockDnsClientRule::Result system_good_result,
+    MockDnsClientRule::Result public_good_result) {
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      Bind(&DnsProbeBrowserTestIOThreadHelper::SetMockDnsClientRules,
+           Unretained(helper_),
+           system_good_result,
+           public_good_result));
+}
+
+void DnsProbeBrowserTest::WaitForNetErrorInfo() {
+  DCHECK(!awaiting_net_error_info_);
+  while (!received_net_error_info_) {
+    awaiting_net_error_info_ = true;
+    MessageLoop::current()->Run();
+    awaiting_net_error_info_ = false;
+  }
+}
+
+// Check title by roundtripping to renderer, to make sure any probe results
+// sent before this have been applied.

[mmenke, 2013/06/24 16:20:00]

Did you consider the title watcher?  The one concern with it is that if the
title is changed to the correct title and then to something else, it still
passes.  Not sure that's an issue for us.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

I did; I like this better, but I'll change it if you want.

[mmenke, 2013/06/26 15:48:20]

I've gone back and forth on this.  One one hand, it mirrors
DnsProbeBrowserTest::PageContains, so understanding it shouldn't add too much
overhead.

On the other hand, we already have a way for browser tests to wait for a
specific title.  If we used it, people familiar with that class don't have to
relearn another way of doing it in this file.

+bool DnsProbeBrowserTest::TitleIs(const std::string& expected) {
+  std::string title;
+
+  WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+
+  bool rv = content::ExecuteScriptAndExtractString(
+      contents,
+      "domAutomationController.send(document.title);",
+      &title);
+  if (!rv)
+    return false;
+
+  return title == expected;
+}
+
+// Check text by roundtripping to renderer, to make sure any probe results
+// sent before this have been applied.
+bool DnsProbeBrowserTest::PageContains(const std::string& expected) {
+  std::string text_content;
+
+  bool rv = content::ExecuteScriptAndExtractString(
+      browser()->tab_strip_model()->GetActiveWebContents(),
+      "domAutomationController.send(document.body.textContent);",
+      &text_content);
+  if (!rv)
+    return false;
+
+  return text_content.find(expected) != std::string::npos;
+}
+
+void DnsProbeBrowserTest::OnNetErrorInfoSent(DnsProbeStatus dns_probe_status) {
+  last_dns_probe_status_ = dns_probe_status;
+
+  net_error_info_count_++;
+  received_net_error_info_ = true;
+  if (awaiting_net_error_info_)
+    MessageLoop::current()->Quit();
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, OtherErrorWithLinkDoctor) {
+  SetLinkDoctorBroken(false);
+
+  NavigateToOtherError();
+  EXPECT_TRUE(TitleIs("Mock Link Doctor"));
+
+  EXPECT_EQ(0, net_error_info_count());
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, OtherErrorWithoutLinkDoctor) {
+  SetLinkDoctorBroken(true);
+
+  NavigateToOtherError();
+  EXPECT_TRUE(PageContains("CONNECTION_REFUSED"));
+
+  EXPECT_EQ(0, net_error_info_count());
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NxdomainWithLinkDoctor) {
+  SetLinkDoctorBroken(false);
+  SetMockDnsClientRules(MockDnsClientRule::OK, MockDnsClientRule::OK);
+
+  NavigateToDnsError();
+  EXPECT_TRUE(TitleIs("Mock Link Doctor"));
+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NoInternetWithoutLinkDoctor) {
+  SetLinkDoctorBroken(true);
+  SetMockDnsClientRules(MockDnsClientRule::TIMEOUT,
+                        MockDnsClientRule::TIMEOUT);
+
+  NavigateToDnsError();
+
+  bool seen_finished = false;
+  bool page_updated = false;
+
+  // Wait for zero or more DNS_PROBE_STARTED followed by one or more
+  // DNS_PROBE_FINISHED_NO_INTERNET.  After each of the latter, check to see if
+  // the error page has been updated.
+  while (!page_updated) {
+    WaitForNetErrorInfo();
+    switch (last_dns_probe_status()) {
+    case chrome_common_net::DNS_PROBE_STARTED:
+      ASSERT_FALSE(seen_finished);
+      break;
+    case chrome_common_net::DNS_PROBE_FINISHED_NO_INTERNET:
+      seen_finished = true;
+      if (PageContains("DNS_PROBE_FINISHED_NO_INTERNET"))
+        page_updated = true;
+      break;
+    default:
+      NOTREACHED();
+    }
+  }
+}
+
+static const FilePath::CharType kIframeDnsErrorHtmlName[] =
+    FILE_PATH_LITERAL("iframe_dns_error.html");

[mmenke, 2013/06/24 16:20:00]

Since this is only used in one test body, suggest just inlining it there.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Done.

+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, NoProbeInSubframe) {
+  SetLinkDoctorBroken(false);
+
+  NavigateToURL(
+      browser(),
+      URLRequestMockHTTPJob::GetMockUrl(FilePath(kIframeDnsErrorHtmlName)));
+  EXPECT_EQ(0, net_error_info_count());

[mmenke, 2013/06/24 16:20:00]

Think it's worth mentioning why these two tests won't flakily pass.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Added to NoProbeInSubFrame.  ProbesDisabled explicitly expects NOT_RUN, so I
don't see how it could flakily pass (run a probe) while claiming it didn't run a
probe.

[mmenke, 2013/06/25 17:40:26]

You're right, was still thinking of the pre-callback version.

+}
+
+IN_PROC_BROWSER_TEST_F(DnsProbeBrowserTest, ProbesDisabled) {
+  NetErrorTabHelper::set_state_for_testing(
+      NetErrorTabHelper::TESTING_FORCE_DISABLED);
+
+  SetLinkDoctorBroken(true);
+  SetMockDnsClientRules(MockDnsClientRule::TIMEOUT,
+                        MockDnsClientRule::TIMEOUT);
+
+  NavigateToDnsError();
+
+  WaitForNetErrorInfo();
+  EXPECT_EQ(chrome_common_net::DNS_PROBE_NOT_RUN, last_dns_probe_status());
+}

[mmenke, 2013/06/24 16:20:00]

Suggest a test with the link doctor diabled via prefs.

[mmenke, 2013/06/24 16:20:00]

Suggest a test with dns failing synchronously, just out of paranoia.  Know we
test it at the lower layer, too, but as that's a case where we risk a crash, as
opposed to just not updating the page properly, think it's worth extra testing.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

That's tricky; we're currently gated on a field trial as well.  I can force
probes on to avoid the field trial, but that bypasses the pref as well.

[Deprecated (see juliatuttle), 2013/06/25 16:45:04]

Done.

[mmenke, 2013/06/25 17:40:26]

The issue I'm concerned about isn't what we're gated on, it's what the link
doctor's second page load is gated on.  We don't even check that pref, do we?  I
just want to make sure we work when the error page has a single load, instead of
two loads.

+
+}  // namespace
+
+}  // namespace chrome_browser_net