Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1772143002:
Use network time for bad clock interstitial. (Closed)
|
Created:
4 years, 9 months ago by mab Modified:
4 years, 9 months ago CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse network time for bad clock interstitial.
This change supplements the existing build-time heuristic (which
renders the bad-clock interstitial only when the system clock is outside
the range (build-2d,build+365d)) with a comparison of the system clock
to network time, fetched from |NetworkTimeTracker|.
Network time has priority: if the network time says the clock is
accurate, the bad-clock intersitial will not be used.
The |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the
interstitial is shown, however the decision was made. The new
histograms |interstitial.ssl.clockstate.build_time| and
|interstitial.ssl.clockstate.network| provide data about the state of
the system clock whenever a |CERT_DATE_INVALID| error is encountered.
This should help us to assess the effects of the change.
Future work will focus on providing better network time in more cases.
BUG=589700
Committed: https://crrev.com/740b02b74735910ec8e185d6375d4eda3f15e0ba
Cr-Commit-Position: refs/heads/master@{#381837}
Patch Set 1 #Patch Set 2 : IOS #
Total comments: 26
Patch Set 3 : estark review 1 #
Total comments: 8
Patch Set 4 : estark review 2 #
Total comments: 16
Patch Set 5 : estark review 3 #
Total comments: 6
Patch Set 6 : felt review 1 #Patch Set 7 : estark review 4 #
Total comments: 5
Patch Set 8 : estark review 5 #Patch Set 9 : estark review 8 #Patch Set 10 : estark review 9 #
Total comments: 6
Patch Set 11 : estark review 10 #Patch Set 12 : pass "gn check out/Default" #Messages
Total messages: 53 (17 generated)
Description was changed from ========== Use network time for bad clock interstitial. This implementation could be improved, but I suspect it is perfectly sufficient. BUG= ========== to ========== Use network time for bad clock interstitial. This implementation could be improved, but I suspect it is perfectly sufficient. BUG=589700 ==========
mab@google.com changed reviewers: + estark@chromium.org
This CL is mostly to establish the plumbing for NetworkTimeTracker, but I see no reason not to actually make it work. Let me know what you think about testing (should I add a unit test, rather than launching a browser?) and instrumentation (should I change anything about what we send to UMA)?
Thanks! A few comments inline. Could you also please expand the CL description to indicate that this uses the network time when available and falls back to the existing build time heuristic otherwise? https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:908: mock_clock->SetNow(base::Time::NowFromSystemTime()); Can you please add a test like this one, except using network time to trigger the interstitial instead of build time? (It's a little redundant with the changes that you made to TestBadClockReporting above, but since the main point of that function is to test certificate reporting, not the bad clock interstitial, I think it's worth having a separate test exclusively to test that network time triggers the bad clock interstitial.) https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:149: const network_time::NetworkTimeTracker* network_time = nit: |network_time_tracker| instead of |network_time|... when I read |network_time| I assume it's a base::Time and it surprises me that it's a NetworkTimeTracker. https://codereview.chromium.org/1772143002/diff/20001/components/security_int... File components/security_interstitials/core/bad_clock_ui.h (right): https://codereview.chromium.org/1772143002/diff/20001/components/security_int... components/security_interstitials/core/bad_clock_ui.h:30: const network_time::NetworkTimeTracker* network_time, same nit here: |network_time_tracker| (and elsewhere too) https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); I think we should preserve the meaning of the existing UMA metrics; otherwise we'll have a period of time where we'll have to be very careful about how we query the data to make sure we get meaningful results and not mixed old-and-new data. To that end, I suggest: 1. Rename CLOCK_PAST to CLOCK_PAST_BY_BUILD_HEURISTIC (or something), same for CLOCK_FUTURE. I think renaming UMA histogram values is fine as long as the ordering doesn't change. So you should be able to rename the enum value here in this file and the corresponding values in SSLErrorCauses in tools/metrics/histograms.xml. 2. Add new enum values for CLOCK_PAST_BY_NETWORK_TIME and CLOCK_FUTURE_BY_NETWORK_TIME and record those too. (I guess you might have to return an enum value or something from IsUserClockAhead() to give you enough information to know which value to record in UMA.) 3. Maybe in a separate CL, we can add additional UMA metrics to answer questions like "How often did we fall back to the build time heuristic?" and "How often did the network time tracker trigger a bad clock interstitial that the build time heuristic wouldn't have caught?" https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:187: bool IsUserClockBehind(const base::Time& now_system, Definition order should match declaration order (it's reversed). https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:187: bool IsUserClockBehind(const base::Time& now_system, Could you add a couple unit tests for these functions? https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.h:32: bool IsUserClockAhead(const base::Time& time_now, My brain might not be doing logic correctly but it looks to me like the comments on IsUserClockAhead() and IsUserClockBehind() are reversed and don't match up with the new names? Also just curious why you decided to change the function names. (I don't really have a preference for the old or the new.) And, I think the comments need to be expanded with a little more detail now that these functions are a bit more complicated. For example, "Returns true if the system time is in the past, as indicated by network time when available or as estimated using a heuristic based on build time, when network time is not available." Maybe something like that? https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.h:32: bool IsUserClockAhead(const base::Time& time_now, Can you name these |now_system|, like you did in the definition?
https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_browser_tests.cc:908: mock_clock->SetNow(base::Time::NowFromSystemTime()); Makes sense, done. https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/20001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:149: const network_time::NetworkTimeTracker* network_time = On 2016/03/08 23:17:36, estark wrote: > nit: |network_time_tracker| instead of |network_time|... when I read > |network_time| I assume it's a base::Time and it surprises me that it's a > NetworkTimeTracker. Moot here; done elsewhere. I wish there were something shorter. https://codereview.chromium.org/1772143002/diff/20001/components/security_int... File components/security_interstitials/core/bad_clock_ui.h (right): https://codereview.chromium.org/1772143002/diff/20001/components/security_int... components/security_interstitials/core/bad_clock_ui.h:30: const network_time::NetworkTimeTracker* network_time, On 2016/03/08 23:17:36, estark wrote: > same nit here: |network_time_tracker| (and elsewhere too) Done. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (left): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: } else if (IsUserClockInTheFuture(base::Time::NowFromSystemTime())) { Why the use of NowFromSystemTime here, BTW, instead of the current_time argument? https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); I like the idea of having an enum to describe what we know about the clock, so, that part is done. But I'm not sure about the rest of it. Consider: today, the heuristic always fires if the system clock is outside the bound (build-2d, build+365d). Whereas now, if network time is available, which will be true for many users, the heuristic *never* fires. To preserve this meaning exactly, we must either (a) sometimes record two metrics to UMA when network time is available, or (b) reverse the precedence such that network time is used only within the bound (build-2d, build+365d), about which today's heuristic has nothing to say. (b) is temporary at best; (a) is wasteful but otherwise inoffensive. But I first want to ask what is so bad about changing the existing metrics? CLOCK_PAST still means the same thing as before; we're simply improving its accuracy. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:187: bool IsUserClockBehind(const base::Time& now_system, On 2016/03/08 23:17:36, estark wrote: > Could you add a couple unit tests for these functions? Done. I couldn't figure out how to run error_classification_unittest, so I added it to components_unittest. Was that the right thing? https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:187: bool IsUserClockBehind(const base::Time& now_system, On 2016/03/08 23:17:36, estark wrote: > Definition order should match declaration order (it's reversed). Sort of moot if you mean just these two functions, but if you want I can move GetClockState's definition down. It's a clumsier diff, tho. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.h:32: bool IsUserClockAhead(const base::Time& time_now, I changed it to be shorter, but I've changed it back, since apparently I managed to confuse myself with the new names. Hopefully the comments on the new enum speak to what you are looking for. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.h:32: bool IsUserClockAhead(const base::Time& time_now, On 2016/03/08 23:17:36, estark wrote: > Can you name these |now_system|, like you did in the definition? Done.
https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (left): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: } else if (IsUserClockInTheFuture(base::Time::NowFromSystemTime())) { On 2016/03/09 23:35:28, mab wrote: > Why the use of NowFromSystemTime here, BTW, instead of the current_time > argument? Hmm... good question. Probably an oversight? I'd say fix it in a separate CL, though (or I can do it if you prefer), and have maybe felt@ and palmer@ look at it to make sure it's not deliberate for some reason. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); On 2016/03/09 23:35:28, mab wrote: > I like the idea of having an enum to describe what we know about the clock, so, > that part is done. > > But I'm not sure about the rest of it. Consider: today, the heuristic always > fires if the system clock is outside the bound (build-2d, build+365d). Whereas > now, if network time is available, which will be true for many users, the > heuristic *never* fires. > > To preserve this meaning exactly, we must either (a) sometimes record two > metrics to UMA when network time is available, or (b) reverse the precedence > such that network time is used only within the bound (build-2d, build+365d), > about which today's heuristic has nothing to say. > > (b) is temporary at best; (a) is wasteful but otherwise inoffensive. But I > first want to ask what is so bad about changing the existing metrics? > CLOCK_PAST still means the same thing as before; we're simply improving its > accuracy. I had (a) in mind, which is wasteful as you said but not horribly so. (And it's not really wasteful if we get value out of it.) My opinion about not wanting to change the existing metrics is based on two things: 1.) We get a useful measure of impact by leaving the existing metric as it is and adding a new metric that includes network time. We'll be able to say "the build time heuristic caught X bad clocks, and we showed Y bad clock interstitials using a combination of build time + network time, so network time caught an additional Y-X bad clocks that we wouldn't have caught otherwise." 2.) It just sounds annoying and error-prone to use the same histogram value to measure something in a very different way. Someone without much context will look at the histogram and be very confused as to why there was a giant increase in bad clocks. At the very least, if we do change the meaning of the existing value, I think we should update the description in histograms.xml to note that the heuristic for detecting bad clocks changed in Jan 16. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:187: bool IsUserClockBehind(const base::Time& now_system, On 2016/03/09 23:35:28, mab wrote: > On 2016/03/08 23:17:36, estark wrote: > > Could you add a couple unit tests for these functions? > > Done. I couldn't figure out how to run error_classification_unittest, so I > added it to components_unittest. Was that the right thing? Urgh, I guess that means error_classification_unittest was never getting built or run before now...? But yes I think that's right. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:187: bool IsUserClockBehind(const base::Time& now_system, On 2016/03/09 23:35:28, mab wrote: > On 2016/03/08 23:17:36, estark wrote: > > Definition order should match declaration order (it's reversed). > > Sort of moot if you mean just these two functions, but if you want I can move > GetClockState's definition down. It's a clumsier diff, tho. I just meant these two functions, so this is fine. https://codereview.chromium.org/1772143002/diff/40001/components/security_int... File components/security_interstitials/core/bad_clock_ui.cc (right): https://codereview.chromium.org/1772143002/diff/40001/components/security_int... components/security_interstitials/core/bad_clock_ui.cc:20: const GURL& request_url, int cert_error, const net::SSLInfo& ssl_info, This formatting looks a little strange to me. Do you know about `git cl format`, and if yes, is this what `git cl format` did? If no, try `git cl format` :) https://codereview.chromium.org/1772143002/diff/40001/components/security_int... components/security_interstitials/core/bad_clock_ui.cc:72: heading_string = IDS_CLOCK_ERROR_BEHIND_HEADING; Eh? NETWORK_OK and UNKNOWN => CLOCK_ERROR_BEHIND? Should this be an ASSERT_UNREACHED() for NETWORK_OK and UNKNOWN, perhaps? (Btw, if you don't know about chrome://interstitials, it lets you preview interstitials without having to do gymnastics to trigger them.) https://codereview.chromium.org/1772143002/diff/40001/components/ssl_errors/e... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/1772143002/diff/40001/components/ssl_errors/e... components/ssl_errors/error_classification.h:36: UNKNOWN, The usual Chromium style would be to name these CLOCK_STATE_UNKNOWN, CLOCK_STATE_NETWORK_OK, etc. Even though it's a little longer, it helps with readability (ssl_errors::UNKNOWN is ambiguous, for example). https://codereview.chromium.org/1772143002/diff/40001/components/ssl_errors/e... components/ssl_errors/error_classification.h:54: BUILD_FUTURE nit: add trailing comma
https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (left): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: } else if (IsUserClockInTheFuture(base::Time::NowFromSystemTime())) { On 2016/03/10 20:10:08, estark wrote: > On 2016/03/09 23:35:28, mab wrote: > > Why the use of NowFromSystemTime here, BTW, instead of the current_time > > argument? > > Hmm... good question. Probably an oversight? I'd say fix it in a separate CL, > though (or I can do it if you prefer), and have maybe felt@ and palmer@ look at > it to make sure it's not deliberate for some reason. Done. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); For background, the calling sequence is like this: IsErrorDueToBadClock → ShowBadClockInterstitial → RecordUMAStatistics. |RecordUMAStatistics| is passed enough bits of state to repeat the time analysis, i.e. rather than simply taking it as a given that the clock is off, it calls |IsClockInThePast| again. I'd argue that's poor separation of concerns: the decision about whether to show the interstitial has already been made. |RecordUMAStatistics| should simply record it. So why am I bringing this up now? Because, if we want to record the verdicts of the build-time heuristic and the network clock separately, we need to account for the possibility that they might disagree! For example, network time might say that the system clock is just fine, in which case we should not call ShowBadClockInterstitial. But if the build-time heuristic disagrees, the CLOCK_PAST metric cannot be recorded, because we only record metrics if we show the interstitial. In short, if we give precedence to network time, we can't help but change the meaning of an existing metric. To your points, I think (1) seems true but not weighty. If network time has a significant impact, I would think we could just eyeball it. To (2) I'd say, yes, let's simply document that our ability to detect bad clocks has improved over time. I would even argue that in the long term, we would rather have one metric than two. Let me know if I'm convincing you. I did try to implement separation between the two methods of assessing the accuracy of the clock, but I found having two bits of clock state was painful to reason about. https://codereview.chromium.org/1772143002/diff/40001/components/security_int... File components/security_interstitials/core/bad_clock_ui.cc (right): https://codereview.chromium.org/1772143002/diff/40001/components/security_int... components/security_interstitials/core/bad_clock_ui.cc:20: const GURL& request_url, int cert_error, const net::SSLInfo& ssl_info, Neato. Done. https://codereview.chromium.org/1772143002/diff/40001/components/security_int... components/security_interstitials/core/bad_clock_ui.cc:72: heading_string = IDS_CLOCK_ERROR_BEHIND_HEADING; It seems ever so slightly unsafe, but, done. https://codereview.chromium.org/1772143002/diff/40001/components/ssl_errors/e... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/1772143002/diff/40001/components/ssl_errors/e... components/ssl_errors/error_classification.h:36: UNKNOWN, On 2016/03/10 20:10:08, estark wrote: > The usual Chromium style would be to name these CLOCK_STATE_UNKNOWN, > CLOCK_STATE_NETWORK_OK, etc. Even though it's a little longer, it helps with > readability (ssl_errors::UNKNOWN is ambiguous, for example). Done. https://codereview.chromium.org/1772143002/diff/40001/components/ssl_errors/e... components/ssl_errors/error_classification.h:54: BUILD_FUTURE On 2016/03/10 20:10:08, estark wrote: > nit: add trailing comma Done.
https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); Oh, I should add: if you buy my argument, I think there is no point to distinguishing between CLOCK_STATE_NETWORK_PAST and CLOCK_STATE_BUILD_PAST, so I would collapse the enum a bit.
If you don't mind, I'm just going to add felt@ to this to weigh in on the metrics. I think I agree with you now that we should just bite the bullet and document the change that we're making, but since she touched this code most recently, and probably also has used this UMA data the most, I just want to make sure she's okay with it first. https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); On 2016/03/11 04:18:41, mab wrote: > For background, the calling sequence is like this: > > IsErrorDueToBadClock → ShowBadClockInterstitial → RecordUMAStatistics. > > |RecordUMAStatistics| is passed enough bits of state to repeat the time > analysis, i.e. rather than simply taking it as a given that the clock is off, it > calls |IsClockInThePast| again. > > I'd argue that's poor separation of concerns: the decision about whether to show > the interstitial has already been made. |RecordUMAStatistics| should simply > record it. > > So why am I bringing this up now? Because, if we want to record the verdicts of > the build-time heuristic and the network clock separately, we need to account > for the possibility that they might disagree! > > For example, network time might say that the system clock is just fine, in which > case we should not call ShowBadClockInterstitial. But if the build-time > heuristic disagrees, the CLOCK_PAST metric cannot be recorded, because we only > record metrics if we show the interstitial. > > In short, if we give precedence to network time, we can't help but change the > meaning of an existing metric. > > To your points, I think (1) seems true but not weighty. If network time has a > significant impact, I would think we could just eyeball it. > > To (2) I'd say, yes, let's simply document that our ability to detect bad clocks > has improved over time. I would even argue that in the long term, we would > rather have one metric than two. > > Let me know if I'm convincing you. I did try to implement separation between > the two methods of assessing the accuracy of the clock, but I found having two > bits of clock state was painful to reason about. Oh, drat! I totally forgot that this is called only after the interstitial is shown. So, yeah, you're right, it would take some pretty weird gymnastics to preserve the existing metric, so I suppose we should just update the description in histograms.xml. (And collapse the enum... or maybe even go back to just a bool? Though I suppose we might want to use the enum to record additional metrics in a follow-up CL.) https://codereview.chromium.org/1772143002/diff/60001/components/security_int... File components/security_interstitials/core/bad_clock_ui.cc (right): https://codereview.chromium.org/1772143002/diff/60001/components/security_int... components/security_interstitials/core/bad_clock_ui.cc:78: heading_string = 0; It's against chromium style to handle NOTREACHED() failure (https://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...). https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: // TODO(mab): Why doesn't this just use |current_time|? Can you please file a bug at include the URL (https://crbug.com/XXXXXX) here? https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:207: if (now_system < now_network - uncertainty - kNetworkTimeFudge) { nit: other code is this file doesn't use curly braces for single-line bodies so I'd remove these. https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:210: if (now_system > now_network + uncertainty + kNetworkTimeFudge) { same nit about the curly braces https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:219: if (now_system < build_time - base::TimeDelta::FromDays(2)) { ditto https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:222: if (now_system > build_time + base::TimeDelta::FromDays(365)) { ditto
estark@chromium.org changed reviewers: + felt@chromium.org
felt: Matt's modifying the bad clock interstitial code to use network time when available, and we ran into a question about what to do with the pre-existing interstitial.ssl.cause.overridable UMA metric for CLOCK_PAST and CLOCK_FUTURE. We expect that we'll see a big increase in these values when we launch network time, which might be a bit confusing to someone looking at the graphs, but we think it's probably okay as long as we document in histograms.xml that this change happened and when. Another option might be to deprecate the existing CLOCK_PAST/CLOCK_FUTURE values and introduce new ones to make the change more clear. Any attempt to exactly preserve the existing meaning of these metrics is sorta impossible, as Matt pointed out, because of the way the code is structured and the fact that we want to have network time (if available) take precedence over the build time heuristic when deciding whether to show the interstitial. Do you have any thoughts or preferences?
https://codereview.chromium.org/1772143002/diff/60001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:149: switch (ssl_errors::GetClockState( mab: another question, unrelated to the metrics stuff. We had talked IRL about the possibility of only showing the bad clock interstitial when the certificate chain would be considered valid if validated with network time, right? I seem to remember that the outcome of that discussion is that we thought it might be nontrivial and/or error-prone to try to do that... is that right? (e.g. we'd have to walk up the chain and validate every cert with the network time; plus the underlying certificate validation code might not actually give us *all* the errors, etc.) Does that match your memory?
https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/20001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:134: RecordSSLInterstitialCause(overridable, CLOCK_PAST); Updated histograms.xml. I've kept the enum, because network time adds the OK value, and I think it's better to have just one function than a function for the past and a function for the future. https://codereview.chromium.org/1772143002/diff/60001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:149: switch (ssl_errors::GetClockState( My memory matches yours. https://codereview.chromium.org/1772143002/diff/60001/components/security_int... File components/security_interstitials/core/bad_clock_ui.cc (right): https://codereview.chromium.org/1772143002/diff/60001/components/security_int... components/security_interstitials/core/bad_clock_ui.cc:78: heading_string = 0; On 2016/03/11 22:00:05, estark wrote: > It's against chromium style to handle NOTREACHED() failure > (https://dev.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...). Done. https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: // TODO(mab): Why doesn't this just use |current_time|? Before I do that, let's ask felt, since she's on this review now. (Or did you specifically want to not change this behavior as part of this CL?) https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:207: if (now_system < now_network - uncertainty - kNetworkTimeFudge) { I kinda think this is how "goto fail" happened, but done. https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:210: if (now_system > now_network + uncertainty + kNetworkTimeFudge) { On 2016/03/11 22:00:05, estark wrote: > same nit about the curly braces Done. https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:219: if (now_system < build_time - base::TimeDelta::FromDays(2)) { On 2016/03/11 22:00:05, estark wrote: > ditto Done. https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:222: if (now_system > build_time + base::TimeDelta::FromDays(365)) { On 2016/03/11 22:00:06, estark wrote: > ditto Done.
https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: // TODO(mab): Why doesn't this just use |current_time|? On 2016/03/11 23:12:50, mab wrote: > Before I do that, let's ask felt, since she's on this review now. (Or did you > specifically want to not change this behavior as part of this CL?) Yeah, that's fine with me. https://codereview.chromium.org/1772143002/diff/60001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:207: if (now_system < now_network - uncertainty - kNetworkTimeFudge) { On 2016/03/11 23:12:50, mab wrote: > I kinda think this is how "goto fail" happened, but done. I know, I hate it too. But, when in Rome...
https://codereview.chromium.org/1772143002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:151: case ssl_errors::CLOCK_STATE_FUTURE: can you add a comment to say that this intentionally falls through? switch statements that fall through give me a little bit of anxiety https://codereview.chromium.org/1772143002/diff/80001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/80001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: // TODO(mab): Why doesn't this just use |current_time|? I'm pretty sure that was a refactoring accident. This should use |current_time|. https://codereview.chromium.org/1772143002/diff/80001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:200: const network_time::NetworkTimeTracker* network_time_tracker) { I was wondering why you're passing around the NTT instead of just the time. It's because you also need the uncertainty estimate? What is the range for the uncertainty estimate?
On 2016/03/11 22:08:23, estark wrote: > felt: Matt's modifying the bad clock interstitial code to use network time when > available, and we ran into a question about what to do with the pre-existing > interstitial.ssl.cause.overridable UMA metric for CLOCK_PAST and CLOCK_FUTURE. > We expect that we'll see a big increase in these values when we launch network > time, which might be a bit confusing to someone looking at the graphs, but we > think it's probably okay as long as we document in histograms.xml that this > change happened and when. > > Another option might be to deprecate the existing CLOCK_PAST/CLOCK_FUTURE values > and introduce new ones to make the change more clear. > > Any attempt to exactly preserve the existing meaning of these metrics is sorta > impossible, as Matt pointed out, because of the way the code is structured and > the fact that we want to have network time (if available) take precedence over > the build time heuristic when deciding whether to show the interstitial. > > Do you have any thoughts or preferences? Considering we are the only people who look at the graphs, I think it's OK. I actually prefer the idea of using the same histogram so we can point to it at the release time and say "look how it jumped on the timeline!" It would be nice however to be able to measure how many were caught by the NTT tracker that wouldn't have been caught by the old heuristic. (You'll probably want this in a future perf packet.) Could you calculate both, histogram the results in a new histogram, and then only use the more precise one if available?
https://codereview.chromium.org/1772143002/diff/80001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/80001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler.cc:151: case ssl_errors::CLOCK_STATE_FUTURE: I'll just make it return. https://codereview.chromium.org/1772143002/diff/80001/components/ssl_errors/e... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/80001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:133: // TODO(mab): Why doesn't this just use |current_time|? Done. https://codereview.chromium.org/1772143002/diff/80001/components/ssl_errors/e... components/ssl_errors/error_classification.cc:200: const network_time::NetworkTimeTracker* network_time_tracker) { It might be O(minutes) in extreme cases. I like being able to use it in the calculation, but I don't think it's critical. Also, if we have two base::Time arguments (or 3, if we pass in the build time as well) that improves the odds of getting them confused. These aren't strong reasons. Happy to pass in the time if you prefer it.
Agree it would be nice to have this number, but measurement by eye is probably good enough, yes? I'm afraid I don't understand most of your last sentence. Some of the difficulties with applying both network time and the build-time heuristic are discussed upthread. In short, it adds complexity that I don't think is outweighed by the benefit it would bring.
On 2016/03/11 23:41:00, mab wrote: > Agree it would be nice to have this number, but measurement by eye is probably > good enough, yes? > > I'm afraid I don't understand most of your last sentence. Some of the > difficulties with applying both network time and the build-time heuristic are > discussed upthread. In short, it adds complexity that I don't think is > outweighed by the benefit it would bring. I don't think I understand the complexity argument (although I did see it). I'm suggesting two new histograms that only record the verdicts. histogram 1: network time tracker CLOCK PAST CLOCK FUTURE CLOCK OK UNABLE TO USE NTT histogram 2: build time heuristic CLOCK PAST CLOCK FUTURE CLOCK OK then we just look at the %ages in aggregate. both would be recorded in GetClockState and neither would be part of the more general interstitial display histograms.
Ah, if it is OK to record metrics from GetClockState, this does indeed get a lot simpler. (Note that GetClockState is called twice for each interstitial: once to decide whether to display the interstitial, and then again from RecordUMAStatistics. I don't know if that's a problem. The second call could potentially be refactored out.) Is adding the additional histograms as simple as modifying histograms.xml, or is there additional work required---say, privacy review?
On 2016/03/11 23:59:06, mab wrote: > Ah, if it is OK to record metrics from GetClockState, this does indeed get a lot > simpler. (Note that GetClockState is called twice for each interstitial: once > to decide whether to display the interstitial, and then again from > RecordUMAStatistics. I don't know if that's a problem. The second call could > potentially be refactored out.) > > Is adding the additional histograms as simple as modifying histograms.xml, or is > there additional work required---say, privacy review? Can we record the new histograms in IsErrorDueToBadClock(), which only gets called once when deciding to show the interstitial, instead of GetClockState()? To add a new histogram, you can just modify histograms.xml and get a histograms.xml owner to approve the CL.
I don't want to leak implementation details, so I've just added a flag to |GetClockState| to indicate whether to record statistics. My preferred way to clean this up would be, eventually, to eliminate all calls where this flag is false. What's the privacy story for these metrics? The new metrics always fire when we see CERT_DATE_INVALID. Is there a process for approving them, or are they covered under an existing approval?
https://codereview.chromium.org/1772143002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:151: true /* record statistics */)) { Sorry, I promise this'll be the last annoying question I ask about this. I'm wondering if you thought about returning the ClockState from this function instead of a boolean, and then pass the ClockState around in place of the NetworkTimeTracker. (So pass the ClockState to the BadClockBlockingPage constructor, which passes it to the BadClockUI, which passes it to RecordUMAStatistics and uses it to select the |heading_string|.) Seems like that would remove the need for the boolean argument, and also save the extra work of calculating the clock state multiple times, but I might be missing something. Or is that the cleanup you wanted to do in a follow-up CL later? https://codereview.chromium.org/1772143002/diff/120001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.cc:228: network_state, CLOCK_STATE_FUTURE + 1); Normal style is to add a CLOCK_STATE_MAX value at the end of the enum and use that.
https://codereview.chromium.org/1772143002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:151: true /* record statistics */)) { Not at all; I should have done it this way to begin with. One thing that seems like a difficulty is SSLErrorUI, which calls RecordUMAStatistics, which necessitates a ClockState argument. In bad_clock_ui.cc there is this: // TODO(felt): Separate the clock statistics from the main ssl statistics. I take it that the idea is to have two versions of |RecordUMAStatistics|, one called from ssl_error_ui.cc and the other called from bad_clock_ui.cc. Does that sound right? I think it would make this CL cleaner if I just went ahead and did that. https://codereview.chromium.org/1772143002/diff/120001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/120001/components/ssl_errors/... components/ssl_errors/error_classification.cc:228: network_state, CLOCK_STATE_FUTURE + 1); On 2016/03/12 07:54:53, estark wrote: > Normal style is to add a CLOCK_STATE_MAX value at the end of the enum and use > that. Done.
https://codereview.chromium.org/1772143002/diff/120001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_error_handler.cc (right): https://codereview.chromium.org/1772143002/diff/120001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_error_handler.cc:151: true /* record statistics */)) { On 2016/03/14 21:25:25, mab wrote: > Not at all; I should have done it this way to begin with. > > One thing that seems like a difficulty is SSLErrorUI, which calls > RecordUMAStatistics, which necessitates a ClockState argument. > > In bad_clock_ui.cc there is this: > > // TODO(felt): Separate the clock statistics from the main ssl statistics. > > I take it that the idea is to have two versions of |RecordUMAStatistics|, one > called from ssl_error_ui.cc and the other called from bad_clock_ui.cc. > > Does that sound right? I think it would make this CL cleaner if I just went > ahead and did that. That sounds reasonable. I think we'd still want to record EXPIRED_RECENTLY on non-bad-clock CERT_DATE_INVALID errors, though, so we should still pass the system time into SSLErrorUI to use in RecordUMAStatistics, I suppose?
Yep. Done.
mab@google.com changed reviewers: + jochen@chromium.org
mab@google.com changed reviewers: + asvitkine@google.com
+asvitkine for histograms.xml +jochen for components/BUILD.gn and chrome/browser/ui/webui/interstitials/interstitial_ui.cc
lgtm with a couple nits inline, also can you please update CL description to add a little more context? Thanks! https://codereview.chromium.org/1772143002/diff/180001/chrome/browser/ssl/bad... File chrome/browser/ssl/bad_clock_blocking_page.cc (right): https://codereview.chromium.org/1772143002/diff/180001/chrome/browser/ssl/bad... chrome/browser/ssl/bad_clock_blocking_page.cc:22: #include "components/ssl_errors/error_classification.h" this #include isn't needed because you have it in the .h, right? https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.cc:183: void RecordUMAStatisticsForClockInterstitial(bool overridable, Note for future: it seems inevitable that we'll add a new metric in RecordUMAStatistics() and forget to add it here as well. Maybe (in a future CL) we should separate out all the metrics that aren't conditioned on the error (just ssl_error_type and connection_type at the moment) into a helper function or something. https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:49: // |CLOCK_STATE| enum. A result from network time, if available, will always be Is something missing from this comment accidentally? Did you mean "Returns a |CLOCK_STATE| enum"?
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
lgtm
Updated change description. https://codereview.chromium.org/1772143002/diff/180001/chrome/browser/ssl/bad... File chrome/browser/ssl/bad_clock_blocking_page.cc (right): https://codereview.chromium.org/1772143002/diff/180001/chrome/browser/ssl/bad... chrome/browser/ssl/bad_clock_blocking_page.cc:22: #include "components/ssl_errors/error_classification.h" On 2016/03/16 00:51:36, estark wrote: > this #include isn't needed because you have it in the .h, right? Done. https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... File components/ssl_errors/error_classification.cc (right): https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.cc:183: void RecordUMAStatisticsForClockInterstitial(bool overridable, Eh, why don't I just get it done. How does this look? https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... File components/ssl_errors/error_classification.h (right): https://codereview.chromium.org/1772143002/diff/180001/components/ssl_errors/... components/ssl_errors/error_classification.h:49: // |CLOCK_STATE| enum. A result from network time, if available, will always be Fixed, thanks.
mab@google.com changed reviewers: - asvitkine@google.com
Description was changed from ========== Use network time for bad clock interstitial. This implementation could be improved, but I suspect it is perfectly sufficient. BUG=589700 ========== to ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The meaning of the |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 ==========
Description was changed from ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The meaning of the |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 ========== to ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 ==========
mab@google.com changed reviewers: + caitkp@chromium.org, dbeam@chromium.org - jochen@chromium.org
Trying to make reviewers more sane ... dbeam can you please review chrome/browser/ui/webui/interstitials/interstitial_ui.cc? caitkp can you please review components/BUILD.gn? Thanks.
lgtm
lgtm
The CQ bit was checked by mab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/1772143002/#ps200001 (title: "estark review 10")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772143002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772143002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by mab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from estark@chromium.org, asvitkine@chromium.org, dbeam@chromium.org, caitkp@chromium.org Link to the patchset: https://codereview.chromium.org/1772143002/#ps220001 (title: "pass "gn check out/Default"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1772143002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1772143002/220001
Message was sent while issue was closed.
Description was changed from ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 ========== to ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 ========== to ========== Use network time for bad clock interstitial. This change supplements the existing build-time heuristic (which renders the bad-clock interstitial only when the system clock is outside the range (build-2d,build+365d)) with a comparison of the system clock to network time, fetched from |NetworkTimeTracker|. Network time has priority: if the network time says the clock is accurate, the bad-clock intersitial will not be used. The |CLOCK_FUTURE| and |CLOCK_PAST| metrics are recorded only when the interstitial is shown, however the decision was made. The new histograms |interstitial.ssl.clockstate.build_time| and |interstitial.ssl.clockstate.network| provide data about the state of the system clock whenever a |CERT_DATE_INVALID| error is encountered. This should help us to assess the effects of the change. Future work will focus on providing better network time in more cases. BUG=589700 Committed: https://crrev.com/740b02b74735910ec8e185d6375d4eda3f15e0ba Cr-Commit-Position: refs/heads/master@{#381837} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/740b02b74735910ec8e185d6375d4eda3f15e0ba Cr-Commit-Position: refs/heads/master@{#381837} |
