Amend sanitization of browser-side navigationStart override.

Amend sanitization of browser-side navigationStart override.

When setting the navigationStart recorded on the browser side for
browser-initiated navigations, we need to ensure the time is lower than
the time of starting the load on the renderer side. Otherwise with heavy
clock differences between processes and particularly fast navigation
start-ups we could end up pushing the navigationStart *forward* and
hitting an assert that is being deployed in
https://codereview.chromium.org/379873002/ .

BUG=376004
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283761

[ppi, 2014-07-10 14:59:45]

Hi John, could you take a look?

[ppi, 2014-07-11 09:32:40]

Ah, John is ooo. Jochen, could you take a look?

[jochen (gone - plz use gerrit), 2014-07-14 11:17:05]

is it possible to write a test for this?

[ppi, 2014-07-16 17:34:49]

Kind of..., sure.

It is a messy case where we calculate and set the override using TimeTicks (and
rightly so), but the Blink API returns wall time. Deltas in one should be in
most cases roughly correspond to deltas in another, but we can't compare them
reliably.

I added a test asserting most basic constraints in patch set 4, I'm not sure how
much value does it add.

BTW, this change allows to land a Blink ASSERT in
https://codereview.chromium.org/379873002/, increasing the total amount of
sanity-checking around this logic.

Wdyt?

[jochen (gone - plz use gerrit), 2014-07-17 09:49:31]

lgtm, thx

[ppi, 2014-07-17 09:57:28]

Thanks!

[ppi, 2014-07-17 09:57:31]

The CQ bit was checked by ppi@chromium.org

[commit-bot: I haz the power, 2014-07-17 09:59:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ppi@chromium.org/384503009/80001

[commit-bot: I haz the power, 2014-07-17 12:31:12]

Change committed as 283761

[pasko, 2014-07-17 13:18:00]

https://codereview.chromium.org/384503009/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/384503009/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:904: base::TimeTicks navigation_start =
std::min(
This min() could be a reason of bi-modal distribution in navigation metrics,
which makes it quite hard to analyse the metric with statistical methods.

If we record the time upon sending and receiving the IPC in the corresponding
processes as |browser_sent_navigation| and |renderer_received_navigation|, then
we can account for this offset in renderer-side navigation timings.

Proposal:

navigation_start = renderer_received_navigation - (browser_sent_navigation -
params.browser_navigation_start)

This way we will be discounting for the IPC travel time, which is not ideal, but
I like thinking that IPC travel time depends more on how the OS behaves and is
somewhat out of our control. I have no better way in mind for synchronizing time
across processes, but this min() time feels confusing, non-monotonic and
<censored>.

[ppi, 2014-07-17 13:27:43]

https://codereview.chromium.org/384503009/diff/80001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/384503009/diff/80001/content/renderer/render_...
content/renderer/render_frame_impl.cc:904: base::TimeTicks navigation_start =
std::min(
On 2014/07/17 13:18:00, pasko wrote:
> This min() could be a reason of bi-modal distribution in navigation metrics,
> which makes it quite hard to analyse the metric with statistical methods.
> 
> If we record the time upon sending and receiving the IPC in the corresponding
> processes as |browser_sent_navigation| and |renderer_received_navigation|,
then
> we can account for this offset in renderer-side navigation timings.
> 
> Proposal:
> 
> navigation_start = renderer_received_navigation - (browser_sent_navigation -
> params.browser_navigation_start)
> 
> This way we will be discounting for the IPC travel time, which is not ideal,
but
> I like thinking that IPC travel time depends more on how the OS behaves and is
> somewhat out of our control. I have no better way in mind for synchronizing
time
> across processes, but this min() time feels confusing, non-monotonic and
> <censored>.

This is an excellent idea, also solving the problem of big clock differences
between processes on Windows. Let me file a bug and let's see what the other
people think.

Thanks!!