Support auto-reload on errors.

chrome/renderer/net/net_error_helper_core_unittest.cc

Index: chrome/renderer/net/net_error_helper_core_unittest.cc
diff --git a/chrome/renderer/net/net_error_helper_core_unittest.cc b/chrome/renderer/net/net_error_helper_core_unittest.cc
index bd451d6d876d34ac999d08f39c6e166ec80c5704..f2d650df5f3d0a8e0b4c4c80e339eedea46466cb 100644
--- a/chrome/renderer/net/net_error_helper_core_unittest.cc
+++ b/chrome/renderer/net/net_error_helper_core_unittest.cc
@@ -64,12 +64,51 @@ std::string NetErrorString(net::Error net_error) {
   return ErrorToString(NetError(net_error), false);
 }
 
+class MockOneShotTimer : public MockableOneShotTimer {
+ public:
+  MockOneShotTimer() {}
+  virtual ~MockOneShotTimer() {}
+
+  virtual void Start(const tracked_objects::Location& posted_from,
+                     base::TimeDelta delay,
+                     const base::Closure& task) {
+    EXPECT_FALSE(running_);
+    task_ = task;
+    running_ = true;
+    delay_ = delay;
+  }
+
+  virtual void Stop() {
+    running_ = false;
+  }
+
+  virtual bool IsRunning() const {
+    return running_;
+  }
+
+  void Fire() {
+    EXPECT_TRUE(running_);
+    running_ = false;
+    task_.Run();

[mmenke, 2014/02/25 22:09:52]

optional:  Out of paranoia, could reset the task here, though would have to
store it in a temporary for safety.  Then could check if it !task_.is_null()
wherever you check running_ (Could optionally get rid of running_ as well)

[Elly Fong-Jones, 2014/03/03 19:31:07]

I don't understand why we'd want to do this, I guess - the task is going to be
set every time Start() is called anyway.

+  }
+
+  base::TimeDelta delay() const { return delay_; }
+
+ private:
+  base::TimeDelta delay_;
+  base::Closure task_;
+  bool running_;
+};
+
 class NetErrorHelperCoreTest : public testing::Test,
                                public NetErrorHelperCore::Delegate {
  public:
-  NetErrorHelperCoreTest() : core_(this),
+  NetErrorHelperCoreTest() : timer_(new MockOneShotTimer()),
+                             core_(this,
+                                   scoped_ptr<MockableOneShotTimer>(timer_)),
                              update_count_(0),
-                             error_html_update_count_(0) {
+                             error_html_update_count_(0),
+                             reload_count_(0) {
   }
 
   virtual ~NetErrorHelperCoreTest() {
@@ -82,12 +121,18 @@ class NetErrorHelperCoreTest : public testing::Test,
   const GURL& url_being_fetched() const { return url_being_fetched_; }
   bool is_url_being_fetched() const { return !url_being_fetched_.is_empty(); }
 
+  int reload_count() const {
+    return reload_count_;
+  }
+
   const std::string& last_update_string() const { return last_update_string_; }
   int update_count() const { return update_count_;  }
 
   const std::string& last_error_html() const { return last_error_html_; }
   int error_html_update_count() const { return error_html_update_count_; }
 
+  MockOneShotTimer* timer() { return timer_; }
+
   void LinkDoctorLoadSuccess() {
     LinkDoctorLoadFinished(kLinkDoctorBody);
   }
@@ -96,6 +141,34 @@ class NetErrorHelperCoreTest : public testing::Test,
     LinkDoctorLoadFinished("");
   }
 
+ protected:

[mmenke, 2014/02/25 22:09:52]

Don't think there's any point of using protected in test fixtures.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+  void DoErrorLoad(net::Error error) {
+    core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                       NetErrorHelperCore::NON_ERROR_PAGE);
+    std::string html;
+    core().GetErrorHTML(NetErrorHelperCore::MAIN_FRAME,
+                        NetError(error), false, &html);
+    EXPECT_FALSE(html.empty());
+    EXPECT_EQ(NetErrorString(error), html);
+
+    core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                       NetErrorHelperCore::ERROR_PAGE);
+    core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME);
+    core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME);
+  }
+
+  void DoSuccessLoad() {
+    core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                       NetErrorHelperCore::NON_ERROR_PAGE);
+    core().OnCommitLoad(NetErrorHelperCore::MAIN_FRAME);
+    core().OnFinishLoad(NetErrorHelperCore::MAIN_FRAME);
+  }
+
+  void DoDnsProbe(chrome_common_net::DnsProbeStatus final_status) {
+    core().OnNetErrorInfo(chrome_common_net::DNS_PROBE_STARTED);
+    core().OnNetErrorInfo(final_status);
+  }
+
  private:
   void LinkDoctorLoadFinished(const std::string& result) {
     url_being_fetched_ = GURL();
@@ -133,6 +206,12 @@ class NetErrorHelperCoreTest : public testing::Test,
     url_being_fetched_ = GURL();
   }
 
+  virtual void ReloadPage() OVERRIDE {
+    reload_count_++;
+  }
+
+  MockOneShotTimer* timer_;
+
   NetErrorHelperCore core_;
 
   GURL url_being_fetched_;
@@ -147,6 +226,8 @@ class NetErrorHelperCoreTest : public testing::Test,
   std::string last_error_html_;
   // Number of times |last_error_html_| has been changed.
   int error_html_update_count_;
+
+  int reload_count_;
 };
 
 //------------------------------------------------------------------------------
@@ -1302,3 +1383,137 @@ TEST_F(NetErrorHelperCoreTest, LinkDoctorStopped) {
             last_error_html());
   EXPECT_EQ(1, error_html_update_count());
 }
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadDisabled) {
+  core().set_auto_reload_enabled(false);

[mmenke, 2014/02/25 22:09:52]

Should this be in the test fixture?  Not exactly a huge concern, just seems like
one less thing to worry about.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+
+  EXPECT_FALSE(timer()->IsRunning());
+  EXPECT_EQ(0, reload_count());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadSucceeds) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+
+  EXPECT_TRUE(timer()->IsRunning());
+  EXPECT_EQ(0, reload_count());
+
+  timer()->Fire();
+  EXPECT_FALSE(timer()->IsRunning());
+  EXPECT_EQ(1, reload_count());
+
+  DoSuccessLoad();
+
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadRetries) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+
+  EXPECT_TRUE(timer()->IsRunning());
+  base::TimeDelta first_delay = timer()->delay();
+  EXPECT_EQ(0, reload_count());
+
+  timer()->Fire();
+  EXPECT_FALSE(timer()->IsRunning());
+  EXPECT_EQ(1, reload_count());
+
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+
+  EXPECT_TRUE(timer()->IsRunning());
+  EXPECT_GT(timer()->delay(), first_delay);
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsTimerOnStop) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(timer()->IsRunning());
+  core().OnStop();
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsLoadingOnStop) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_EQ(1, core().auto_reload_count());
+  timer()->Fire();
+  EXPECT_EQ(1, reload_count());
+  core().OnStop();
+  EXPECT_FALSE(timer()->IsRunning());
+  EXPECT_EQ(0, core().auto_reload_count());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOtherLoadStart) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(timer()->IsRunning());
+  core().OnStartLoad(NetErrorHelperCore::MAIN_FRAME,
+                     NetErrorHelperCore::NON_ERROR_PAGE);
+  EXPECT_FALSE(timer()->IsRunning());
+  EXPECT_EQ(1, core().auto_reload_count());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadResetsCountOnSuccess) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  base::TimeDelta delay = timer()->delay();
+  timer()->Fire();
+  EXPECT_EQ(1, reload_count());
+  DoSuccessLoad();
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_EQ(timer()->delay(), delay);
+  timer()->Fire();
+  EXPECT_EQ(2, reload_count());
+  DoSuccessLoad();
+  EXPECT_EQ(0, core().auto_reload_count());

[mmenke, 2014/02/25 22:09:52]

Think this test could use a couple more auto_reload_count checks.  A couple of
the above tests use it as a proxy for checking delay without starting another
request, and it seems like one should explicitly test both together.  This seems
the best test for that.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadRestartsOnOnline) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  base::TimeDelta delay = timer()->delay();
+  timer()->Fire();
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(timer()->IsRunning());
+  EXPECT_NE(delay, timer()->delay());
+  core().NetworkStateChanged(true);
+  EXPECT_TRUE(timer()->IsRunning());
+  EXPECT_EQ(delay, timer()->delay());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadDoesNotStartOnOnline) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  timer()->Fire();
+  DoSuccessLoad();
+  EXPECT_FALSE(timer()->IsRunning());
+  core().NetworkStateChanged(true);
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadStopsOnOffline) {

[mmenke, 2014/02/25 22:09:52]

I know it's a lot of tests for this, but I'd like a test of the full
online->offline->online transition.  Could just add another online transition at
the end of this one, or make another test just like it.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.

+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  EXPECT_TRUE(timer()->IsRunning());
+  core().NetworkStateChanged(false);
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadDoesNotRestartOnOnlineAfterStop) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  timer()->Fire();
+  core().OnStop();
+  core().NetworkStateChanged(true);
+  EXPECT_FALSE(timer()->IsRunning());
+}
+
+TEST_F(NetErrorHelperCoreTest, AutoReloadWithDnsProbes) {
+  core().set_auto_reload_enabled(true);
+  DoErrorLoad(net::ERR_CONNECTION_RESET);
+  DoDnsProbe(chrome_common_net::DNS_PROBE_FINISHED_NXDOMAIN);
+  timer()->Fire();
+  EXPECT_EQ(1, reload_count());
+}

[Randy Smith (Not in Mondays), 2014/02/26 00:07:38]

Suggestion: A test that confirms that we eventually level off in our exponential
backoff timing.

[Elly Fong-Jones, 2014/03/03 19:31:07]

Done.