Switch input event ack timeout in RenderWidgetHostImpl to OneShotTimer

Bug: 1059865
Change-Id: Ic5c31a97233624fdf5848b6cf571fa0e7d120aa0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2097183
Commit-Queue: Evan Stade <estade@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#751118}

content/browser/devtools/devtools_manager_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "content/browser/devtools/shared_worker_devtools_manager.h"
+#include "content/common/content_constants_internal.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
@@ -140,33 +141,18 @@ TEST_F(DevToolsManagerTest, NoUnresponsiveDialogInInspectedContents) {
       WebContents::FromRenderViewHost(inspected_rvh)));
   client_host.InspectAgentHost(agent_host.get());
 
-  // Start with a short timeout.
-  inspected_rvh->GetWidget()->StartInputEventAckTimeout(
-      TimeDelta::FromMilliseconds(10));
-  {
-    base::RunLoop run_loop;
-    // Wait long enough for first timeout and see if it fired. We use quit-when-
-    // idle so that all tasks due after the timeout get a chance to run.
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, run_loop.QuitWhenIdleClosure(),
-        TimeDelta::FromMilliseconds(10));
-    run_loop.Run();
-  }
+  // Start a timeout.
+  inspected_rvh->GetWidget()->StartInputEventAckTimeout();
+  task_environment()->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_FALSE(delegate.renderer_unresponsive_received());
 
   // Now close devtools and check that the notification is delivered.
   client_host.Close();
-  // Start with a short timeout.
-  inspected_rvh->GetWidget()->StartInputEventAckTimeout(
-      TimeDelta::FromMilliseconds(10));
-  {
-    base::RunLoop run_loop;
-    // Wait long enough for first timeout and see if it fired.
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, run_loop.QuitWhenIdleClosure(),
-        TimeDelta::FromMilliseconds(10));
-    run_loop.Run();
-  }
+  // Start a timeout.
+  inspected_rvh->GetWidget()->StartInputEventAckTimeout();
+  task_environment()->FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_TRUE(delegate.renderer_unresponsive_received());
 
   contents()->SetDelegate(nullptr);

content/browser/renderer_host/render_widget_host_impl.cc

@@ -144,6 +144,11 @@ namespace {
 
 bool g_check_for_pending_visual_properties_ack = true;
 
+bool ShouldDisableHangMonitor() {
+  return base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kDisableHangMonitor);
+}
+
 // <process id, routing id>
 using RenderWidgetHostID = std::pair<int32_t, int32_t>;
 using RoutingIDWidgetMap =
@@ -364,12 +369,6 @@ RenderWidgetHostImpl::RenderWidgetHostImpl(
   SetWidget(std::move(widget));
 
   const auto* command_line = base::CommandLine::ForCurrentProcess();
-  if (!command_line->HasSwitch(switches::kDisableHangMonitor)) {
-    input_event_ack_timeout_ = std::make_unique<TimeoutMonitor>(
-        base::BindRepeating(&RenderWidgetHostImpl::OnInputEventAckTimeout,
-                            weak_factory_.GetWeakPtr()));
-  }
-
   if (!command_line->HasSwitch(switches::kDisableNewContentRenderingTimeout)) {
     new_content_rendering_timeout_ = std::make_unique<TimeoutMonitor>(
         base::BindRepeating(&RenderWidgetHostImpl::ClearDisplayedGraphics,
@@ -1144,17 +1143,27 @@ void RenderWidgetHostImpl::RenderProcessBlockedStateChanged(bool blocked) {
     RestartInputEventAckTimeoutIfNecessary();
 }
 
-void RenderWidgetHostImpl::StartInputEventAckTimeout(TimeDelta delay) {
-  if (!input_event_ack_timeout_)
+void RenderWidgetHostImpl::StartInputEventAckTimeout() {
+  if (ShouldDisableHangMonitor())
     return;
-  input_event_ack_timeout_->Start(delay);
-  input_event_ack_start_time_ = clock_->NowTicks();
+
+  if (!input_event_ack_timeout_.IsRunning()) {
+    input_event_ack_timeout_.Start(
+        FROM_HERE, hung_renderer_delay_,
+        base::BindOnce(&RenderWidgetHostImpl::OnInputEventAckTimeout,
+                       weak_factory_.GetWeakPtr()));
+    input_event_ack_start_time_ = clock_->NowTicks();
+  }
 }
 
 void RenderWidgetHostImpl::RestartInputEventAckTimeoutIfNecessary() {
-  if (!GetProcess()->IsBlocked() && input_event_ack_timeout_ &&
-      in_flight_event_count_ > 0 && !is_hidden_)
-    input_event_ack_timeout_->Restart(hung_renderer_delay_);
+  if (!GetProcess()->IsBlocked() && !ShouldDisableHangMonitor() &&
+      in_flight_event_count_ > 0 && !is_hidden_) {
+    input_event_ack_timeout_.Start(
+        FROM_HERE, hung_renderer_delay_,
+        base::BindOnce(&RenderWidgetHostImpl::OnInputEventAckTimeout,
+                       weak_factory_.GetWeakPtr()));
+  }
 }
 
 bool RenderWidgetHostImpl::IsCurrentlyUnresponsive() {
@@ -1162,8 +1171,7 @@ bool RenderWidgetHostImpl::IsCurrentlyUnresponsive() {
 }
 
 void RenderWidgetHostImpl::StopInputEventAckTimeout() {
-  if (input_event_ack_timeout_)
-    input_event_ack_timeout_->Stop();
+  input_event_ack_timeout_.Stop();
 
   if (!input_event_ack_start_time_.is_null()) {
     base::TimeDelta elapsed = clock_->NowTicks() - input_event_ack_start_time_;
@@ -2693,7 +2701,7 @@ InputEventAckState RenderWidgetHostImpl::FilterInputEvent(
 void RenderWidgetHostImpl::IncrementInFlightEventCount() {
   ++in_flight_event_count_;
   if (!is_hidden_)
-    StartInputEventAckTimeout(hung_renderer_delay_);
+    StartInputEventAckTimeout();
 }
 
 void RenderWidgetHostImpl::DecrementInFlightEventCount(

content/browser/renderer_host/render_widget_host_impl.h

@@ -29,6 +29,7 @@
 #include "base/time/tick_clock.h"
 #include "base/time/time.h"
 #include "base/timer/elapsed_timer.h"
+#include "base/timer/timer.h"
 #include "build/build_config.h"
 #include "components/viz/common/surfaces/frame_sink_id.h"
 #include "content/browser/renderer_host/event_with_latency_info.h"
@@ -178,12 +179,6 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   // RenderWidgetHostImpl.
   static RenderWidgetHostImpl* From(RenderWidgetHost* rwh);
 
-  void set_hung_renderer_delay(const base::TimeDelta& delay) {
-    hung_renderer_delay_ = delay;
-  }
-
-  base::TimeDelta hung_renderer_delay() { return hung_renderer_delay_; }
-
   void set_new_content_rendering_delay_for_testing(
       const base::TimeDelta& delay) {
     new_content_rendering_delay_ = delay;
@@ -958,7 +953,7 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   // Starts a hang monitor timeout. If there's already a hang monitor timeout
   // the new one will only fire if it has a shorter delay than the time
   // left on the existing timeouts.
-  void StartInputEventAckTimeout(base::TimeDelta delay);
+  void StartInputEventAckTimeout();
 
   // Stops all existing hang monitor timeouts and assumes the renderer is
   // responsive.
@@ -1181,7 +1176,7 @@ class CONTENT_EXPORT RenderWidgetHostImpl
   // Receives and handles all input events.
   std::unique_ptr<InputRouter> input_router_;
 
-  std::unique_ptr<TimeoutMonitor> input_event_ack_timeout_;
+  base::OneShotTimer input_event_ack_timeout_;
   base::TimeTicks input_event_ack_start_time_;
 
   std::unique_ptr<base::CallbackList<void(bool)>::Subscription>

content/browser/renderer_host/render_widget_host_unittest.cc

@@ -33,6 +33,7 @@
 #include "content/browser/renderer_host/render_view_host_delegate_view.h"
 #include "content/browser/renderer_host/render_widget_host_delegate.h"
 #include "content/browser/renderer_host/render_widget_host_view_base.h"
+#include "content/common/content_constants_internal.h"
 #include "content/common/edit_command.h"
 #include "content/common/input/synthetic_web_input_event_builders.h"
 #include "content/common/input_messages.h"
@@ -1322,53 +1323,43 @@ TEST_F(RenderWidgetHostTest, UnhandledGestureEvent) {
 // 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, DontPostponeInputEventAckTimeout) {
-  // Start with a short timeout.
-  host_->StartInputEventAckTimeout(TimeDelta::FromMilliseconds(10));
+  base::TimeDelta delay =
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs);
 
-  // Immediately try to add a long 30 second timeout.
+  // Start a timeout.
+  host_->StartInputEventAckTimeout();
+
+  task_environment_.FastForwardBy(delay / 2);
+
+  // Add another timeout.
   EXPECT_FALSE(delegate_->unresponsive_timer_fired());
-  host_->StartInputEventAckTimeout(TimeDelta::FromSeconds(30));
+  host_->StartInputEventAckTimeout();
 
   // Wait long enough for first timeout and see if it fired.
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(10));
+  task_environment_.FastForwardBy(delay);
   EXPECT_TRUE(delegate_->unresponsive_timer_fired());
 }
 
 // Test that the hang monitor timer expires properly if it is started, stopped,
 // and then started again.
 TEST_F(RenderWidgetHostTest, StopAndStartInputEventAckTimeout) {
-  // Start with a short timeout, then stop it.
-  host_->StartInputEventAckTimeout(TimeDelta::FromMilliseconds(10));
+  // Start a timeout, then stop it.
+  host_->StartInputEventAckTimeout();
   host_->StopInputEventAckTimeout();
 
   // Start it again to ensure it still works.
   EXPECT_FALSE(delegate_->unresponsive_timer_fired());
-  host_->StartInputEventAckTimeout(TimeDelta::FromMilliseconds(10));
+  host_->StartInputEventAckTimeout();
 
   // Wait long enough for first timeout and see if it fired.
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(40));
-  EXPECT_TRUE(delegate_->unresponsive_timer_fired());
-}
-
-// Test that the hang monitor timer expires properly if it is started, then
-// updated to a shorter duration.
-TEST_F(RenderWidgetHostTest, ShorterDelayInputEventAckTimeout) {
-  // Start with a timeout.
-  host_->StartInputEventAckTimeout(TimeDelta::FromMilliseconds(100));
-
-  // Start it again with shorter delay.
-  EXPECT_FALSE(delegate_->unresponsive_timer_fired());
-  host_->StartInputEventAckTimeout(TimeDelta::FromMilliseconds(20));
-
-  // Wait long enough for the second timeout and see if it fired.
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(25));
+  task_environment_.FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_TRUE(delegate_->unresponsive_timer_fired());
 }
 
 // Test that the hang monitor timer is effectively disabled when the widget is
 // hidden.
 TEST_F(RenderWidgetHostTest, InputEventAckTimeoutDisabledForInputWhenHidden) {
-  host_->set_hung_renderer_delay(base::TimeDelta::FromMicroseconds(1));
   SimulateMouseEvent(WebInputEvent::kMouseMove, 10, 10, 0, false);
 
   // Hiding the widget should deactivate the timeout.
@@ -1376,18 +1367,21 @@ TEST_F(RenderWidgetHostTest, InputEventAckTimeoutDisabledForInputWhenHidden) {
 
   // The timeout should not fire.
   EXPECT_FALSE(delegate_->unresponsive_timer_fired());
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(2));
+  task_environment_.FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_FALSE(delegate_->unresponsive_timer_fired());
 
   // The timeout should never reactivate while hidden.
   SimulateMouseEvent(WebInputEvent::kMouseMove, 10, 10, 0, false);
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(2));
+  task_environment_.FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_FALSE(delegate_->unresponsive_timer_fired());
 
   // Showing the widget should restore the timeout, as the events have
   // not yet been ack'ed.
   host_->WasShown(base::nullopt /* record_tab_switch_time_request */);
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(2));
+  task_environment_.FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_TRUE(delegate_->unresponsive_timer_fired());
 }
 
@@ -1395,12 +1389,10 @@ TEST_F(RenderWidgetHostTest, InputEventAckTimeoutDisabledForInputWhenHidden) {
 // This can happen if the second input event causes the renderer to hang.
 // This test will catch a regression of crbug.com/111185.
 TEST_F(RenderWidgetHostTest, MultipleInputEvents) {
-  // Configure the host to wait 10ms before considering
-  // the renderer hung.
-  host_->set_hung_renderer_delay(base::TimeDelta::FromMicroseconds(10));
-
   // Send two events but only one ack.
   SimulateKeyboardEvent(WebInputEvent::kRawKeyDown);
+  task_environment_.FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs / 2));
   SimulateKeyboardEvent(WebInputEvent::kRawKeyDown);
 
   MockWidgetInputHandler::MessageVector dispatched_events =
@@ -1411,10 +1403,12 @@ TEST_F(RenderWidgetHostTest, MultipleInputEvents) {
   // Send the simulated response from the renderer back.
   dispatched_events[0]->ToEvent()->CallCallback(INPUT_EVENT_ACK_STATE_CONSUMED);
 
-  // Wait long enough for first timeout and see if it fired.
-  task_environment_.FastForwardBy(TimeDelta::FromMilliseconds(20));
+  // Wait long enough for second timeout and see if it fired.
+  task_environment_.FastForwardBy(
+      base::TimeDelta::FromMilliseconds(kHungRendererDelayMs + 10));
   EXPECT_TRUE(delegate_->unresponsive_timer_fired());
 }
+
 TEST_F(RenderWidgetHostTest, IgnoreInputEvent) {
   host_->SetupForInputRouterTest();
 

content/test/test_render_view_host.cc

@@ -28,6 +28,7 @@
 #include "content/public/common/page_state.h"
 #include "content/public/common/web_preferences.h"
 #include "content/test/test_render_frame_host.h"
+#include "content/test/test_render_view_host.h"
 #include "content/test/test_web_contents.h"
 #include "media/base/video_frame.h"
 #include "mojo/public/cpp/bindings/pending_remote.h"
@@ -320,7 +321,9 @@ void TestRenderViewHost::TestOnUpdateStateWithFile(
   static_cast<RenderFrameHostImpl*>(GetMainFrame())->OnUpdateState(state);
 }
 
-RenderViewHostImplTestHarness::RenderViewHostImplTestHarness() {
+RenderViewHostImplTestHarness::RenderViewHostImplTestHarness()
+    : RenderViewHostTestHarness(
+          base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
   std::vector<ui::ScaleFactor> scale_factors;
   scale_factors.push_back(ui::SCALE_FACTOR_100P);
   scoped_set_supported_scale_factors_.reset(