Fixes a timeout issue in RenderWidgetHost.

Before, each call to StartHangMonitorTimeout would reset the timer,
possibly postponing it indefinitely.

BUG=11007
BUG=16535
TEST=RenderWidgetHostTest.DontPostponeHangMonitorTimeout
TEST=RenderWidgetHostTest.StopAndStartHangMonitorTimeout
TEST=UnloadTest.CrossSiteInfiniteUnloadAsyncInputEvent

Review URL: http://codereview.chromium.org/1034001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@41844 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 97 insertions, 12 deletions

diff --git a/chrome/browser/renderer_host/render_widget_host.cc b/chrome/browser/renderer_host/render_widget_host.cc
index e1e69ea..1cdc0a8 100644
--- a/chrome/browser/renderer_host/render_widget_host.cc
+++ b/chrome/browser/renderer_host/render_widget_host.cc
@@ -339,22 +339,27 @@ void RenderWidgetHost::DonePaintingToBackingStore() {
 }
 
 void RenderWidgetHost::StartHangMonitorTimeout(TimeDelta delay) {
-  time_when_considered_hung_ = Time::Now() + delay;
-
   // If we already have a timer that will expire at or before the given delay,
-  // then we have nothing more to do now.
+  // then we have nothing more to do now.  If we have set our end time to null
+  // by calling StopHangMonitorTimeout, though, we will need to restart the
+  // timer.
   if (hung_renderer_timer_.IsRunning() &&
-      hung_renderer_timer_.GetCurrentDelay() <= delay)
+      hung_renderer_timer_.GetCurrentDelay() <= delay &&
+      !time_when_considered_hung_.is_null()) {
     return;
+  }
 
   // Either the timer is not yet running, or we need to adjust the timer to
   // fire sooner.
+  time_when_considered_hung_ = Time::Now() + delay;
   hung_renderer_timer_.Stop();
   hung_renderer_timer_.Start(delay, this,
       &RenderWidgetHost::CheckRendererIsUnresponsive);
 }
 
 void RenderWidgetHost::RestartHangMonitorTimeout() {
+  // Setting to null will cause StartHangMonitorTimeout to restart the timer.
+  time_when_considered_hung_ = Time();
   StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kHungRendererDelayMs));
 }
 
diff --git a/chrome/browser/renderer_host/render_widget_host_unittest.cc b/chrome/browser/renderer_host/render_widget_host_unittest.cc
index e103849..6d51f65 100644
--- a/chrome/browser/renderer_host/render_widget_host_unittest.cc
+++ b/chrome/browser/renderer_host/render_widget_host_unittest.cc
@@ -7,12 +7,15 @@
 #include "base/keyboard_codes.h"
 #include "base/scoped_ptr.h"
 #include "base/shared_memory.h"
+#include "base/timer.h"
 #include "build/build_config.h"
 #include "chrome/browser/renderer_host/backing_store.h"
 #include "chrome/browser/renderer_host/test/test_render_view_host.h"
 #include "chrome/common/render_messages.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
+using base::TimeDelta;
+
 using WebKit::WebInputEvent;
 using WebKit::WebMouseWheelEvent;
 
@@ -55,7 +58,7 @@ class RenderWidgetHostProcess : public MockRenderProcessHost {
 
   TransportDIB* current_update_buf_;
 
-  // Set to true when WaitForUpdateMsg should return a successful update messaage
+  // Set to true when WaitForUpdateMsg should return a successful update message
   // reply. False implies timeout.
   bool update_msg_should_reply_;
 
@@ -130,7 +133,8 @@ class MockRenderWidgetHost : public RenderWidgetHost {
         prehandle_keyboard_event_called_(false),
         prehandle_keyboard_event_type_(WebInputEvent::Undefined),
         unhandled_keyboard_event_called_(false),
-        unhandled_keyboard_event_type_(WebInputEvent::Undefined) {
+        unhandled_keyboard_event_type_(WebInputEvent::Undefined),
+        unresponsive_timer_fired_(false) {
   }
 
   // Tests that make sure we ignore keyboard event acknowledgments to events we
@@ -155,6 +159,10 @@ class MockRenderWidgetHost : public RenderWidgetHost {
     prehandle_keyboard_event_ = handle;
   }
 
+  bool unresponsive_timer_fired() const {
+    return unresponsive_timer_fired_;
+  }
+
  protected:
   virtual bool PreHandleKeyboardEvent(const NativeWebKeyboardEvent& event,
                                       bool* is_keyboard_shortcut) {
@@ -168,6 +176,10 @@ class MockRenderWidgetHost : public RenderWidgetHost {
     unhandled_keyboard_event_called_ = true;
   }
 
+  virtual void NotifyRendererUnresponsive() {
+    unresponsive_timer_fired_ = true;
+  }
+
  private:
   bool prehandle_keyboard_event_;
   bool prehandle_keyboard_event_called_;
@@ -175,6 +187,8 @@ class MockRenderWidgetHost : public RenderWidgetHost {
 
   bool unhandled_keyboard_event_called_;
   WebInputEvent::Type unhandled_keyboard_event_type_;
+
+  bool unresponsive_timer_fired_;
 };
 
 // RenderWidgetHostTest --------------------------------------------------------
@@ -600,3 +614,39 @@ TEST_F(RenderWidgetHostTest, CoalescesWheelEvents) {
   SendInputEventACK(WebInputEvent::MouseWheel, true);
   EXPECT_EQ(0U, process_->sink().message_count());
 }
+
+// Test that the hang monitor timer expires properly if a new timer is started
+// while one is in progress (see crbug.com/11007).
+TEST_F(RenderWidgetHostTest, DontPostponeHangMonitorTimeout) {
+  // Start with a short timeout.
+  host_->StartHangMonitorTimeout(TimeDelta::FromMilliseconds(10));
+
+  // Immediately try to add a long 30 second timeout.
+  EXPECT_FALSE(host_->unresponsive_timer_fired());
+  host_->StartHangMonitorTimeout(TimeDelta::FromMilliseconds(30000));
+
+  // Wait long enough for first timeout and see if it fired.
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          new MessageLoop::QuitTask(), 10);
+  MessageLoop::current()->Run();
+  EXPECT_TRUE(host_->unresponsive_timer_fired());
+}
+
+// Test that the hang monitor timer expires properly if it is started, stopped,
+// and then started again.
+TEST_F(RenderWidgetHostTest, StopAndStartHangMonitorTimeout) {
+  // Start with a short timeout, then stop it.
+  host_->StartHangMonitorTimeout(TimeDelta::FromMilliseconds(10));
+  host_->StopHangMonitorTimeout();
+
+  // Start it again to ensure it still works.
+  EXPECT_FALSE(host_->unresponsive_timer_fired());
+  host_->StartHangMonitorTimeout(TimeDelta::FromMilliseconds(10));
+
+  // Wait long enough for first timeout and see if it fired.
+  MessageLoop::current()->PostDelayedTask(FROM_HERE,
+                                          new MessageLoop::QuitTask(), 10);
+  MessageLoop::current()->Run();
+  EXPECT_TRUE(host_->unresponsive_timer_fired());
+}
+
diff --git a/chrome/browser/unload_uitest.cc b/chrome/browser/unload_uitest.cc
index b4b311c..59e9a3d 100644
--- a/chrome/browser/unload_uitest.cc
+++ b/chrome/browser/unload_uitest.cc
@@ -6,11 +6,14 @@
 #include "base/file_util.h"
 #include "base/platform_thread.h"
 #include "chrome/browser/net/url_request_mock_http_job.h"
+#include "chrome/browser/view_ids.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/automation/browser_proxy.h"
 #include "chrome/test/automation/tab_proxy.h"
+#include "chrome/test/automation/window_proxy.h"
 #include "chrome/test/ui/ui_test.h"
 #include "net/url_request/url_request_unittest.h"
+#include "views/event.h"
 
 const std::string NOLISTENERS_HTML =
     "<html><head><title>nolisteners</title></head><body></body></html>";
@@ -143,13 +146,9 @@ class UnloadTest : public UITest {
   // load is purposely async to test the case where the user loads another
   // page without waiting for the first load to complete.
   void NavigateToNolistenersFileTwiceAsync() {
-    // TODO(ojan): We hit a DCHECK in RenderViewHost::OnMsgShouldCloseACK
-    // if we don't sleep here.
-    PlatformThread::Sleep(400);
     NavigateToURLAsync(
         URLRequestMockHTTPJob::GetMockUrl(
             FilePath(FILE_PATH_LITERAL("title2.html"))));
-    PlatformThread::Sleep(400);
     NavigateToURL(
         URLRequestMockHTTPJob::GetMockUrl(
             FilePath(FILE_PATH_LITERAL("title2.html"))));
@@ -185,7 +184,7 @@ class UnloadTest : public UITest {
 };
 
 // Navigate to a page with an infinite unload handler.
-// Then two two async crosssite requests to ensure
+// Then two async crosssite requests to ensure
 // we don't get confused and think we're closing the tab.
 TEST_F(UnloadTest, CrossSiteInfiniteUnloadAsync) {
   // Tests makes no sense in single-process mode since the renderer is hung.
@@ -199,7 +198,7 @@ TEST_F(UnloadTest, CrossSiteInfiniteUnloadAsync) {
 }
 
 // Navigate to a page with an infinite unload handler.
-// Then two two sync crosssite requests to ensure
+// Then two sync crosssite requests to ensure
 // we correctly nav to each one.
 TEST_F(UnloadTest, CrossSiteInfiniteUnloadSync) {
   // Tests makes no sense in single-process mode since the renderer is hung.
@@ -212,6 +211,37 @@ TEST_F(UnloadTest, CrossSiteInfiniteUnloadSync) {
   ASSERT_TRUE(IsBrowserRunning());
 }
 
+// Navigate to a page with an infinite unload handler.
+// Then an async crosssite request followed by an input event to ensure that
+// the short unload timeout (not the long input event timeout) is used.
+// See crbug.com/11007.
+TEST_F(UnloadTest, CrossSiteInfiniteUnloadAsyncInputEvent) {
+  // Tests makes no sense in single-process mode since the renderer is hung.
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess))
+    return;
+
+  NavigateToDataURL(INFINITE_UNLOAD_HTML, L"infiniteunload");
+
+  // Navigate to a new URL asynchronously.
+  NavigateToURLAsync(
+      URLRequestMockHTTPJob::GetMockUrl(
+          FilePath(FILE_PATH_LITERAL("title2.html"))));
+
+  // Now send an input event while we're stalled on the unload handler.
+  scoped_refptr<BrowserProxy> browser(automation()->GetBrowserWindow(0));
+  ASSERT_TRUE(browser.get());
+  scoped_refptr<WindowProxy> window(browser->GetWindow());
+  ASSERT_TRUE(window.get());
+  gfx::Rect bounds;
+  ASSERT_TRUE(window->GetViewBounds(VIEW_ID_TAB_0, &bounds, false));
+  ASSERT_TRUE(browser->SimulateDrag(bounds.CenterPoint(), bounds.CenterPoint(),
+                                    views::Event::EF_LEFT_BUTTON_DOWN, false));
+
+  // The title should update before the timeout in CheckTitle.
+  CheckTitle(L"Title Of Awesomeness");
+  ASSERT_TRUE(IsBrowserRunning());
+}
+
 // Navigate to a page with an infinite beforeunload handler.
 // Then two two async crosssite requests to ensure
 // we don't get confused and think we're closing the tab.