Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1835823002:
network_time_tracker: add temporary time protocol. (Closed)
|
Created:
4 years, 8 months ago by mab Modified:
4 years, 7 months ago Reviewers:
*waffles, sky, Ryan Sleevi, *estark, Robert Sesek, Nicolas Zea, mmenke, rohitrao (ping after 24h) 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. |
Descriptionnetwork_time_tracker: add temporary time protocol.
This adds queries of the temporary secure time server, with signature
verification, and a very conservative scheduler. (Checks once per hour,
queries only if sync believed to be lost.)
BUG=589700
Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4
Cr-Commit-Position: refs/heads/master@{#392208}
Committed: https://crrev.com/2f07cb9a5744a021e3d7c34e9c5de5ef6d246f27
Cr-Commit-Position: refs/heads/master@{#392711}
Patch Set 1 #Patch Set 2 : fix build? #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : msvc #
Total comments: 22
Patch Set 6 : estark review 1 #
Total comments: 4
Patch Set 7 : estark review 2 #Patch Set 8 : Use real URL and public key. Add iOS. #Patch Set 9 : rebase #Patch Set 10 : a wild dependency check appears! #
Total comments: 11
Patch Set 11 : zea review 1 #Patch Set 12 : rebase (note: scoped_ptr -> std::unique_ptr) #
Total comments: 1
Patch Set 13 : zea review 2, and fix test #
Total comments: 49
Patch Set 14 : rsleevi review 1 #
Total comments: 8
Patch Set 15 : comments from rsesek, mmenke, rsleevi #Patch Set 16 : rebase #Patch Set 17 : scoped_refptr #
Total comments: 17
Patch Set 18 : rsleevi comments #Patch Set 19 : rsleevi comments #Patch Set 20 : move header #Patch Set 21 : and iOS #
Total comments: 22
Patch Set 22 : mmenke comments #Patch Set 23 : add test of signed bad data #
Total comments: 8
Patch Set 24 : mmenke comments #Patch Set 25 : cleanup #Patch Set 26 : patch estark's 1925793002 so this is Finch-disabled on commit #Patch Set 27 : finch test fix #Patch Set 28 : rebase #Patch Set 29 : fix memory leak #Patch Set 30 : lint #Messages
Total messages: 69 (18 generated)
Description was changed from ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 ========== to ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 ==========
mab@google.com changed reviewers: + estark@chromium.org
https://codereview.chromium.org/1835823002/diff/80001/components/network_time... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:127: if (GetNetworkTime(&network_time, nullptr)) { suggested comment: // If GetNetworkTime() returns true, the NetworkTimeTracker thinks // it is in sync, so there is no need to query. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:148: net::LOAD_DO_NOT_SAVE_COOKIES); might also want LOAD_BYPASS_CACHE and, for extra paranoia, LOAD_DO_NOT_SEND_AUTH_DATA (I'm actually not sure if BYPASS_CACHE is redundant with DISABLE_CACHE, but I see other examples of them being used together, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/timezone/...) https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:153: static std::string GetServerETag(const net::URLFetcher* source) { This should go in an anonymous namespace at the top of the file. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:154: const auto response_headers(source->GetResponseHeaders()); personal style nit: I'm not a huge fan of auto here, I think it decreases readability https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:175: CHECK(source->GetResponseAsString(&response_body)); CHECK seems heavy-handed here. Can we just log and return like we do for a non-200 response? https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:178: VLOG(1) << "invalid signature"; missing return here, I think? https://codereview.chromium.org/1835823002/diff/80001/components/network_time... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.h:87: // Called to process responses from the secure time service. Preface this section with // net::URLFetcherDelegate: https://codereview.chromium.org/1835823002/diff/80001/components/network_time... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:20: struct NetworkTimeTrackerPeer { I haven't seen this pattern before. FRIEND_TEST_ALL_PREFIXES is more common in Chromium, though maybe you're trying to avoid enumerating all the tests that need to be friended? (If you end up sticking with this pattern, maybe name it NetworkTimeTrackerPeerForTesting or something to indicate that it's designed for testing; I was confused about this when reading the header file.) https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:315: EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL)); nullptr instead of NULL https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:319: EXPECT_NE(nullptr, fetcher); ASSERT_NE (if this fails, the test should be aborted) https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:362: EXPECT_NE(nullptr, fetcher); ASSERT_NE
mab@google.com changed reviewers: + waffles@chromium.org
mab@google.com changed required reviewers: + estark@chromium.org, waffles@chromium.org
Thanks! +waffles for CUP changes and server interactions. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:127: if (GetNetworkTime(&network_time, nullptr)) { On 2016/03/28 17:35:09, estark wrote: > suggested comment: > > // If GetNetworkTime() returns true, the NetworkTimeTracker thinks > // it is in sync, so there is no need to query. Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:148: net::LOAD_DO_NOT_SAVE_COOKIES); On 2016/03/28 17:35:09, estark wrote: > might also want LOAD_BYPASS_CACHE and, for extra paranoia, > LOAD_DO_NOT_SEND_AUTH_DATA > > (I'm actually not sure if BYPASS_CACHE is redundant with DISABLE_CACHE, but I > see other examples of them being used together, e.g. > https://code.google.com/p/chromium/codesearch#chromium/src/chromeos/timezone/...) Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:153: static std::string GetServerETag(const net::URLFetcher* source) { On 2016/03/28 17:35:09, estark wrote: > This should go in an anonymous namespace at the top of the file. Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:154: const auto response_headers(source->GetResponseHeaders()); On 2016/03/28 17:35:09, estark wrote: > personal style nit: I'm not a huge fan of auto here, I think it decreases > readability Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:175: CHECK(source->GetResponseAsString(&response_body)); Quite right, done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.cc:178: VLOG(1) << "invalid signature"; Done, sorry. I've been cheating a bunch since we're still working out the keys. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker.h:87: // Called to process responses from the secure time service. On 2016/03/28 17:35:10, estark wrote: > Preface this section with > > // net::URLFetcherDelegate: Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:20: struct NetworkTimeTrackerPeer { No, no, I just wasn't aware of the standard pattern. Thanks. Fixed. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:315: EXPECT_FALSE(tracker_->GetNetworkTime(&out_network_time, NULL)); On 2016/03/28 17:35:10, estark wrote: > nullptr instead of NULL Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:319: EXPECT_NE(nullptr, fetcher); On 2016/03/28 17:35:10, estark wrote: > ASSERT_NE (if this fails, the test should be aborted) Done. https://codereview.chromium.org/1835823002/diff/80001/components/network_time... components/network_time/network_time_tracker_unittest.cc:362: EXPECT_NE(nullptr, fetcher); On 2016/03/28 17:35:10, estark wrote: > ASSERT_NE Done.
Thanks, lgtm. https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... components/network_time/network_time_tracker.cc:93: pref_service_(pref_service) { Initialize |query_signer_| and |time_fetcher_| to nullptr here to be safe? https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... components/network_time/network_time_tracker.h:31: } nit: these should all end with } // namespace net
https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... components/network_time/network_time_tracker.cc:93: pref_service_(pref_service) { Whoa. Surprised that's necessary for scoped_ptr. Done. https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/100001/components/network_tim... components/network_time/network_time_tracker.h:31: } On 2016/03/28 23:18:59, estark wrote: > nit: these should all end with > > } // namespace net Done.
@waffles: can you have a look at the CUP-related bits? Thanks.
lgtm https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:71: return response_headers->EnumerateHeader(nullptr, "x-cup-server-proof", Hm, filed crbug/606958 for this, but this is fine to check in as-is. https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:221: base::TimeTicks post_time) { IIRC, this is called by a number of services, do we trust those services to call it correctly? (i.e. do we trust HTTPS?)
mab@google.com changed reviewers: + rohitrao@chromium.org, sky@chromium.org, zea@chromium.org
+sky can you review browser_process_impl.cc? +rohitrao can you review application_context_impl.cc? +zea can you approve components/network_time? Thanks! https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:221: base::TimeTicks post_time) { On 2016/04/26 21:16:46, waffles wrote: > IIRC, this is called by a number of services, do we trust those services to call > it correctly? (i.e. do we trust HTTPS?) We do, but I also think it makes sense to move away from updates from other sources, if only to make it easier to audit where time can come from.
ios lgtm
Couple nits https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:90: time_fetcher_(nullptr), nit: both time_fetcher_ and query_signer_ are scoped_ptrs, so don't need to be initialized to nullptr (they're default initialized). https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:126: base::TimeDelta period = base::TimeDelta::FromMinutes(60); nit: although this is only used here, might be good to pull this constant out into the anon namespace at the top of file, so they can all be found easily. https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:156: VLOG(1) << "tried to make fetch happen; failed"; Why VLOG vs DVLOG (here and elsewhere)? Unless there's strong reason not to, DVLOG is preferred to avoid executable bloat. https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:209: // happen to know, which its maximum allowable clock skew. It's unlikely that nit: which its -> which is its
zea: PTAL https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:90: time_fetcher_(nullptr), On 2016/04/27 18:58:22, Nicolas Zea wrote: > nit: both time_fetcher_ and query_signer_ are scoped_ptrs, so don't need to be > initialized to nullptr (they're default initialized). Done. https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:126: base::TimeDelta period = base::TimeDelta::FromMinutes(60); On 2016/04/27 18:58:22, Nicolas Zea wrote: > nit: although this is only used here, might be good to pull this constant out > into the anon namespace at the top of file, so they can all be found easily. Done. https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:156: VLOG(1) << "tried to make fetch happen; failed"; On 2016/04/27 18:58:22, Nicolas Zea wrote: > Why VLOG vs DVLOG (here and elsewhere)? Unless there's strong reason not to, > DVLOG is preferred to avoid executable bloat. Done. https://codereview.chromium.org/1835823002/diff/180001/components/network_tim... components/network_time/network_time_tracker.cc:209: // happen to know, which its maximum allowable clock skew. It's unlikely that On 2016/04/27 18:58:22, Nicolas Zea wrote: > nit: which its -> which is its Done.
LGTM https://codereview.chromium.org/1835823002/diff/220001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/220001/components/network_tim... components/network_time/network_time_tracker.h:87: // If syncrhonization has been lost, sends a query to the secure time service. nit: synchronization
browser_process_impl.cc LGTM
mab@google.com changed reviewers: + rsleevi@chromium.org
rsleevi: added you because of this message:
** Presubmit Messages **
Missing LGTM from OWNERS of dependencies added to DEPS:
'+net',
rsleevi@chromium.org changed reviewers: + mmenke@chromium.org, rsesek@chromium.org
rsesek: See one question below about JSON parsing from a trusted (Google) URL in the browser process (your name is flagged) mmenke: Could you double check my review of the URLFetcher side? I believe it's safe, mod the caveat I noted about ResponseAsString and the lifetime issue if the URLRequestContextGetter. Matt's LG should be sufficient for the DEPS addition and as the second check for the URLFetcher. That said, there doesn't appear to be any backoff code here, so it's possible we could end up hammering a server. URLFetcher does not have any backoff code built in, and having a billion Chrome users DoS a Google service could be... bad? Did I miss something when reviewing? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:67: static std::string GetServerProof(const net::URLFetcher* source) { This is in an unnamed namespace; the static is unnecessary https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:70: if (response_headers == nullptr) { if (!response_headers) ? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:124: if (getter != nullptr) { Chromium base/ owners chided me enough, but I believe the preference here is to just use the boolean form, as that also 'transparently' works for other pointer types in Chromium (unique, scoped_refptr, weakptr), without incurring the added .get() call or requiring overloading the operators That is, if (getter) { https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:127: {reinterpret_cast<const char*>(kKeyPubBytes), sizeof(kKeyPubBytes)}); Officially, Uniform Initialization Syntax is not allowed ( https://chromium-cpp.appspot.com/ ), but I think that's just because the CL to update it hasn't landed yet (based on https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f... ) That said, based on the guidance from the aforementioned internal document, I don't believe this usage is consistent with that style, although perhaps I've misunderstood. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:150: query_signer_->SignRequest({"", 0}, &query_string); Same with UIS here - I believe you should just use base::StringPiece() or simply nullptr and let implicit conversion work. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:153: replacements.SetQueryStr(query_string); Is the assumption that kTimeServiceURL can never contain existing query string parameters? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:158: if (time_fetcher_ == nullptr) { if (time_fetcher_) https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:166: net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_BYPASS_CACHE | Perhaps structure the cache and cookie directives adjacent to eachother? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:183: if (!source->GetResponseAsString(&response_body)) { Note: Usually this is discouraged, because it may be an unbounded message size. I defer to Matt here https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:187: DCHECK(query_signer_.get()); I believe you drop the .get(), it should still have the same behaviour by invoking the bool conversion DCHECK(query_signer_) https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); Parsing JSON in the browser process is generally discouraged, although exceptions may exist if the source of the data is 'trusted'. rsesek: You good with this? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:209: // There's also a "server_nonce" key, which we can ignore. Who is the "we" here? https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3... // There's also a "server_nonce" key, but that can be ignored. [natural next question is why] https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:215: // class. // The 10 seconds happens to be a known property of the time server, // which is that is its maximum allowable clock skew. ... Should the magic value '10' be moved up to be associated with the Time Server URL, to indicate the coupling of these two? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:267: query_timer_.Reset(); Why the conditional? Why not explicitly call it all the time? (Context, checking .is_null() on a callback is _usually_ an anti-pattern, since it's largely an implementation detail of the message passing system; but I may have missed something special to your use here, hence the question) https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.h:55: net::URLRequestContextGetter* getter = nullptr); URLRequestContextGetter is designed to be ref-counted and can be safely held on to by clients, to ensure that they can actually get from the getter (if the underlying URLRequestContext goes away, it behaves safely) I believe you should be passing this as a scoped_refptr<net::URLRequestContextGetter>, and storing as such. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:28: void SetUp() override { Please see https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... Based on this code, I believe you should be fine moving it all into the constructor, rather than the SetUp method. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:41: new net::TestURLRequestContextGetter(message_loop_.task_runner()))); This means the TestURLRequestContextGetter is leaked, correct? Since you only pass and store the raw pointer. That's where changing to the scoped_refptr<> will help :) https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:330: net::HttpResponseHeaders* headers = new net::HttpResponseHeaders(""); pedantry: For empty strings, std::string() avoids any dependencies on compiler inlining and behaviours (namely, realizing it's strlen on a constant, that the length is 0, then folding the branch that copies the string if size != 0 while initializing the object) https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:335: "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"); This proof is tied to the key used, right? Is it also tied to the response? I'm just wondering whether it makes sense to put as a constant for easier updating (if the key changes). But perhaps I'm dense.
Re: thundering herd scenarios, there are a few protections here: - Low frequency, no more than once per hour. - Queries stop entirely so long as client's clocks stay synchronized, which should be most clients most of the time. - estark has a Finch control for the feature as a whole that she plans to land right on top of this. That said, I'm never going to complain about adding backoff. Something like, exponential increase in time between queries upon encountering an error? https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:67: static std::string GetServerProof(const net::URLFetcher* source) { On 2016/04/28 01:08:34, Ryan Sleevi wrote: > This is in an unnamed namespace; the static is unnecessary Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:70: if (response_headers == nullptr) { Sorry, didn't know that was preferred style. Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:124: if (getter != nullptr) { On 2016/04/28 01:08:34, Ryan Sleevi wrote: > Chromium base/ owners chided me enough, but I believe the preference here is to > just use the boolean form, as that also 'transparently' works for other pointer > types in Chromium (unique, scoped_refptr, weakptr), without incurring the added > .get() call or requiring overloading the operators > > That is, > if (getter) { Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:127: {reinterpret_cast<const char*>(kKeyPubBytes), sizeof(kKeyPubBytes)}); Rewritten to follow the super-fresh advice at https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:150: query_signer_->SignRequest({"", 0}, &query_string); On 2016/04/28 01:08:34, Ryan Sleevi wrote: > Same with UIS here - I believe you should just use base::StringPiece() or simply > nullptr and let implicit conversion work. Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:153: replacements.SetQueryStr(query_string); I guess so? That's the case now, and I wasn't anticipating having to change it. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:158: if (time_fetcher_ == nullptr) { if (!time_fetcher_) rather, but done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:166: net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_BYPASS_CACHE | On 2016/04/28 01:08:34, Ryan Sleevi wrote: > Perhaps structure the cache and cookie directives adjacent to eachother? Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:183: if (!source->GetResponseAsString(&response_body)) { What's the best alternative? I could check the length before decoding and bail out if it's unreasonably large. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:187: DCHECK(query_signer_.get()); On 2016/04/28 01:08:34, Ryan Sleevi wrote: > I believe you drop the .get(), it should still have the same behaviour by > invoking the bool conversion > > DCHECK(query_signer_) Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:209: // There's also a "server_nonce" key, which we can ignore. Tried to address both. If you ask me "why" again, though, I will have to appeal to authority. :-/ https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:215: // class. On 2016/04/28 01:08:34, Ryan Sleevi wrote: > // The 10 seconds happens to be a known property of the time server, > // which is that is its maximum allowable clock skew. > ... > > > Should the magic value '10' be moved up to be associated with the Time Server > URL, to indicate the coupling of these two? Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:267: query_timer_.Reset(); Lamely, it's for unit tests. (You can't Reset the timer if it was never Started, and you can't even start it when there isn't a task runner.) Changed to test getter_ instead, to make it clearer what's going on (?). https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.h:55: net::URLRequestContextGetter* getter = nullptr); Good catch, thank you. Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:28: void SetUp() override { On 2016/04/28 01:08:34, Ryan Sleevi wrote: > Please see > https://github.com/google/googletest/blob/master/googletest/docs/FAQ.md#shoul... > > Based on this code, I believe you should be fine moving it all into the > constructor, rather than the SetUp method. Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:41: new net::TestURLRequestContextGetter(message_loop_.task_runner()))); I guess leaks are not detected automatically by the test infrastructure. :-/ Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:330: net::HttpResponseHeaders* headers = new net::HttpResponseHeaders(""); On 2016/04/28 01:08:34, Ryan Sleevi wrote: > pedantry: For empty strings, std::string() avoids any dependencies on compiler > inlining and behaviours (namely, realizing it's strlen on a constant, that the > length is 0, then folding the branch that copies the string if size != 0 while > initializing the object) Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:335: "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855"); It is tied to the server's keypair, to the (overridden) request nonce above, and to the response. Honestly I don't think making it a constant is a huge help. My procedure here is pretty much: issue a query; copy the results. But that's worth documenting, so, did that.
https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:183: if (!source->GetResponseAsString(&response_body)) { On 2016/04/28 02:29:04, mab wrote: > What's the best alternative? I could check the length before decoding and bail > out if it's unreasonably large. You can't, actually - by the point OnURLFetchComplete is called, we've already decoded the body, and stored it all in memory. Currently, you can either save to a file (Which URLFetcher supports directly) or provide your own URLFetcherResponseWriter implementation, which caps the length and saves to a string. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:267: query_timer_.Reset(); On 2016/04/28 02:29:03, mab wrote: > Lamely, it's for unit tests. (You can't Reset the timer if it was never > Started, and you can't even start it when there isn't a task runner.) > > Changed to test getter_ instead, to make it clearer what's going on (?). I'd suggest using a real getter in unit tests. net/url_request/url_request_test_util.h has a TestURLRequestContextGetter class that you can use. I'd also suggest using the embedded test server to return both valid responses, and errors. Seems like not testing OnURLFetchComplete is a mistake. Beyond the scope of this CL, though. Happy to talk more about specifics, and/or help out reviewing tests that wire things up. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.h:55: net::URLRequestContextGetter* getter = nullptr); On 2016/04/28 01:08:34, Ryan Sleevi wrote: > URLRequestContextGetter is designed to be ref-counted and can be safely held on > to by clients, to ensure that they can actually get from the getter (if the > underlying URLRequestContext goes away, it behaves safely) > > I believe you should be passing this as a > scoped_refptr<net::URLRequestContextGetter>, and storing as such. Only certainly URLRequestContextGetters are safely shut down - it requires coordination between the URLRequestContextGetter and the URLREquestContext's owner for that to work. Currently, only the Profile's URLRequestContextGetters shutdown properly. The system and proxy URLRequestContexts use another magic path to shut down safely, so I suppose those should be fine, too. Others can only be used safely if you're destroyed before the URLRequestContext itself is. Regardless, this code should definitely be hanging on to a reference, rather than a raw pointer. https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.cc:56: const char kTimeServiceURL[] = "http://clients2.google.com/time/1/current"; I thought there was a plan to move most google domains, including clients.google.com, over to HSTS? I assume you need this request to be non-HTTPS, so are you going to need to find another domain to use here? Also worth noting that using URLFetcher with non-HTTPS domains (And non-google domains) in general seems like not a great idea, since it has no size cap, so a third party can easily OOM the browser. https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.h:56: scoped_refptr<net::URLRequestContextGetter>& getter); Bug: Passing by non-const ref is not allowed. There was some discussion of best form on chromium-dev. I believe the decision was passing refptrs directly, and using std::move when you're going to hold on to a reference is the best option?
https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); On 2016/04/28 01:08:34, Ryan Sleevi wrote: > Parsing JSON in the browser process is generally discouraged, although > exceptions may exist if the source of the data is 'trusted'. > > rsesek: You good with this? How trusted is this data? Fetched from pinned, https, Google connection? We generally prefer to always parse out-of-process, but we don't do so exclusively. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); You can just use the static ::Read if you're not holding onto the reader.
https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); On 2016/04/28 16:51:23, Robert Sesek wrote: > On 2016/04/28 01:08:34, Ryan Sleevi wrote: > > Parsing JSON in the browser process is generally discouraged, although > > exceptions may exist if the source of the data is 'trusted'. > > > > rsesek: You good with this? > > How trusted is this data? Fetched from pinned, https, Google connection? > > We generally prefer to always parse out-of-process, but we don't do so > exclusively. We look to be fetching from an http connection, so not trusted.
https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); On 2016/04/28 16:54:29, mmenke wrote: > On 2016/04/28 16:51:23, Robert Sesek wrote: > > On 2016/04/28 01:08:34, Ryan Sleevi wrote: > > > Parsing JSON in the browser process is generally discouraged, although > > > exceptions may exist if the source of the data is 'trusted'. > > > > > > rsesek: You good with this? > > > > How trusted is this data? Fetched from pinned, https, Google connection? > > > > We generally prefer to always parse out-of-process, but we don't do so > > exclusively. > > We look to be fetching from an http connection, so not trusted. We fetch over HTTP protected by CUP with a pinned ECDSA key, (the same way browser updates do). A MITM would have to compromise the ECDSA signature of the [request, response] pair in order to inject malicious content that passes query_signer->ValidateResponse on :188. This code should continue to work in the presence of a compromised HTTPS stack (e.g. clock is bad).
https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); On 2016/04/28 16:58:57, waffles wrote: > On 2016/04/28 16:54:29, mmenke wrote: > > On 2016/04/28 16:51:23, Robert Sesek wrote: > > > On 2016/04/28 01:08:34, Ryan Sleevi wrote: > > > > Parsing JSON in the browser process is generally discouraged, although > > > > exceptions may exist if the source of the data is 'trusted'. > > > > > > > > rsesek: You good with this? > > > > > > How trusted is this data? Fetched from pinned, https, Google connection? > > > > > > We generally prefer to always parse out-of-process, but we don't do so > > > exclusively. > > > > We look to be fetching from an http connection, so not trusted. > > We fetch over HTTP protected by CUP with a pinned ECDSA key, (the same way > browser updates do). A MITM would have to compromise the ECDSA signature of the > [request, response] pair in order to inject malicious content that passes > query_signer->ValidateResponse on :188. This code should continue to work in the > presence of a compromised HTTPS stack (e.g. clock is bad). OK, then lgtm.
https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:183: if (!source->GetResponseAsString(&response_body)) { OK, I did the latter (= provided a size-limiting implementation of URLFetcherResponseWriter). https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:194: std::unique_ptr<base::Value> value = reader.Read(response_body); On 2016/04/28 16:51:23, Robert Sesek wrote: > You can just use the static ::Read if you're not holding onto the reader. Done. https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.cc:267: query_timer_.Reset(); Done. Solid unit tests are very much in scope for this CL. I've rewritten this to use the EmbeddedTestServer; let me know how it looks. (I'm especially unsure of myself when it comes to thread-safety; still getting a feel for how that works in Chromium.) https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/240001/components/network_tim... components/network_time/network_time_tracker.h:55: net::URLRequestContextGetter* getter = nullptr); Done. https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.cc:56: const char kTimeServiceURL[] = "http://clients2.google.com/time/1/current"; I'll have to defer to waffles on the HSTS question. The choice of HTTP is deliberate here, since we assume the clock may be so badly broken as to prevent HTTPS from working. I added a thing to prevent arbitrarily-sized responses from being buffered. https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.h:56: scoped_refptr<net::URLRequestContextGetter>& getter); I think this is moot. Looking more closely, I see that the pattern here is that a URLRequestContextGetter is an explicitly reference-counted object, & that the pattern seems to be that system_request_context() returns a raw pointer. In short, what seems to be done elsewhere is a plain-pointer argument with scoped_refptr interally. SG?
https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.cc:56: const char kTimeServiceURL[] = "http://clients2.google.com/time/1/current"; mmenke: Please contact me (internally) with any new information you have on HTTP deprecation. As of a year (?) ago Omaha team was in the loop on this and we had the understanding that *secure* HTTP traffic would continue to be permitted. In the short term, I recommend checking in with this URL, since if client2 migrates to HTTPS-only we'll have to migrate a few other things (component updates, etc), and having this here won't add significant complexity to that effort.
I'll plan to review the test next week. https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.cc:56: const char kTimeServiceURL[] = "http://clients2.google.com/time/1/current"; On 2016/04/29 19:51:22, waffles wrote: > mmenke: Please contact me (internally) with any new information you have on HTTP > deprecation. As of a year (?) ago Omaha team was in the loop on this and we had > the understanding that *secure* HTTP traffic would continue to be permitted. > > In the short term, I recommend checking in with this URL, since if client2 > migrates to HTTPS-only we'll have to migrate a few other things (component > updates, etc), and having this here won't add significant complexity to that > effort. You're probably more knowledgeable here than I am. I thought the plan was to migrate to HSTS, with would automatically redirect all HTTP requests, but I could well be mistaken.
https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.h:56: scoped_refptr<net::URLRequestContextGetter>& getter); On 2016/04/29 19:42:07, mab wrote: > In short, what seems to be done elsewhere is a plain-pointer argument with > scoped_refptr interally. SG? If you're going to take a reference, the recommendation is to pass as scoped_refptr<> getter and std::move(). This is both consistent with the semantics of how we pass std::unique_ptr<>, while also consistent with how std::shared_ptr<> works (where you can't take a reference from a naked pointer, due to external refcounting, versus Chromium's internal ref counting) Now that we have std::move(), pass as the object you'll store.
I think I've addressed all outstanding comments, so please let me know if not. https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/260001/components/network_tim... components/network_time/network_time_tracker.h:56: scoped_refptr<net::URLRequestContextGetter>& getter); Fair enough, done.
Double check with mmenke on the URLFetcher changes (I think they're good), but LGTM % nits https://codereview.chromium.org/1835823002/diff/320001/components/client_upda... File components/client_update_protocol/ecdsa.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/client_upda... components/client_update_protocol/ecdsa.h:69: friend class network_time::NetworkTimeTrackerTest; FWIW, this feels like a layering violation (//components/client_update_protocol sets below //components/network_time/, so CUP shouldn't know about network_time, nor should network_time be reaching in to the implementation rather than the interface). The alternative way to do this would be something like OverrideKeyForTesting() or something (as a public method), which our presubmit code would enforce is only used in _unittest files. Whether that's strictly better is subjective - it leaks more of the interface out, such that anyone can use it, but keeps the dependencies better aligned. Just $.02 https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:25: #include "url/gurl.h" This is already included in the .h https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:165: void NetworkTimeTracker::SetTimeServerURL(const GURL& url) { The order of declarations should match the order of definitions. These should be added to the end of the file, not here, and in the order they're declared (SetMaxResponseSize -> SetTimeServerURL -> QueryTimeService -> OnURLFetchComplete -> QueueTimeQuery) https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:218: DCHECK(source == time_fetcher_.get()); DCHECK_EQ ? https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:220: // Back off unless the response was success. "unless the response was a success" isn't handled here. I'm guessing it's being implied in QueueTimeQuery resetting the timer? Is that valid with Timer? Perhaps a little explanation as to the subtlety. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.h:17: #include "net/url_request/url_request_context_getter.h" You forward declare this, no need for the header. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:56: void tearDown() { This naming isn't consistent with Chromium. Was this meant to be TearDown() with an override?
https://codereview.chromium.org/1835823002/diff/320001/components/client_upda... File components/client_update_protocol/ecdsa.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/client_upda... components/client_update_protocol/ecdsa.h:69: friend class network_time::NetworkTimeTrackerTest; I didn't know the |ForTesting| suffix was enforced. That does change the balance. Done. waffles, you OK with this? https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:25: #include "url/gurl.h" On 2016/04/29 22:54:15, Ryan Sleevi wrote: > This is already included in the .h Done. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:165: void NetworkTimeTracker::SetTimeServerURL(const GURL& url) { On 2016/04/29 22:54:15, Ryan Sleevi wrote: > The order of declarations should match the order of definitions. > > These should be added to the end of the file, not here, and in the order they're > declared (SetMaxResponseSize -> SetTimeServerURL -> QueryTimeService -> > OnURLFetchComplete -> QueueTimeQuery) Done. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:218: DCHECK(source == time_fetcher_.get()); On 2016/04/29 22:54:15, Ryan Sleevi wrote: > DCHECK_EQ ? Done. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.cc:220: // Back off unless the response was success. On 2016/04/29 22:54:15, Ryan Sleevi wrote: > "unless the response was a success" isn't handled here. I'm guessing it's being > implied in QueueTimeQuery resetting the timer? Is that valid with Timer? Perhaps > a little explanation as to the subtlety. Done. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.h:17: #include "net/url_request/url_request_context_getter.h" Actually it's the forward declaration that has to go: scoped_refptr requires the definition. https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:56: void tearDown() { ... which I guess means this is unnecessary? :-/
https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.h:17: #include "net/url_request/url_request_context_getter.h" On 2016/04/29 23:30:09, mab wrote: > Actually it's the forward declaration that has to go: scoped_refptr requires the > definition. It doesn't require it in this header, it requires it in the place you instantiate the NetworkTimeTracker. Because your ctor/dtor are out of line, it's fine, but when a callsite attempts to pass a by-value scoped_refptr<>, it needs to know how to handle the dtor for the dependent type. So the forward declaration can stay, but you'd need to make sure to include the url_request_context_getter header in the place you're forwarding it from (e.g. the SystemURLRequestContext), since that code can *also* forward declare when it's returning by-value.
https://codereview.chromium.org/1835823002/diff/320001/components/client_upda... File components/client_update_protocol/ecdsa.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/client_upda... components/client_update_protocol/ecdsa.h:69: friend class network_time::NetworkTimeTrackerTest; On 2016/04/29 23:30:08, mab wrote: > I didn't know the |ForTesting| suffix was enforced. That does change the > balance. Done. > > waffles, you OK with this? I'm OK with it. I'll admit it seems a little bit weird to me.
https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/320001/components/network_tim... components/network_time/network_time_tracker.h:17: #include "net/url_request/url_request_context_getter.h" OK, done.
Set up of the context and test server looks good to me, though I do have some other suggestions. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.cc:209: query_timer_.Reset(); This seems really unexpected. Suggest moving it to the end of OnURLFetchComplete, at least. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.cc:298: // the standard interval. Since you always set the timer delay, any reason not to make this a OneshotTimer? Are you concerned about the URLFetcher hanging? If so, that seems a valid concern, but then you probably want to test that case, and document that behavior where you declare the timer. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.cc:310: time_fetcher_.reset(); optional: May want to extract most of this function to a subroutine. Then you could do: if (UpdateTimeFromResponse()) { // Stop timer. } else { // Figure out retry delay here, and set timer. } time_fetcher_.reset(); https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.h:94: UpdateFromNetworkLargeResponse); Up to you, but I'd suggest getting rid of these freind functions, and add ForTesting methods as needed. Gives a defined API for tests as well, and we have some sort of tests that ForTesting methods aren't called in production code, I believe. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.h:111: scoped_refptr<net::URLRequestContextGetter> getter_; Should probably include ref_counted.h https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:59: } This isn't needed - the test server will shut itself down on destruction. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:67: base::MessageLoop::current()->RunUntilIdle(); This works, but repeatedly spinning up a runloop while waiting for one thing to happen is considered a bit of an anti-pattern. Can we just watch the pref service's prefs::kNetworkTimeMapping, and when it's modified, quite the run loop? Or if there's some other way to watch the service I'm unaware of, you could use that instead. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:126: } You should also test a network error (Response has just an error code, no headers or body). Can do with with net::test_server::RawHttpResponse("", ""); https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:399: TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkBadSignature) { Seems like there are two cases we should care about testing: Bad signature / good data, and good signature / bad data. This one tests bad signature / bad data.
https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.cc:209: query_timer_.Reset(); Added a comment explaining why this is here. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.cc:298: // the standard interval. I guess the main reason is that it obviates the need to audit whether the timer is running, because it's always running. I.e. there is no failure mode in which time queries stop because I have failed to requeue the timer in some edge case. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.cc:310: time_fetcher_.reset(); That's a great idea. Done. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.h:94: UpdateFromNetworkLargeResponse); Yep, you're right. Done. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker.h:111: scoped_refptr<net::URLRequestContextGetter> getter_; On 2016/05/02 17:47:28, mmenke wrote: > Should probably include ref_counted.h Done. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:59: } On 2016/05/02 17:47:28, mmenke wrote: > This isn't needed - the test server will shut itself down on destruction. Done. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:67: base::MessageLoop::current()->RunUntilIdle(); Modification of prefs is an incidental effect of a *successful* fetch, so I'd rather not depend upon it. If it's important to not busy-wait here, then I'd rather block in the most straightforward way possible, e.g. with a condition variable. Would that work? https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:126: } On 2016/05/02 17:47:28, mmenke wrote: > You should also test a network error (Response has just an error code, no > headers or body). Can do with with net::test_server::RawHttpResponse("", ""); Done. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:399: TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkBadSignature) { It's a fair point that we should test good data + bad signature, to make sure that the bad signature alone is sufficient to cause the data to be rejected. I'm not sure what we would additionally learn from bad data + bad signature, though.
https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:399: TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkBadSignature) { On 2016/05/05 19:57:42, mab wrote: > It's a fair point that we should test good data + bad signature, to make sure > that the bad signature alone is sufficient to cause the data to be rejected. > I'm not sure what we would additionally learn from bad data + bad signature, > though. I didn't suggest bad data + bad signature, I said that's what this test was currently doing. I suggested bad data + good signature (Which basically protects against bugs server side). Admittedly, there are a lot of bad data paths, and checking them all is probably a bit too much.
https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:399: TEST_F(NetworkTimeTrackerTest, UpdateFromNetworkBadSignature) { Ah, I see what you're saying. Done.
LGTM! Though I did not do an exhaustive final pass. https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:67: base::MessageLoop::current()->RunUntilIdle(); On 2016/05/05 19:57:42, mab wrote: > Modification of prefs is an incidental effect of a *successful* fetch, so I'd > rather not depend upon it. > > If it's important to not busy-wait here, then I'd rather block in the most > straightforward way possible, e.g. with a condition variable. Would that work? SGTM https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker.h:84: void SetPublicKeyForTesting(base::StringPiece); nit: +key (argument name). https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker.h:84: void SetPublicKeyForTesting(base::StringPiece); Looks to me like StringPieces are generally passed by const ref. Not sure if it matters - all their methods are const, anyways, and they only include a pointer and a value. The preference for some similarly sized classes seems to be passing by value. https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker.h:90: void WaitForFetchForTesting(uint32_t nonce); include stdint.h https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:96: // Good signature over invalid, though made with a non-production key. "Good signature over invalid" seems a bit hard to follow. Maybe "Good signature for invalid response/body/data".
https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/400001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:67: base::MessageLoop::current()->RunUntilIdle(); I ended up doing this by simply passing in the MessageLoop, to be Quit when complete. It seemed most straightforward. LMK if this is horrible. :-) https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... File components/network_time/network_time_tracker.h (right): https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker.h:84: void SetPublicKeyForTesting(base::StringPiece); On 2016/05/06 15:00:37, mmenke wrote: > Looks to me like StringPieces are generally passed by const ref. Not sure if it > matters - all their methods are const, anyways, and they only include a pointer > and a value. The preference for some similarly sized classes seems to be > passing by value. Done. https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker.h:84: void SetPublicKeyForTesting(base::StringPiece); On 2016/05/06 15:00:37, mmenke wrote: > nit: +key (argument name). Done. https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker.h:90: void WaitForFetchForTesting(uint32_t nonce); On 2016/05/06 15:00:37, mmenke wrote: > include stdint.h Done. https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/1835823002/diff/440001/components/network_tim... components/network_time/network_time_tracker_unittest.cc:96: // Good signature over invalid, though made with a non-production key. On 2016/05/06 15:00:37, mmenke wrote: > "Good signature over invalid" seems a bit hard to follow. Maybe "Good signature > for invalid response/body/data". Done.
The CQ bit was checked by mab@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835823002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835823002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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, waffles@chromium.org, rohitrao@chromium.org, zea@chromium.org, sky@chromium.org, rsesek@chromium.org, rsleevi@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/1835823002/#ps520001 (title: "finch test fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835823002/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835823002/520001
Message was sent while issue was closed.
Description was changed from ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 ========== to ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 ==========
Message was sent while issue was closed.
Committed patchset #27 (id:520001)
Message was sent while issue was closed.
Description was changed from ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 ========== to ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} ==========
Message was sent while issue was closed.
Patchset 27 (id:??) landed as https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208}
Message was sent while issue was closed.
A revert of this CL (patchset #27 id:520001) has been created in https://codereview.chromium.org/1956173002/ by thakis@chromium.org. The reason for reverting is: Speculative; lots of browser tests started failing with leak reports, and all failing tests look network related. See https://build.chromium.org/p/chromium.linux/builders/Linux%20Tests%20%28dbg%2..., e.g. [444:555:0506/185642:ERROR:leak_tracker.h(100)] #0 0x7fd9c5c00d0e base::debug::StackTrace::StackTrace() #1 0x00000186cf6f base::debug::LeakTracker<>::LeakTracker() #2 0x0000018635bb SystemURLRequestContextGetter::SystemURLRequestContextGetter() #3 0x000001865c21 IOThread::InitSystemRequestContext() #4 0x000001865af5 IOThread::system_url_request_context_getter() #5 0x000001a08b90 BrowserProcessImpl::system_request_context() #6 0x000001ef00c4 safe_browsing::SafeBrowsingService::Initialize() #7 0x000001a0c1b0 BrowserProcessImpl::CreateSafeBrowsingService() #8 0x000001a0c022 BrowserProcessImpl::safe_browsing_service().
Message was sent while issue was closed.
rsleevi: I've uploaded my patch, reflecting your comments, to this the original issue. Will that work? (Do I want to remove the Cr-Commit-Position: metadata? I wonder if that might be the reason I can't use 'git cl try': "Cannot send tryjobs for a closed CL".) Sorry to be such a n00b.
Message was sent while issue was closed.
Description was changed from ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} ========== to ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} ==========
On 2016/05/10 18:14:23, mab wrote: > rsleevi: I've uploaded my patch, reflecting your comments, to this the original > issue. Will that work? > > (Do I want to remove the Cr-Commit-Position: metadata? I wonder if that might > be the reason I can't use 'git cl try': "Cannot send tryjobs for a closed CL".) Just click "Edit Issue" and uncheck "Closed". I did that for you.
LGTM. The changes you made are also correct (for URLFetchers), so I would suggest CQing (although you can run the tryjobs on memory bots to make sure, but I believe you got both of them)
CQ looks fine, but I wouldn't mind being sure, so I don't have to roll back again. I'm having trouble figuring out exactly which bot(s) were responsible for detecting the leak the first time around. http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem... isn't super helpful in this respect. At a guess I'd say anything with msan, asan, memcheck, or drmemory in the name, but if you can suggest something more precise I'm all ears.
On 2016/05/10 19:12:32, mab wrote: > CQ looks fine, but I wouldn't mind being sure, so I don't have to roll back > again. I'm having trouble figuring out exactly which bot(s) were responsible > for detecting the leak the first time around. > http://www.chromium.org/developers/tree-sheriffs/sheriff-details-chromium/mem... > isn't super helpful in this respect. At a guess I'd say anything with msan, > asan, memcheck, or drmemory in the name, but if you can suggest something more > precise I'm all ears. Because the leak was a URLRequest, it actually started triggering DCHECKs. So any of the DBG bots should be fine (Nico's Comment #56 indicated Linux Tests (dbg)(1) was the bot so... look for a bot like that)
The CQ bit was checked by mab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from waffles@chromium.org, sky@chromium.org, estark@chromium.org, rsesek@chromium.org, zea@chromium.org, mmenke@chromium.org, rohitrao@chromium.org Link to the patchset: https://codereview.chromium.org/1835823002/#ps580001 (title: "lint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1835823002/580001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1835823002/580001
Message was sent while issue was closed.
Description was changed from ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} ========== to ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} ==========
Message was sent while issue was closed.
Committed patchset #30 (id:580001)
Message was sent while issue was closed.
Description was changed from ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} ========== to ========== network_time_tracker: add temporary time protocol. This adds queries of the temporary secure time server, with signature verification, and a very conservative scheduler. (Checks once per hour, queries only if sync believed to be lost.) BUG=589700 Committed: https://crrev.com/ee2016a374b73ba35508ae26837755a23b3defa4 Cr-Commit-Position: refs/heads/master@{#392208} Committed: https://crrev.com/2f07cb9a5744a021e3d7c34e9c5de5ef6d246f27 Cr-Commit-Position: refs/heads/master@{#392711} ==========
Message was sent while issue was closed.
Patchset 30 (id:??) landed as https://crrev.com/2f07cb9a5744a021e3d7c34e9c5de5ef6d246f27 Cr-Commit-Position: refs/heads/master@{#392711} |
