Create NetworkTimeTracker on startup for users in Finch experiment

Create NetworkTimeTracker on startup for users in Finch experiment

On Canary/Dev, we are experimenting with a secure network time
service. The NetworkTimeTracker queries the time service to receive a
secure timestamp, and then maintains an offset from that time to
calculate the current time, only re-querying when it has lost sync (due
to a suspend/resume cycle, for example). We are using the secure
timestamp to make SSL errors more actionable when they are caused by bad
client clocks.

The NetworkTimeTracker makes its first query when it is first created,
and the NetworkTimeTracker is created when it is first accessed (which
is not necessarily on startup). Data from UMA, however, shows that the
first query happens too late, and that many SSL errors occur before the
first query. To make the secure time queries more useful for SSL errors
(which may happen very soon after startup -- for example if the user
immediately navigates to https://google.com, or tabs reload upon
startup), this CL ensures that the NetworkTimeTracker is created on
startup (only for users who are in the Finch trial).

BUG=589700
Committed: https://crrev.com/4f480f4f6f1596f54d6de2c149fafbd72ed33030
Cr-Commit-Position: refs/heads/master@{#421842}

[estark, 2016-09-24 06:31:27]

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

[commit-bot: I haz the power, 2016-09-24 06:31:39]

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

[estark, 2016-09-24 06:33:10]

estark@chromium.org changed reviewers:
+ mab@google.com

[estark, 2016-09-24 06:33:11]

mab, do you think this is reasonable?

[commit-bot: I haz the power, 2016-09-24 06:42:50]

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

[commit-bot: I haz the power, 2016-09-24 06:42:50]

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

[estark, 2016-09-24 06:55:30]

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

[commit-bot: I haz the power, 2016-09-24 06:55:39]

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, 2016-09-24 07:28:33]

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

[commit-bot: I haz the power, 2016-09-24 07:28:34]

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

[estark, 2016-09-25 16:12:20]

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

[commit-bot: I haz the power, 2016-09-25 16:12:27]

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, 2016-09-25 16:45:56]

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

[commit-bot: I haz the power, 2016-09-25 16:45:57]

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_...)

[estark, 2016-09-25 22:49:54]

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

[commit-bot: I haz the power, 2016-09-25 22:49:58]

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, 2016-09-25 23:57:51]

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

[commit-bot: I haz the power, 2016-09-25 23:57:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mab, 2016-09-26 21:45:09]

lgtm

seems legit

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1075: system_request_context()));
Note that the network_time_tracker() getter above also can create a
NetworkTimeTracker.  You could call it here, although it is a bit obscure that
calling that innocently-named getter has the side effect of creating the thing
in question.

[mab, 2016-09-26 21:45:11]

lgtm

seems legit

[estark, 2016-09-26 21:55:40]

estark@chromium.org changed reviewers:
+ thakis@chromium.org, zea@chromium.org

[estark, 2016-09-26 21:55:40]

+thakis for chrome/browser/browser_process_impl.cc
+zea for components/network_time

I tried to include all the context for this change in the CL description, but
please let me know if anything's not clear. Thanks!

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1075: system_request_context()));
On 2016/09/26 21:45:09, mab wrote:
> Note that the network_time_tracker() getter above also can create a
> NetworkTimeTracker.  You could call it here, although it is a bit obscure that
> calling that innocently-named getter has the side effect of creating the thing
> in question.

Yeah, I considered that but thought it was a little too weird to call a getter
for its side effects.

[Nico, 2016-09-27 15:51:24]

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1072: network_time_tracker_.reset(new
network_time::NetworkTimeTracker(
Does this do any amount of real work that could slow down startup?

[Nicolas Zea, 2016-09-27 17:17:06]

network_time lgtm

[estark, 2016-09-27 18:56:24]

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1072: network_time_tracker_.reset(new
network_time::NetworkTimeTracker(
On 2016/09/27 15:51:24, Nico (mostly away until Oct 3) wrote:
> Does this do any amount of real work that could slow down startup?

It reads a pref and then, in some cases, starts a URLFetcher to the time server.

Since the request is async I think this should not slow down startup. If it does
for some reason, I think we'd get a Chirp alert about it (because we're only
doing this for users in the Finch trial)?

[Nico, 2016-09-27 19:07:23]

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1072: network_time_tracker_.reset(new
network_time::NetworkTimeTracker(
On 2016/09/27 18:56:24, estark wrote:
> On 2016/09/27 15:51:24, Nico (mostly away until Oct 3) wrote:
> > Does this do any amount of real work that could slow down startup?
> 
> It reads a pref and then, in some cases, starts a URLFetcher to the time
server.
> 
> Since the request is async I think this should not slow down startup. If it
does
> for some reason, I think we'd get a Chirp alert about it (because we're only
> doing this for users in the Finch trial)?

Reading prefs is ok. Is the network request needed right at startup? Or could
that be delayed a bit? Startup is a busy place.

[estark, 2016-09-27 19:37:44]

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:1072: network_time_tracker_.reset(new
network_time::NetworkTimeTracker(
On 2016/09/27 19:07:23, Nico (mostly away until Oct 3) wrote:
> On 2016/09/27 18:56:24, estark wrote:
> > On 2016/09/27 15:51:24, Nico (mostly away until Oct 3) wrote:
> > > Does this do any amount of real work that could slow down startup?
> > 
> > It reads a pref and then, in some cases, starts a URLFetcher to the time
> server.
> > 
> > Since the request is async I think this should not slow down startup. If it
> does
> > for some reason, I think we'd get a Chirp alert about it (because we're only
> > doing this for users in the Finch trial)?
> 
> Reading prefs is ok. Is the network request needed right at startup? Or could
> that be delayed a bit? Startup is a busy place.

It is, unfortunately, needed right at startup. The scenario is that the user's
system clock is wrong and they start up Chrome, which immediately restores their
tabs, resulting in scary certificate validation errors on all their tabs.
Instead we'd like to have secure time available before those tabs load, so that
we can show a helpful error message ("Please fix your clock") instead of a
generic scary one ("Your connection is not private").

Do you think it would help at all to delay a little bit, like one second? We
could potentially delay showing the certificate validation errors at startup for
a few seconds until the first time query completes.

[Nico, 2016-09-27 20:12:22]

On 2016/09/27 19:37:44, estark wrote:
>
https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
> File chrome/browser/browser_process_impl.cc (right):
> 
>
https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
> chrome/browser/browser_process_impl.cc:1072: network_time_tracker_.reset(new
> network_time::NetworkTimeTracker(
> On 2016/09/27 19:07:23, Nico (mostly away until Oct 3) wrote:
> > On 2016/09/27 18:56:24, estark wrote:
> > > On 2016/09/27 15:51:24, Nico (mostly away until Oct 3) wrote:
> > > > Does this do any amount of real work that could slow down startup?
> > > 
> > > It reads a pref and then, in some cases, starts a URLFetcher to the time
> > server.
> > > 
> > > Since the request is async I think this should not slow down startup. If
it
> > does
> > > for some reason, I think we'd get a Chirp alert about it (because we're
only
> > > doing this for users in the Finch trial)?
> > 
> > Reading prefs is ok. Is the network request needed right at startup? Or
could
> > that be delayed a bit? Startup is a busy place.
> 
> It is, unfortunately, needed right at startup. The scenario is that the user's
> system clock is wrong and they start up Chrome, which immediately restores
their
> tabs, resulting in scary certificate validation errors on all their tabs.
> Instead we'd like to have secure time available before those tabs load, so
that
> we can show a helpful error message ("Please fix your clock") instead of a
> generic scary one ("Your connection is not private").
> 
> Do you think it would help at all to delay a little bit, like one second? We
> could potentially delay showing the certificate validation errors at startup
for
> a few seconds until the first time query completes.

It sounds like it doesn't have to be right at startup, but it sounds like tab
loads will be blocked until this request is done (but bringing up the browser UI
shouldn't be blocked on this). So I think you might want to start this later
(this is in PreMainMessageLoopRun -- if you're starting network requests, isn't
"before message loops" a bit early anyhow?). lgtm if you want to look at other
things first, but that's something you should bring up during eng review for
launch.

[estark, 2016-09-28 05:04:23]

On 2016/09/27 20:12:22, Nico (mostly away until Oct 3) wrote:
> On 2016/09/27 19:37:44, estark wrote:
> >
>
https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
> > File chrome/browser/browser_process_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/2366943003/diff/60001/chrome/browser/browser_...
> > chrome/browser/browser_process_impl.cc:1072: network_time_tracker_.reset(new
> > network_time::NetworkTimeTracker(
> > On 2016/09/27 19:07:23, Nico (mostly away until Oct 3) wrote:
> > > On 2016/09/27 18:56:24, estark wrote:
> > > > On 2016/09/27 15:51:24, Nico (mostly away until Oct 3) wrote:
> > > > > Does this do any amount of real work that could slow down startup?
> > > > 
> > > > It reads a pref and then, in some cases, starts a URLFetcher to the time
> > > server.
> > > > 
> > > > Since the request is async I think this should not slow down startup. If
> it
> > > does
> > > > for some reason, I think we'd get a Chirp alert about it (because we're
> only
> > > > doing this for users in the Finch trial)?
> > > 
> > > Reading prefs is ok. Is the network request needed right at startup? Or
> could
> > > that be delayed a bit? Startup is a busy place.
> > 
> > It is, unfortunately, needed right at startup. The scenario is that the
user's
> > system clock is wrong and they start up Chrome, which immediately restores
> their
> > tabs, resulting in scary certificate validation errors on all their tabs.
> > Instead we'd like to have secure time available before those tabs load, so
> that
> > we can show a helpful error message ("Please fix your clock") instead of a
> > generic scary one ("Your connection is not private").
> > 
> > Do you think it would help at all to delay a little bit, like one second? We
> > could potentially delay showing the certificate validation errors at startup
> for
> > a few seconds until the first time query completes.
> 
> It sounds like it doesn't have to be right at startup, but it sounds like tab
> loads will be blocked until this request is done (but bringing up the browser
UI
> shouldn't be blocked on this). So I think you might want to start this later
> (this is in PreMainMessageLoopRun -- if you're starting network requests,
isn't
> "before message loops" a bit early anyhow?).

I don't know very much about how startup works (though I'd like to learn) -- is
there a place that seems more natural to you than PreMainMessageLoopRun? Looking
at BrowserProcessImpl the only other candidate I'm seeing might be
ResourceDispatcherHostCreated(). Maybe that would be a better place?

>  lgtm if you want to look at other
> things first, but that's something you should bring up during eng review for
> launch.

[estark, 2016-09-29 15:44:11]

I'm going to go ahead and land this to see how it affects our data coming in
from Canary users. If it looks good, I'll bring up the startup behavior in
launch review to make sure we are okay with this change and that we are doing
this in the right place.

[estark, 2016-09-29 15:59:36]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2016-09-29 15:59:58]

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, 2016-09-29 16:41:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-29 16:52:37]

Description was changed from

==========
Create NetworkTimeTracker on startup for users in Finch experiment

On Canary/Dev, we are experimenting with a secure network time
service. The NetworkTimeTracker queries the time service to receive a
secure timestamp, and then maintains an offset from that time to
calculate the current time, only re-querying when it has lost sync (due
to a suspend/resume cycle, for example). We are using the secure
timestamp to make SSL errors more actionable when they are caused by bad
client clocks.

The NetworkTimeTracker makes its first query when it is first created,
and the NetworkTimeTracker is created when it is first accessed (which
is not necessarily on startup). Data from UMA, however, shows that the
first query happens too late, and that many SSL errors occur before the
first query. To make the secure time queries more useful for SSL errors
(which may happen very soon after startup -- for example if the user
immediately navigates to https://google.com, or tabs reload upon
startup), this CL ensures that the NetworkTimeTracker is created on
startup (only for users who are in the Finch trial).

BUG=589700
==========

to

==========
Create NetworkTimeTracker on startup for users in Finch experiment

On Canary/Dev, we are experimenting with a secure network time
service. The NetworkTimeTracker queries the time service to receive a
secure timestamp, and then maintains an offset from that time to
calculate the current time, only re-querying when it has lost sync (due
to a suspend/resume cycle, for example). We are using the secure
timestamp to make SSL errors more actionable when they are caused by bad
client clocks.

The NetworkTimeTracker makes its first query when it is first created,
and the NetworkTimeTracker is created when it is first accessed (which
is not necessarily on startup). Data from UMA, however, shows that the
first query happens too late, and that many SSL errors occur before the
first query. To make the secure time queries more useful for SSL errors
(which may happen very soon after startup -- for example if the user
immediately navigates to https://google.com, or tabs reload upon
startup), this CL ensures that the NetworkTimeTracker is created on
startup (only for users who are in the Finch trial).

BUG=589700

Committed: https://crrev.com/4f480f4f6f1596f54d6de2c149fafbd72ed33030
Cr-Commit-Position: refs/heads/master@{#421842}
==========

[commit-bot: I haz the power, 2016-09-29 16:52:38]

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