Fix hung renderer timer code in RenderWidgetHost (task spam bug)

content/browser/renderer_host/render_widget_host.cc

Index: content/browser/renderer_host/render_widget_host.cc
diff --git a/content/browser/renderer_host/render_widget_host.cc b/content/browser/renderer_host/render_widget_host.cc
index 404ff86093ef97319b1da921c70cda7dfbc631d1..972bea222239edf740bd44788b457a6b5dd61332 100644
--- a/content/browser/renderer_host/render_widget_host.cc
+++ b/content/browser/renderer_host/render_widget_host.cc
@@ -94,6 +94,7 @@ RenderWidgetHost::RenderWidgetHost(RenderProcessHost* process,
       touch_event_is_queued_(false),
       needs_repainting_on_restore_(false),
       is_unresponsive_(false),
+      is_monitoring_for_hung_renderer_(false),
       in_get_backing_store_(false),
       view_being_painted_(false),
       ignore_input_events_(false),
@@ -506,32 +507,22 @@ void RenderWidgetHost::StartHangMonitorTimeout(TimeDelta delay) {
     return;
   }
 
-  // If we already have a timer that will expire at or before the given delay,
-  // 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 &&

[jbates, 2011/11/10 01:51:31]

This code seems to assume that GetCurrentDelay is the time remaining until the
task fires, but it's actually just the original delay set for the timer. This
caused the Stop/Start code below to execute every few milliseconds in some cases
(possibly based on mouse inputs and other forwarded communication). This showed
up in about:tracing as null MessageLoop tasks running every few milliseconds.

[Charlie Reis, 2011/11/10 19:35:54]

Thanks for catching this!

-      !time_when_considered_hung_.is_null()) {
-    return;
-  }
+  is_monitoring_for_hung_renderer_ = true;
 
-  // 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(FROM_HERE, delay, this,
-      &RenderWidgetHost::CheckRendererIsUnresponsive);
+  // Start the timer if it's not already pending.
+  if (!hung_renderer_timer_.IsRunning()) {
+    hung_renderer_timer_.Start(FROM_HERE, delay, this,
+                               &RenderWidgetHost::CheckRendererIsUnresponsive);
+  }
 }
 
 void RenderWidgetHost::RestartHangMonitorTimeout() {
-  // Setting to null will cause StartHangMonitorTimeout to restart the timer.
-  time_when_considered_hung_ = Time();

[jbates, 2011/11/10 01:51:31]

Instead of overloading the meaning of this time, I added a bool to indicate
whether we are monitoring for hung renderer.

[Charlie Reis, 2011/11/10 19:35:54]

I like it.

+  time_of_last_renderer_response_ = TimeTicks::Now();

[jbates, 2011/11/10 01:51:31]

Storing the time_of_last_renderer_response_ rather than the
time_when_considered_hung_ simplifies the code a bit.

[Charlie Reis, 2011/11/10 19:35:54]

I like simplifying it, but I'm not sure this is correct.  If someone calls
StartHangMonitorTimeout directly, this field doesn't get set.  That means the
timeout reflects the time since the last IPC received from the renderer
(possibly a long time ago), not the time since StartHangMonitorTimeout was
called.

Because of the way CheckRendererIsUnresponsive calls StartHangMonitorTimeout, I
also think it's not as simple as updating time_of_last_renderer_response_ in
StartHangMonitorTimeout.

Is there another way to fix it?  Or should we rename Restart to Start and Start
to Resume?

   StartHangMonitorTimeout(TimeDelta::FromMilliseconds(kHungRendererDelayMs));
 }
 
 void RenderWidgetHost::StopHangMonitorTimeout() {
-  time_when_considered_hung_ = Time();
+  is_monitoring_for_hung_renderer_ = false;
   RendererIsResponsive();
 
   // We do not bother to stop the hung_renderer_timer_ here in case it will be
@@ -859,13 +850,14 @@ void RenderWidgetHost::Destroy() {
 
 void RenderWidgetHost::CheckRendererIsUnresponsive() {
   // If we received a call to StopHangMonitorTimeout.
-  if (time_when_considered_hung_.is_null())
+  if (!is_monitoring_for_hung_renderer_)
     return;
 
   // If we have not waited long enough, then wait some more.
-  Time now = Time::Now();
-  if (now < time_when_considered_hung_) {
-    StartHangMonitorTimeout(time_when_considered_hung_ - now);
+  TimeDelta elapsed_time = TimeTicks::Now() - time_of_last_renderer_response_;
+  TimeDelta hung_delay = TimeDelta::FromMilliseconds(kHungRendererDelayMs);

[Charlie Reis, 2011/11/10 19:35:54]

This isn't correct-- we use different values in different places.  (It's a much
longer delay for interaction response than for closing a tab, for example.  See
kUnloadTimeoutMS.)

+  if (elapsed_time < hung_delay) {
+    StartHangMonitorTimeout(hung_delay - elapsed_time);
     return;
   }
 
@@ -879,6 +871,7 @@ void RenderWidgetHost::CheckRendererIsUnresponsive() {
 }
 
 void RenderWidgetHost::RendererIsResponsive() {
+  time_of_last_renderer_response_ = TimeTicks::Now();
   if (is_unresponsive_) {
     is_unresponsive_ = false;
     NotifyRendererResponsive();