Prevent hang monitor restarts when hidden

Prevent hang monitor restarts when hidden

When the render widget is hidden, the hang monitor timeout is explicitly
stopped. However, tasks executed *after* the widget hide signal may
trigger a restart of the hang monitor timeout, e.g., queued input
events. Prevent such restarts when the widget is hidden while
also ensuring the hang monitor restarts when the widget is shown again.

BUG=458594
Committed: https://crrev.com/d20d7b3d4af9f483793c3c4eaa96d5e195e3609a
Cr-Commit-Position: refs/heads/master@{#319778}

[jdduke (slow), 2015-02-19 19:30:34]

jdduke@chromium.org changed reviewers:
+ piman@chromium.org

[jdduke (slow), 2015-02-19 19:30:34]

piman@: PTAL, thanks. Just noticed this could (conceivably) occur when following
the discussion on crbug.com/458594.

[piman, 2015-02-19 23:43:52]

lgtm

https://codereview.chromium.org/938123003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/938123003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:868: return;
Do we need to restart it when we go visible (i.e. keep around the state that we
want it running), or does it not matter?

[jdduke (slow), 2015-02-20 16:40:22]

New patchsets have been uploaded after l-g-t-m from piman@chromium.org

[jdduke (slow), 2015-02-23 16:03:27]

https://codereview.chromium.org/938123003/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/938123003/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_impl.cc:868: return;
On 2015/02/19 23:43:52, piman (Very slow to review) wrote:
> Do we need to restart it when we go visible (i.e. keep around the state that
we
> want it running), or does it not matter?

Good question, and I don't really have the answer. I think ideally the answer is
yes, but this change is simply making our decision to turn the timeout off
consistent when the widget is hidden.

I'm a little hesitant to make that behavior change now, given how fragile this
has been, but I think we should look into it. I'll file a bug and note it here.

[jdduke (slow), 2015-02-23 16:03:40]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-02-23 16:04:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938123003/20001

[commit-bot: I haz the power, 2015-02-23 16:07:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 16:07:25]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[jdduke (slow), 2015-02-23 16:13:49]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-02-23 16:14:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938123003/20001

[commit-bot: I haz the power, 2015-02-23 20:52:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 20:52:16]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2015-02-23 20:52:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-02-23 20:52:53]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdduke (slow), 2015-03-06 21:16:40]

I've updated the patch to suppress timeout restarts only for input events (when
the widget is hidden), while also restarting the timeout when we reshow the
widget and there are outstanding input event acks.

The way we allow subclasses to at while stop/restart the timeout
(RenderViewHostImpl/RenderFrameHostImpl) is a little disconcerting (why not have
a separate timeout instead of hijacking the input timeout?!), but I'm hesitant
to refactor that code now...

https://codereview.chromium.org/938123003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/938123003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:544:
RestartHangMonitorTimeout();
Hmm, I wonder if this could actually increase the hang rate if we already have
issues with slow foregrounding of deprioritized renderers.

[jdduke (slow), 2015-03-06 21:21:07]

https://codereview.chromium.org/938123003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_impl.cc (right):

https://codereview.chromium.org/938123003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_impl.cc:544:
RestartHangMonitorTimeout();
On 2015/03/06 21:16:39, jdduke wrote:
> Hmm, I wonder if this could actually increase the hang rate if we already have
> issues with slow foregrounding of deprioritized renderers.

Actually, this should be strictly better than what we have now, which would
restart the monitor while still hidden.

[jdduke (slow), 2015-03-09 21:56:46]

piman@: Sorry for the noise, but would you mind taking another look? This latest
change should handle restarting the timeout, per your original suggestion.

[piman, 2015-03-09 22:25:13]

lgtm

[jdduke (slow), 2015-03-09 22:26:10]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-03-09 22:27:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/938123003/60001

[commit-bot: I haz the power, 2015-03-09 23:55:20]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-09 23:56:13]

Patchset 4 (id:??) landed as
https://crrev.com/d20d7b3d4af9f483793c3c4eaa96d5e195e3609a
Cr-Commit-Position: refs/heads/master@{#319778}