Move page closing hang timer duties to its own timer.

Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2725993002
Cr-Commit-Position: refs/heads/master@{#454490}
Committed: https://chromium.googlesource.com/chromium/src/+/c031d3984b2fe7a91e05d666410c20f151c8b86d

[Avi (use Gerrit), 2017-03-01 22:42:36]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[Avi (use Gerrit), 2017-03-01 22:42:37]

Description was changed from

==========
Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
==========

to

==========
Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2017-03-01 22:43:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-01 23:11:25]

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

[commit-bot: I haz the power, 2017-03-01 23:11:25]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Avi (use Gerrit), 2017-03-01 23:38:35]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-01 23:39:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-03-01 23:41:23]

Description was changed from

==========
Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move page closing hang timer duties to its own timer.

BUG=418266, 684202
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-02 00:25:54]

avi@chromium.org changed reviewers:
+ creis@chromium.org, pfeldman@chromium.org

[Avi (use Gerrit), 2017-03-02 00:25:55]

Charlie: PTAL.
Pavel: What are your thoughts re debuggability? I won't be submitting this until
we get the devtools issues sorted out, as in your bug, but this is for you to
look at re implementation.

[commit-bot: I haz the power, 2017-03-02 00:33:51]

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

[commit-bot: I haz the power, 2017-03-02 00:33:52]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-03-02 01:24:39]

Description was changed from

==========
Move page closing hang timer duties to its own timer.

BUG=418266, 684202
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Avi (use Gerrit), 2017-03-02 01:27:40]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 01:28:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-03-02 01:35:48]

This has issues; please hold off.

[Avi (use Gerrit), 2017-03-02 02:10:31]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 02:11:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-03-02 02:22:06]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 02:23:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-03-02 02:27:41]

Charlie, Pavel: please review.

Charlie:

A note on the removal of ShouldCloseTabOnUnresponsiveRenderer.

In WebContentsImpl::RendererUnresponsive, I'm removing the
IsWaitingForUnloadACK() block. IsWaitingForUnloadACK() checks for one of two
things: either the user tried to close the page and the page hung in the unload
that way, or the frame was swapped out and the page hung in the unload in that
way.

But RenderFrameImpl *already* has a swapping-out timeout timer for that. So that
block is just about closing. Now that we're putting the closing timeout on its
own timer, this entire block is dead, and it was the only caller of
ShouldCloseTabOnUnresponsiveRenderer.

In fact, ShouldCloseTabOnUnresponsiveRenderer was self-contradictory!

WebContents only called it if IsWaitingForUnloadACK() was true. Yet inside of
ShouldCloseTabOnUnresponsiveRenderer we see:

-----
bool ShouldCloseTabOnUnresponsiveRenderer() {
  if (something)
    return true;

  CHECK(!IsWaitingForUnloadACK())
-----

So ShouldCloseTabOnUnresponsiveRenderer is only called if
IsWaitingForUnloadACK() was true, yet the only way out without hitting a CHECK
if IsWaitingForUnloadACK() was false is to return true. This is a broken
function, a remnant of old times, and deserves to be excised.

[commit-bot: I haz the power, 2017-03-02 04:17:31]

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

[commit-bot: I haz the power, 2017-03-02 04:17:32]

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-03-02 05:36:33]

https://codereview.chromium.org/2725993002/diff/80001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2725993002/diff/80001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:335: // is ok to do
nothing here.
I may also need to cut down this comment.

[Avi (use Gerrit), 2017-03-02 15:33:54]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-02 15:34:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 16:49:05]

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

[commit-bot: I haz the power, 2017-03-02 16:49:06]

Dry run: This issue passed the CQ dry run.

[Charlie Reis, 2017-03-03 01:06:00]

Yes, I'm not surprised-- that code is pretty old and didn't get properly
revisited (in depth) when we moved unload to run in the background.  I love that
it's gone now.

Might be worth verifying that we have some test coverage for this by
intentionally breaking the timer and seeing if try jobs fail?  Hopefully we can
still close a tab if the renderer is frozen.

If so, LGTM!

https://codereview.chromium.org/2725993002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (left):

https://codereview.chromium.org/2725993002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:286: bool
ShouldCloseTabOnUnresponsiveRenderer();
Hooray!  This is exactly what I hoped to get out of this change.

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:589:
close_timeout_->Start(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
Sure, moving the timer start within both of these checks makes sense.

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.h:321:
std::unique_ptr<TimeoutMonitor> close_timeout_;
nit: Let's add a comment about when this is used (i.e., specifically from the
period of time between Close() and receiving a Close_ACK, while the unload
handler runs?).

[Avi (use Gerrit), 2017-03-03 01:25:17]

The CQ bit was checked by avi@chromium.org to run a CQ dry run

[Avi (use Gerrit), 2017-03-03 01:25:25]

I uploaded https://codereview.chromium.org/2724203004 as this change but with
the timer intentionally broken. Do we have tests to catch it? Let's find out!

https://codereview.chromium.org/2725993002/diff/100001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.h (left):

https://codereview.chromium.org/2725993002/diff/100001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.h:286: bool
ShouldCloseTabOnUnresponsiveRenderer();
On 2017/03/03 01:05:59, Charlie Reis (OOO til Mar 6) wrote:
> Hooray!  This is exactly what I hoped to get out of this change.

Acknowledged.

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.cc:589:
close_timeout_->Start(TimeDelta::FromMilliseconds(kUnloadTimeoutMS));
On 2017/03/03 01:06:00, Charlie Reis (OOO til Mar 6) wrote:
> Sure, moving the timer start within both of these checks makes sense.

Acknowledged.

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_view_host_impl.h (right):

https://codereview.chromium.org/2725993002/diff/100001/content/browser/render...
content/browser/renderer_host/render_view_host_impl.h:321:
std::unique_ptr<TimeoutMonitor> close_timeout_;
On 2017/03/03 01:06:00, Charlie Reis (OOO til Mar 6) wrote:
> nit: Let's add a comment about when this is used (i.e., specifically from the
> period of time between Close() and receiving a Close_ACK, while the unload
> handler runs?).

Done.

[commit-bot: I haz the power, 2017-03-03 01:25:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Avi (use Gerrit), 2017-03-03 02:18:04]

FYI there are browser tests in
chrome/browser/lifetime/browser_close_manager_browsertest.cc and
chrome/browser/unload_browsertest.cc that test to make sure that hung pages can
still be closed. And we fail them if we neuter the timeout.

[Avi (use Gerrit), 2017-03-03 02:36:34]

The CQ bit was unchecked by avi@chromium.org

[Avi (use Gerrit), 2017-03-03 02:36:39]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-03-03 02:36:40]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2725993002/#ps120001
(title: "creis")

[commit-bot: I haz the power, 2017-03-03 02:37:18]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-03 03:09:45]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1488508599519130,
"parent_rev": "cbfa0cc27ff472e0911c74160eb05d87d4c14a2b", "commit_rev":
"c031d3984b2fe7a91e05d666410c20f151c8b86d"}

[commit-bot: I haz the power, 2017-03-03 03:11:05]

Description was changed from

==========
Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Move page closing hang timer duties to its own timer.

BUG=418266
TEST=no user-visible change
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2725993002
Cr-Commit-Position: refs/heads/master@{#454490}
Committed:
https://chromium.googlesource.com/chromium/src/+/c031d3984b2fe7a91e05d666410c...
==========

[commit-bot: I haz the power, 2017-03-03 03:11:07]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/chromium/src/+/c031d3984b2fe7a91e05d666410c...