Move beforeunload hang timer duties to its own timer.

Move beforeunload 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/2727253005
Cr-Commit-Position: refs/heads/master@{#455140}
Committed: https://chromium.googlesource.com/chromium/src/+/ddd4c22cebfb3be9570877a7fcfebeca24c5f83b

[Avi (use Gerrit), 2017-03-03 18:54:17]

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

[Avi (use Gerrit), 2017-03-03 18:54:20]

Description was changed from

==========
Move beforeunload hang timer duties to its own timer.

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

to

==========
Move beforeunload 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-03 18:54:37]

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-03 19:45:06]

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

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

Dry run: This issue passed the CQ dry run.

[Avi (use Gerrit), 2017-03-03 20:27:38]

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

[commit-bot: I haz the power, 2017-03-03 20:28:13]

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-03 20:38:27]

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

[commit-bot: I haz the power, 2017-03-03 20:38:28]

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

[Avi (use Gerrit), 2017-03-03 21:24:12]

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

[commit-bot: I haz the power, 2017-03-03 21:24:35]

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-03 22:14:39]

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

[commit-bot: I haz the power, 2017-03-03 22:14:39]

Dry run: This issue passed the CQ dry run.

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

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

[Avi (use Gerrit), 2017-03-03 22:24:43]

Charlie is OOO until Monday but I'd like his review.
Dave, what do you think? Once the pending input event timeout is just for
pending events, dealing with its correctness will be easier.

[dtapuska, 2017-03-06 18:23:46]

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3474:
"ChildProcess.HangRendererType", beforeunload_timeout_type_,
Is this safe? Declaring the histogram twice? The macro is going to load the
value into two different static fields. I imagine you probably want to make this
code common.

[Avi (use Gerrit), 2017-03-06 18:27:26]

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3474:
"ChildProcess.HangRendererType", beforeunload_timeout_type_,
On 2017/03/06 18:23:46, dtapuska wrote:
> Is this safe? Declaring the histogram twice? The macro is going to load the
> value into two different static fields. I imagine you probably want to make
this
> code common.

Wait.. you can't do this? I already did this in
https://codereview.chromium.org/2725993002/... dammit.

Is this a useful enumeration?

[Avi (use Gerrit), 2017-03-06 18:43:09]

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3474:
"ChildProcess.HangRendererType", beforeunload_timeout_type_,
On 2017/03/06 18:27:26, Avi wrote:
> On 2017/03/06 18:23:46, dtapuska wrote:
> > Is this safe? Declaring the histogram twice? The macro is going to load the
> > value into two different static fields. I imagine you probably want to make
> this
> > code common.
> 
> Wait.. you can't do this? I already did this in
> https://codereview.chromium.org/2725993002/... dammit.
> 
> Is this a useful enumeration?

Ilya says in chat: "I see.  That's fine, in terms of things working correctly.
We do generally recommend refactoring the code so that there's a shared
codepath, both to reduce on the amount of generated code (the macro expands to
several lines) and to reduce the likelihood of different paths diverging from
one another (basically the same motivation as the DRY principle in general)."

[Charlie Reis, 2017-03-06 21:10:44]

Nice.  I love the simplification of detangling the timers, and I don't see
immediate problems with splitting them apart.  Looks good, with one question
below.

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2628: :
render_view_host_->GetWidget()->hung_renderer_delay());
In the new approach where the beforeunload timer is split out, what are we
waiting for when |success| is false?  Do we get a beforeunload ACK in that case
as well?

I'm just trying to understand why we use the longer delay (or use the timer at
all) in that case.  It'd be nice if this can be simplified, since the comment
above doesn't seem to provide a strong reason for it.  If there is a reason,
though, I'm curious to understand it.

https://codereview.chromium.org/2727253005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (left):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:552: //
RenderViewHostImpl can account for in-flight beforeunload/unload events.
Yay!

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

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2628: :
render_view_host_->GetWidget()->hung_renderer_delay());
On 2017/03/06 21:10:44, Charlie Reis (OOO til Mar 6) wrote:
> In the new approach where the beforeunload timer is split out, what are we
> waiting for when |success| is false?  Do we get a beforeunload ACK in that
case
> as well?

We always get a beforeunload ACK, even when the user decides to stay on the
page. (In fact, the beforeunload ack is how the render process tells us whether
we need to stay or go.)

> I'm just trying to understand why we use the longer delay (or use the timer at
> all) in that case.

In that case? Or ever?

For "in that case", there's no reason to treat "stay" vs "leave" differently. In
both cases, we run the beforeunload handler, get told to run a dialog or not,
send the result back, and get an ack.

For "ever", that's actually a very interesting question. The fact that we get
told to run a dialog means that the page succeeded in running the beforeunload
handler to completion. We totally could say, "hey, we've seen a dialog, so we
know we didn't hang." That would be a change in what we do from today, and I
would want to put that in a different CL so that it's didn't get caught up in
this refactor. In addition, today we pass the result of the stay/go back to the
render process, and get returned it via the FrameHostMsg_BeforeUnload_ACK, so
there's some plumbing work that would need to be done.

It's totally doable but I want to focus on one refactor at a time.

https://codereview.chromium.org/2727253005/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_impl.h (left):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_impl.h:552: //
RenderViewHostImpl can account for in-flight beforeunload/unload events.
On 2017/03/06 21:10:44, Charlie Reis (OOO til Mar 6) wrote:
> Yay!

Acknowledged.

[Charlie Reis, 2017-03-06 23:47:56]

Thanks!  LGTM with the change mentioned below.

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2628: :
render_view_host_->GetWidget()->hung_renderer_delay());
On 2017/03/06 22:11:17, Avi wrote:
> On 2017/03/06 21:10:44, Charlie Reis (OOO til Mar 6) wrote:
> > In the new approach where the beforeunload timer is split out, what are we
> > waiting for when |success| is false?  Do we get a beforeunload ACK in that
> case
> > as well?
> 
> We always get a beforeunload ACK, even when the user decides to stay on the
> page. (In fact, the beforeunload ack is how the render process tells us
whether
> we need to stay or go.)
> 
> > I'm just trying to understand why we use the longer delay (or use the timer
at
> > all) in that case.
> 
> In that case? Or ever?
> 
> For "in that case", there's no reason to treat "stay" vs "leave" differently.
In
> both cases, we run the beforeunload handler, get told to run a dialog or not,
> send the result back, and get an ack.

Great-- maybe we can use the shorter timeout for both just to simplify things a
bit?  No reason to use the long one now that we're distinct from input events.

> For "ever", that's actually a very interesting question. The fact that we get
> told to run a dialog means that the page succeeded in running the beforeunload
> handler to completion. We totally could say, "hey, we've seen a dialog, so we
> know we didn't hang." That would be a change in what we do from today, and I
> would want to put that in a different CL so that it's didn't get caught up in
> this refactor. In addition, today we pass the result of the stay/go back to
the
> render process, and get returned it via the FrameHostMsg_BeforeUnload_ACK, so
> there's some plumbing work that would need to be done.
> 
> It's totally doable but I want to focus on one refactor at a time.

Cool.  Let's consider that for later.  (Agreed there's no reason to do it in
this CL.)

[Avi (use Gerrit), 2017-03-06 23:58:35]

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

[Avi (use Gerrit), 2017-03-06 23:59:15]

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:2628: :
render_view_host_->GetWidget()->hung_renderer_delay());
On 2017/03/06 23:47:56, Charlie Reis (OOO til Mar 8) wrote:
> Great-- maybe we can use the shorter timeout for both just to simplify things
a
> bit?  No reason to use the long one now that we're distinct from input events.

Sure!

https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:3474:
"ChildProcess.HangRendererType", beforeunload_timeout_type_,
On 2017/03/06 18:23:46, dtapuska wrote:
> I imagine you probably want to make this
> code common.

WebContentsImpl::RendererUnresponsive is a bunch of unrelated code, and
ChildProcess.HangRendererType registered the value of which of the unrelated
bits of code ran. This bug is about ripping apart RendererUnresponsive and
moving its random bits of code to be with the other code they're related to.

I want to entirely remove this enum as it doesn't make sense, but until then, I
don't want to merge this code into somewhere common. The whole point is that
there _is_ no common place to put it that makes sense.

Given that Ilya says it's safe, I'm assuming you're ok proceeding here?

[commit-bot: I haz the power, 2017-03-06 23:59:19]

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-07 00:55:06]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dtapuska, 2017-03-07 01:26:36]

On 2017/03/06 23:59:15, Avi wrote:
>
https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
> content/browser/frame_host/render_frame_host_impl.cc:2628: :
> render_view_host_->GetWidget()->hung_renderer_delay());
> On 2017/03/06 23:47:56, Charlie Reis (OOO til Mar 8) wrote:
> > Great-- maybe we can use the shorter timeout for both just to simplify
things
> a
> > bit?  No reason to use the long one now that we're distinct from input
events.
> 
> Sure!
> 
>
https://codereview.chromium.org/2727253005/diff/40001/content/browser/frame_h...
> content/browser/frame_host/render_frame_host_impl.cc:3474:
> "ChildProcess.HangRendererType", beforeunload_timeout_type_,
> On 2017/03/06 18:23:46, dtapuska wrote:
> > I imagine you probably want to make this
> > code common.
> 
> WebContentsImpl::RendererUnresponsive is a bunch of unrelated code, and
> ChildProcess.HangRendererType registered the value of which of the unrelated
> bits of code ran. This bug is about ripping apart RendererUnresponsive and
> moving its random bits of code to be with the other code they're related to.
> 
> I want to entirely remove this enum as it doesn't make sense, but until then,
I
> don't want to merge this code into somewhere common. The whole point is that
> there _is_ no common place to put it that makes sense.
> 
> Given that Ilya says it's safe, I'm assuming you're ok proceeding here?

Lgtm

[Avi (use Gerrit), 2017-03-07 05:47:02]

The CQ bit was checked by avi@chromium.org

[Avi (use Gerrit), 2017-03-07 05:47:03]

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/2727253005/#ps60001
(title: "timeout values")

[commit-bot: I haz the power, 2017-03-07 05:48:02]

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-07 08:09:53]

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

[commit-bot: I haz the power, 2017-03-07 08:09:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Avi (use Gerrit), 2017-03-07 16:04:15]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2017-03-07 16:06:36]

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-07 18:19:37]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1488902655927680,
"parent_rev": "545222f357e150eec7c10740b2f752cc2b30a215", "commit_rev":
"ddd4c22cebfb3be9570877a7fcfebeca24c5f83b"}

[commit-bot: I haz the power, 2017-03-07 18:20:19]

Description was changed from

==========
Move beforeunload 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 beforeunload 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/2727253005
Cr-Commit-Position: refs/heads/master@{#455140}
Committed:
https://chromium.googlesource.com/chromium/src/+/ddd4c22cebfb3be9570877a7fcfe...
==========

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

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/ddd4c22cebfb3be9570877a7fcfe...

[tmartino, 2017-03-07 22:06:16]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2740493004/ by tmartino@chromium.org.

The reason for reverting is: We're seeing reliable failures on Win7 as a result
of this patch:

BrowserTest.SingleBeforeUnloadAfterRedirect

Specific error:

[6776:6744:0307/133434.182:FATAL:render_frame_host_impl.cc(2555)] Check failed:
is_waiting_for_beforeunload_ack_..