New histogram to track overhead time spent around beforeunload event + fixes

Adds a new histogram to track the overhead time spent around handling the beforeunload event.

Also fixes a couple issues with the shortcutting of
RenderFrameHostImpl::OnBeforeUnloadACK in case there's no actual IPC call. The main issue was that when there was no active renderer these calls inside RenderFrameHostImpl::OnBeforeUnloadACK would be executed without the prior due mirror calls:
  render_view_host_->decrement_in_flight_event_count();
  render_view_host_->StopHangMonitorTimeout();

BUG=416877
Committed: https://crrev.com/caf1a4bb5487a8f84a7a6aa57dbc118b6c95dc90
Cr-Commit-Position: refs/heads/master@{#300484}

[carlosk, 2014-10-15 12:29:03]

carlosk@chromium.org changed reviewers:
+ clamy@chromium.org, isherman@chromium.org, nasko@chromium.org

[carlosk, 2014-10-15 12:29:04]

clamy, nasko: PTAL

isherman: PTAL @ histograms.xml.

This is the last Navigation histogram to add (for now).

https://codereview.chromium.org/659773002/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/659773002/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigator_impl.cc:890:
DCHECK(!navigation_data_->before_unload_delay_.InMicroseconds());
This needed to be removed as I found out that after restoring a session, when
one navigates back in history from a restored page that has beforeunload
registered, this condition will trigger.

https://codereview.chromium.org/659773002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
This wasn't really working anyway.

https://codereview.chromium.org/659773002/diff/1/content/browser/frame_host/r...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/659773002/diff/1/content/browser/frame_host/r...
content/browser/frame_host/render_frame_host_impl.cc:1312: "navigation",
"RenderFrameHostImpl::BeforeUnload", this);
I moved this trace start down for it doesn't seem like we need it if not doing
an IPC.

[clamy, 2014-10-15 12:50:57]

Thanks1 LGTM provided you change the histogram description (and the bots are
fine with the change in the call to OnBeforeUnloadACK).

https://chromiumcodereview.appspot.com/659773002/diff/1/tools/metrics/histogr...
File tools/metrics/histograms/histograms.xml (right):

https://chromiumcodereview.appspot.com/659773002/diff/1/tools/metrics/histogr...
tools/metrics/histograms/histograms.xml:14015: +    standpoint. In other words
its total time the browser spent minus the time
I think this is a bit misleading: it reads as the overhead happens only in the
browser, when actually it doesn't. How about rephrasing it as:
More precisely, it is the total time between dispatch and acknowledgement of the
BeforeUnload event on the browser side, minus the actual time spent executing
the BeforeUnload handlers on the renderer side.

[Ilya Sherman, 2014-10-15 22:50:49]

LGTM, thanks.

[carlosk, 2014-10-16 07:43:41]

Thanks. This PS is just to fix the comment as clamy@ suggested. 

About the failing test, android_dbg_tests_recipe, it initially seemed like
something regarding my changes for it was related to beforeunload but:
* It failed in the first try bots run
* I ran it locally on a Nexus 5 and it passed
* I requested try bots to run it once more and it passed.

Let's see how it will behave for this new PS but I think it's failure was due to
flaky-ness.

https://codereview.chromium.org/659773002/diff/1/tools/metrics/histograms/his...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/659773002/diff/1/tools/metrics/histograms/his...
tools/metrics/histograms/histograms.xml:14015: +    standpoint. In other words
its total time the browser spent minus the time
On 2014/10/15 12:50:56, clamy wrote:
> I think this is a bit misleading: it reads as the overhead happens only in the
> browser, when actually it doesn't. How about rephrasing it as:
> More precisely, it is the total time between dispatch and acknowledgement of
the
> BeforeUnload event on the browser side, minus the actual time spent executing
> the BeforeUnload handlers on the renderer side.

Done.

[Charlie Reis, 2014-10-16 22:11:59]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2014-10-16 22:12:00]

Thanks for catching that bug in the shortcutting logic!  I do think we want to
do something like this, but I want to be sure the other cases make sense as
well.  If it works in those cases, this should be fine.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:827: if (GetParent()) {
There are more callers of OnBeforeUnloadACK than just the IPC handler and
DispatchBeforeUnload.  What happens when a subframe shortcuts in
DidCommitProvisionalLoad?  Will this affect any of the tests using
TestRenderFrameHost::SendBeforeUnloadACK?

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
Why remove this?

[carlosk, 2014-10-17 11:55:06]

Thanks Charlie.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:827: if (GetParent()) {
On 2014/10/16 22:12:00, Charlie Reis wrote:
> There are more callers of OnBeforeUnloadACK than just the IPC handler and
> DispatchBeforeUnload.  What happens when a subframe shortcuts in
> DidCommitProvisionalLoad?  Will this affect any of the tests using
> TestRenderFrameHost::SendBeforeUnloadACK?

If we can trust code search there's only two other places place where it's also
called from (apart from the IPCs):
* RenderFrameHostImpl::OnDidCommitProvisionalLoad, as you mentioned;
* TestRenderFrameHost::SendBeforeUnloadACK.

As all try bots are green I'm assuming all currently mapped situations are still
working fine.

For the specific subframe situation it doesn't seem that's a possible code path.
For that shortcut to be triggered is_waiting_for_beforeunload_ack_ must be true
so DispatchBeforeUnload must have been called before *not* from a subframe. And
if I understand the current architecture a RenderFrameHostImpl instance is
whether mapped to a main frame XOR to a subframe and this can't change during
its lifetime, right?

Also in OnDidCommitProvisionalLoad beside checking for
is_waiting_for_beforeunload_ack_ it also checks for
ui::PageTransitionIsMainFrame before short-cutting into OnBeforeUnloadACK.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
On 2014/10/16 22:12:00, Charlie Reis wrote:
> Why remove this?

Because this is a pointless check: send_before_unload_start_time_ was *never*
reset after it's set the 1st time. It seemed like it would serve as a flag but
that would never be the case.

Also for this if clause to be executed is_waiting_for_beforeunload_ack_ must
have been set to true (see early return @ 846) and if that's the case
send_before_unload_start_time_ would have been set in the earlier call to
DispatchBeforeUnload.

[Charlie Reis, 2014-10-17 23:52:46]

Thanks for looking into it.  LGTM with the suggestions below.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:827: if (GetParent()) {
On 2014/10/17 11:55:06, carlosk wrote:
> On 2014/10/16 22:12:00, Charlie Reis wrote:
> > There are more callers of OnBeforeUnloadACK than just the IPC handler and
> > DispatchBeforeUnload.  What happens when a subframe shortcuts in
> > DidCommitProvisionalLoad?  Will this affect any of the tests using
> > TestRenderFrameHost::SendBeforeUnloadACK?
> 
> If we can trust code search there's only two other places place where it's
also
> called from (apart from the IPCs):
> * RenderFrameHostImpl::OnDidCommitProvisionalLoad, as you mentioned;
> * TestRenderFrameHost::SendBeforeUnloadACK.
> 
> As all try bots are green I'm assuming all currently mapped situations are
still
> working fine.

You have more confidence in our tests than I do.  :)  Also, I'm not sure a test
would fail in a clear way if it called SendBeforeUnloadACK for a subframe-- it
would just execute the logic below and possible confuse things.

Since we don't expect to call this for subframes anymore, can we put a
DCHECK(!GetParent()) here instead?

> 
> For the specific subframe situation it doesn't seem that's a possible code
path.
> For that shortcut to be triggered is_waiting_for_beforeunload_ack_ must be
true
> so DispatchBeforeUnload must have been called before *not* from a subframe.
And
> if I understand the current architecture a RenderFrameHostImpl instance is
> whether mapped to a main frame XOR to a subframe and this can't change during
> its lifetime, right?

I don't understand this last sentence, but the rest sounds right.

> Also in OnDidCommitProvisionalLoad beside checking for
> is_waiting_for_beforeunload_ack_ it also checks for
> ui::PageTransitionIsMainFrame before short-cutting into OnBeforeUnloadACK.

Yes, you're right-- that limits that case to main frames.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
On 2014/10/17 11:55:06, carlosk wrote:
> On 2014/10/16 22:12:00, Charlie Reis wrote:
> > Why remove this?
> 
> Because this is a pointless check: send_before_unload_start_time_ was *never*
> reset after it's set the 1st time. It seemed like it would serve as a flag but
> that would never be the case.
> 
> Also for this if clause to be executed is_waiting_for_beforeunload_ack_ must
> have been set to true (see early return @ 846) and if that's the case
> send_before_unload_start_time_ would have been set in the earlier call to
> DispatchBeforeUnload.

Yes, I agree that seems confusing.  Can we clean it up a bit instead?  I would
suggest resetting it when we set is_waiting_for_beforeunload_ack_ to false, and
DCHECKing that it's not null when we arrive in this function (which is a no-op
in release builds but shows the invariant we expect).

And just to clarify, we're keeping the renderer_before_unload_{start,end}_time
checks because they come from a renderer IPC and can't be trusted, right?  That
sounds fine to me.

[carlosk, 2014-10-20 17:40:34]

Thanks. 

creis@: just one more question in my replies for your comments before I can
commit this.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:827: if (GetParent()) {
On 2014/10/17 23:52:45, Charlie Reis wrote:
> On 2014/10/17 11:55:06, carlosk wrote:
> > On 2014/10/16 22:12:00, Charlie Reis wrote:
> > > There are more callers of OnBeforeUnloadACK than just the IPC handler and
> > > DispatchBeforeUnload.  What happens when a subframe shortcuts in
> > > DidCommitProvisionalLoad?  Will this affect any of the tests using
> > > TestRenderFrameHost::SendBeforeUnloadACK?
> > 
> > If we can trust code search there's only two other places place where it's
> also
> > called from (apart from the IPCs):
> > * RenderFrameHostImpl::OnDidCommitProvisionalLoad, as you mentioned;
> > * TestRenderFrameHost::SendBeforeUnloadACK.
> > 
> > As all try bots are green I'm assuming all currently mapped situations are
> still
> > working fine.
> 
> You have more confidence in our tests than I do.  :)  Also, I'm not sure a
test
> would fail in a clear way if it called SendBeforeUnloadACK for a subframe-- it
> would just execute the logic below and possible confuse things.

Ignorance is a bliss. ;) But I got your concern.

> Since we don't expect to call this for subframes anymore, can we put a
> DCHECK(!GetParent()) here instead?

Done.

> > For the specific subframe situation it doesn't seem that's a possible code
> path.
> > For that shortcut to be triggered is_waiting_for_beforeunload_ack_ must be
> true
> > so DispatchBeforeUnload must have been called before *not* from a subframe.
> And
> > if I understand the current architecture a RenderFrameHostImpl instance is
> > whether mapped to a main frame XOR to a subframe and this can't change
during
> > its lifetime, right?
> 
> I don't understand this last sentence, but the rest sounds right.

I was just trying to confirm that for any specific RenderFrameHostImpl instance
a call to its GetParent() will never change the result. In other words the
instance is assigned whether to a main frame, whether to a subframe, and that
fact never changes during its lifetime.
 
> > Also in OnDidCommitProvisionalLoad beside checking for
> > is_waiting_for_beforeunload_ack_ it also checks for
> > ui::PageTransitionIsMainFrame before short-cutting into OnBeforeUnloadACK.
> 
> Yes, you're right-- that limits that case to main frames.

Nice.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
On 2014/10/17 23:52:46, Charlie Reis wrote:
> On 2014/10/17 11:55:06, carlosk wrote:
> > On 2014/10/16 22:12:00, Charlie Reis wrote:
> > > Why remove this?
> > 
> > Because this is a pointless check: send_before_unload_start_time_ was
*never*
> > reset after it's set the 1st time. It seemed like it would serve as a flag
but
> > that would never be the case.
> > 
> > Also for this if clause to be executed is_waiting_for_beforeunload_ack_ must
> > have been set to true (see early return @ 846) and if that's the case
> > send_before_unload_start_time_ would have been set in the earlier call to
> > DispatchBeforeUnload.
> 
> Yes, I agree that seems confusing.  Can we clean it up a bit instead?  I would
> suggest resetting it when we set is_waiting_for_beforeunload_ack_ to false,
and
> DCHECKing that it's not null when we arrive in this function (which is a no-op
> in release builds but shows the invariant we expect).

Done the send_before_unload_start_time_ resetting. I created a copy of it here
in this method so that the resetting happens along with
is_waiting_for_beforeunload_ack_'s. Also reset it in the one other place.

This change triggered an error in
WebContentsImplTest::CrossSiteNotPreemptedDuringBeforeUnload which I fixed by
providing a default value of renderer_before_unload_end_time to
before_unload_end_time.

> And just to clarify, we're keeping the renderer_before_unload_{start,end}_time
> checks because they come from a renderer IPC and can't be trusted, right? 
That
> sounds fine to me.

IDK about the motivations to that checking. Looking at RenderFrameImpl code
(https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
there's no way those values would return unset. It sounds like the safest choice
but for a renderer that much foobar to return invalid values in this situation I
think crashing is not that bad? I could replaces this if-clause with a couple
CHECK() calls. WDYT?

[Charlie Reis, 2014-10-20 18:22:21]

LGTM.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:827: if (GetParent()) {
On 2014/10/20 17:40:34, carlosk wrote:
> > > For the specific subframe situation it doesn't seem that's a possible code
> > path.
> > > For that shortcut to be triggered is_waiting_for_beforeunload_ack_ must be
> > true
> > > so DispatchBeforeUnload must have been called before *not* from a
subframe.
> > And
> > > if I understand the current architecture a RenderFrameHostImpl instance is
> > > whether mapped to a main frame XOR to a subframe and this can't change
> during
> > > its lifetime, right?
> > 
> > I don't understand this last sentence, but the rest sounds right.
> 
> I was just trying to confirm that for any specific RenderFrameHostImpl
instance
> a call to its GetParent() will never change the result. In other words the
> instance is assigned whether to a main frame, whether to a subframe, and that
> fact never changes during its lifetime.

A RenderFrameHostImpl is specific to a frame and can't change between a main
frame and a subframe, yes.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
On 2014/10/20 17:40:34, carlosk wrote:
> On 2014/10/17 23:52:46, Charlie Reis wrote:
> > On 2014/10/17 11:55:06, carlosk wrote:
> > > On 2014/10/16 22:12:00, Charlie Reis wrote:
> > > > Why remove this?
> > > 
> > > Because this is a pointless check: send_before_unload_start_time_ was
> *never*
> > > reset after it's set the 1st time. It seemed like it would serve as a flag
> but
> > > that would never be the case.
> > > 
> > > Also for this if clause to be executed is_waiting_for_beforeunload_ack_
must
> > > have been set to true (see early return @ 846) and if that's the case
> > > send_before_unload_start_time_ would have been set in the earlier call to
> > > DispatchBeforeUnload.
> > 
> > Yes, I agree that seems confusing.  Can we clean it up a bit instead?  I
would
> > suggest resetting it when we set is_waiting_for_beforeunload_ack_ to false,
> and
> > DCHECKing that it's not null when we arrive in this function (which is a
no-op
> > in release builds but shows the invariant we expect).
> 
> Done the send_before_unload_start_time_ resetting. I created a copy of it here
> in this method so that the resetting happens along with
> is_waiting_for_beforeunload_ack_'s. Also reset it in the one other place.
> 
> This change triggered an error in
> WebContentsImplTest::CrossSiteNotPreemptedDuringBeforeUnload which I fixed by
> providing a default value of renderer_before_unload_end_time to
> before_unload_end_time.
> 
> > And just to clarify, we're keeping the
renderer_before_unload_{start,end}_time
> > checks because they come from a renderer IPC and can't be trusted, right? 
> That
> > sounds fine to me.
> 
> IDK about the motivations to that checking. Looking at RenderFrameImpl code
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
> there's no way those values would return unset. It sounds like the safest
choice
> but for a renderer that much foobar to return invalid values in this situation
I
> think crashing is not that bad? I could replaces this if-clause with a couple
> CHECK() calls. WDYT?

No, we shouldn't trust the renderer process.  In the browser process, we should
always treat such values as untrusted because the process could be exploited.  A
CHECK would just give an exploited renderer another way to crash the browser
process.

[carlosk, 2014-10-21 13:07:09]

Thanks again Charlie. I added a few comments (in code and as review comments) to
explain my latest changes.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (left):

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:827: if (GetParent()) {
On 2014/10/20 18:22:21, Charlie Reis wrote:
> On 2014/10/20 17:40:34, carlosk wrote:
> > > > For the specific subframe situation it doesn't seem that's a possible
code
> > > path.
> > > > For that shortcut to be triggered is_waiting_for_beforeunload_ack_ must
be
> > > true
> > > > so DispatchBeforeUnload must have been called before *not* from a
> subframe.
> > > And
> > > > if I understand the current architecture a RenderFrameHostImpl instance
is
> > > > whether mapped to a main frame XOR to a subframe and this can't change
> > during
> > > > its lifetime, right?
> > > 
> > > I don't understand this last sentence, but the rest sounds right.
> > 
> > I was just trying to confirm that for any specific RenderFrameHostImpl
> instance
> > a call to its GetParent() will never change the result. In other words the
> > instance is assigned whether to a main frame, whether to a subframe, and
that
> > fact never changes during its lifetime.
> 
> A RenderFrameHostImpl is specific to a frame and can't change between a main
> frame and a subframe, yes.

Acknowledged.

https://codereview.chromium.org/659773002/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:852: if
(!send_before_unload_start_time_.is_null() &&
On 2014/10/20 18:22:21, Charlie Reis wrote:
> On 2014/10/20 17:40:34, carlosk wrote:
> > On 2014/10/17 23:52:46, Charlie Reis wrote:
> > > On 2014/10/17 11:55:06, carlosk wrote:
> > > > On 2014/10/16 22:12:00, Charlie Reis wrote:
> > > > > Why remove this?
> > > > 
> > > > Because this is a pointless check: send_before_unload_start_time_ was
> > *never*
> > > > reset after it's set the 1st time. It seemed like it would serve as a
flag
> > but
> > > > that would never be the case.
> > > > 
> > > > Also for this if clause to be executed is_waiting_for_beforeunload_ack_
> must
> > > > have been set to true (see early return @ 846) and if that's the case
> > > > send_before_unload_start_time_ would have been set in the earlier call
to
> > > > DispatchBeforeUnload.
> > > 
> > > Yes, I agree that seems confusing.  Can we clean it up a bit instead?  I
> would
> > > suggest resetting it when we set is_waiting_for_beforeunload_ack_ to
false,
> > and
> > > DCHECKing that it's not null when we arrive in this function (which is a
> no-op
> > > in release builds but shows the invariant we expect).
> > 
> > Done the send_before_unload_start_time_ resetting. I created a copy of it
here
> > in this method so that the resetting happens along with
> > is_waiting_for_beforeunload_ack_'s. Also reset it in the one other place.
> > 
> > This change triggered an error in
> > WebContentsImplTest::CrossSiteNotPreemptedDuringBeforeUnload which I fixed
by
> > providing a default value of renderer_before_unload_end_time to
> > before_unload_end_time.
> > 
> > > And just to clarify, we're keeping the
> renderer_before_unload_{start,end}_time
> > > checks because they come from a renderer IPC and can't be trusted, right? 
> > That
> > > sounds fine to me.
> > 
> > IDK about the motivations to that checking. Looking at RenderFrameImpl code
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...)
> > there's no way those values would return unset. It sounds like the safest
> choice
> > but for a renderer that much foobar to return invalid values in this
situation
> I
> > think crashing is not that bad? I could replaces this if-clause with a
couple
> > CHECK() calls. WDYT?
> 
> No, we shouldn't trust the renderer process.  In the browser process, we
should
> always treat such values as untrusted because the process could be exploited. 
A
> CHECK would just give an exploited renderer another way to crash the browser
> process.

Acknowledged.

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:710: base::TimeTicks
approx_renderer_start_time = send_before_unload_start_time_;
I'm making this copy because it was unsafe to pass a reference to
send_before_unload_start_time_ as that value could be reset anytime.

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:851: base::TimeTicks
before_unload_end_time = renderer_before_unload_end_time;
So finally, this initialization was not the correct solution for the problem I
found out before (the previous comment is the fix) but it is required if we want
the browser not to die from a hacked renderer.

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:897:
send_before_unload_start_time_ = base::TimeTicks();
I moved the state reset down here so that both variables could be changed at the
same time. Otherwise I'd have to whether split these two statements apart or
make a copy of send_before_unload_start_time_.

[carlosk, 2014-10-21 13:22:27]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2014-10-21 13:23:15]

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

[commit-bot: I haz the power, 2014-10-21 14:17:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 14:17:51]

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

[Charlie Reis, 2014-10-21 18:21:49]

Thanks, works for me.

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:710: base::TimeTicks
approx_renderer_start_time = send_before_unload_start_time_;
On 2014/10/21 13:07:09, carlosk wrote:
> I'm making this copy because it was unsafe to pass a reference to
> send_before_unload_start_time_ as that value could be reset anytime.

"Could be reset anytime" doesn't seem relevant here.  It only seems like this is
needed if OnBeforeUnloadACK resets send_before_unload_start_time_ before
accessing renderer_before_unload_start_time, since they're aliased.

That said, this is safer against future changes, so it's fine.

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:895: // Resets beforeunload
waiting state.
nit: There should have been a blank line before this because it's separate from
the block above.  (Not worth its own CL, but for future reference.)

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:897:
send_before_unload_start_time_ = base::TimeTicks();
On 2014/10/21 13:07:09, carlosk wrote:
> I moved the state reset down here so that both variables could be changed at
the
> same time. Otherwise I'd have to whether split these two statements apart or
> make a copy of send_before_unload_start_time_.

Acknowledged.

[carlosk, 2014-10-22 09:08:56]

Thanks.

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:710: base::TimeTicks
approx_renderer_start_time = send_before_unload_start_time_;
On 2014/10/21 18:21:48, Charlie Reis wrote:
> On 2014/10/21 13:07:09, carlosk wrote:
> > I'm making this copy because it was unsafe to pass a reference to
> > send_before_unload_start_time_ as that value could be reset anytime.
> 
> "Could be reset anytime" doesn't seem relevant here.  It only seems like this
is
> needed if OnBeforeUnloadACK resets send_before_unload_start_time_ before
> accessing renderer_before_unload_start_time, since they're aliased.
> 
> That said, this is safer against future changes, so it's fine.

Yeah I agree with that: it isn't really required. But future-proofing was
exactly what drove me to do it. In fact in a previous version of the code I did
precisely what you described and suddenly renderer_before_unload_start_time was
unexpectedly turning up as 0. :)

https://codereview.chromium.org/659773002/diff/60001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:895: // Resets beforeunload
waiting state.
On 2014/10/21 18:21:48, Charlie Reis wrote:
> nit: There should have been a blank line before this because it's separate
from
> the block above.  (Not worth its own CL, but for future reference.)

Acknowledged.