Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2453523002:
Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries (Closed)
DescriptionAdd NetworkTimeTracker::StartTimeFetch() for on-demand time queries
Since network time is frequently not available when a certificate date
error is encountered, this CL adds a method to start a time fetch on
demand and call a callback when the on-demand fetch completes.
A follow-up CL will use StartTimeFetch() to start an on-demand time
fetch when a certificate date error is encountered and delay the
interstitial (up to a maximum timeout) until the fetch completes.
On-demand time queries are gated behind a Finch experiment param so that
the on-demand fetch behavior can be controlled dynamically.
This is a series of patches to add on-demand time fetches when handling
cert date invalid errors.
CL #1: https://codereview.chromium.org/2447183002/
CL #2: https://codereview.chromium.org/2453523002/ <== this one
CL #3: https://codereview.chromium.org/2449193002/
BUG=589700
Committed: https://crrev.com/7f9d03bdd801f303ae6e5987fd054bde02da779d
Cr-Commit-Position: refs/heads/master@{#428537}
Patch Set 1 #
Total comments: 9
Patch Set 2 : meacer comments #
Total comments: 2
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (12 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Description was changed from ========== Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries Since network time is frequently not available when a certificate date error is encountered, this CL adds a method to start a time fetch on demand and call a callback when the on-demand fetch completes. A follow-up CL will use StartTimeFetch() to start an on-demand time fetch when a certificate date error is encountered and delay the interstitial (up to a maximum timeout) until the fetch completes. On-demand time queries are gated behind a Finch experiment param so that the on-demand fetch behavior can be controlled dynamically. BUG=589700 ========== to ========== Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries Since network time is frequently not available when a certificate date error is encountered, this CL adds a method to start a time fetch on demand and call a callback when the on-demand fetch completes. A follow-up CL will use StartTimeFetch() to start an on-demand time fetch when a certificate date error is encountered and delay the interstitial (up to a maximum timeout) until the fetch completes. On-demand time queries are gated behind a Finch experiment param so that the on-demand fetch behavior can be controlled dynamically. This is a series of patches to add on-demand time fetches when handling cert date invalid errors. CL #1: https://codereview.chromium.org/2447183002/ CL #2: https://codereview.chromium.org/2453523002/ <== this one CL #3: https://codereview.chromium.org/2449193002/ BUG=589700 ==========
estark@chromium.org changed reviewers: + meacer@chromium.org
meacer: two of three. This adds the NetworkTimeTracker API to initiate a time query on-demand, but doesn't use it anywhere yet. https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:322: return NETWORK_TIME_NO_SUCCESSFUL_SYNC; unrelated cleanup, sorry. there was too much indentation.
Some questions https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:393: // start one now. For my own education: If two clients call StartTimeFetch one of the other, this will result in a single CheckTime operation, but with both callbacks being called. Is my understanding correct here? If yes, what happens when 4 clients call StartTimeFetch with 1 second intervals? We'll have 4 callbacks, and a single fetch happening after 4 seconds. Is it going to be a problem if the owner of the first callback disappears? (E.g. the 3 second interstitial wait expires, the interstitial is shown, ssl error handler is deleted which owns the callback) Let me know if none of my words make sense :) https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker_unittest.cc:531: base::TimeDelta::FromMilliseconds(1461621971825), Just curious, what's the significance of 04/25/2016 @ 10:06pm (UTC)?
https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:393: // start one now. On 2016/10/26 00:38:15, Mustafa Emre Acer wrote: > For my own education: If two clients call StartTimeFetch one of the other, this > will result in a single CheckTime operation, but with both callbacks being > called. > > Is my understanding correct here? If yes, what happens when 4 clients call > StartTimeFetch with 1 second intervals? > We'll have 4 callbacks, and a single fetch happening after 4 seconds. Is it > going to be a problem if the owner of the first callback disappears? (E.g. the 3 > second interstitial wait expires, the interstitial is shown, ssl error handler > is deleted which owns the callback) > > Let me know if none of my words make sense :) "If two clients call StartTimeFetch one of the other" should be "If two clients call StartTimeFetch one after the other". To answer my own question, I think this might be problematic since SSLErrorHandler is passed as Unretained: https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Bind... You'll probably want to cancel the callback when the error handler goes away, or make it a weak ptr as described in Binding-A-Class-Method-With-Weak-Pointers. CheckSuggestedURL doesn't have that problem since the objects that run the callbacks (timer_ and common_name_mismatch_handler_) are owned by SSLErrorHandler, so when it's destroyed callbacks don't run.
On 2016/10/26 01:07:37, Mustafa Emre Acer wrote: > https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... > File components/network_time/network_time_tracker.cc (right): > > https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... > components/network_time/network_time_tracker.cc:393: // start one now. > On 2016/10/26 00:38:15, Mustafa Emre Acer wrote: > > For my own education: If two clients call StartTimeFetch one of the other, > this > > will result in a single CheckTime operation, but with both callbacks being > > called. > > > > Is my understanding correct here? Yep, that's correct. > > If yes, what happens when 4 clients call > > StartTimeFetch with 1 second intervals? > > We'll have 4 callbacks, and a single fetch happening after 4 seconds. Is it > > going to be a problem if the owner of the first callback disappears? (E.g. the > 3 > > second interstitial wait expires, the interstitial is shown, ssl error handler > > is deleted which owns the callback) > > > > Let me know if none of my words make sense :) > > > "If two clients call StartTimeFetch one of the other" should be "If two clients > call StartTimeFetch one after the other". > > To answer my own question, I think this might be problematic since > SSLErrorHandler is passed as Unretained: > https://chromium.googlesource.com/chromium/src/+/master/docs/callback.md#Bind... > > You'll probably want to cancel the callback when the error handler goes away, or > make it a weak ptr as described in Binding-A-Class-Method-With-Weak-Pointers. > > CheckSuggestedURL doesn't have that problem since the objects that run the > callbacks (timer_ and common_name_mismatch_handler_) are owned by > SSLErrorHandler, so when it's destroyed callbacks don't run. Thanks. I think using a WeakPtr is simplest here and gives the behavior I want, so I updated the follow-up CL (https://codereview.chromium.org/2449193002/) to do that.
https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker_unittest.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker_unittest.cc:531: base::TimeDelta::FromMilliseconds(1461621971825), On 2016/10/26 00:38:15, Mustafa Emre Acer wrote: > Just curious, what's the significance of 04/25/2016 @ 10:06pm (UTC)? Not sure! My best guess is that it was arbitrarily chosen and was the time that the tests in this file were originally written.
Lgtm with nits. https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:375: bool NetworkTimeTracker::StartTimeFetch(const base::Closure& closure) { Could you add a DCHECK(thread_checker_.CalledOnValidThread()) here? https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:402: // to complete. nit: I'm a bit confused by this comment, it sounds like no callbacks are expected instead of having to clear existing callbacks. Perhaps you could reword it to say something like "If no query is in progress, no callbacks need to be called"?
Thanks! https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:375: bool NetworkTimeTracker::StartTimeFetch(const base::Closure& closure) { On 2016/10/26 19:19:22, Mustafa Emre Acer wrote: > Could you add a DCHECK(thread_checker_.CalledOnValidThread()) here? Done. https://codereview.chromium.org/2453523002/diff/1/components/network_time/net... components/network_time/network_time_tracker.cc:402: // to complete. On 2016/10/26 19:19:22, Mustafa Emre Acer wrote: > nit: I'm a bit confused by this comment, it sounds like no callbacks are > expected instead of having to clear existing callbacks. Perhaps you could reword > it to say something like "If no query is in progress, no callbacks need to be > called"? Oh, yes, what I wrote was confusing. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2453523002/#ps20001 (title: "meacer comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2453523002/diff/20001/components/network_time... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/20001/components/network_time... components/network_time/network_time_tracker.cc:395: timer_.Stop(); Since this hasn't landed yet, I think there is a missed opportunity here: "// Stop trying to make fetch happen." (complimentary to line 430)
https://codereview.chromium.org/2453523002/diff/20001/components/network_time... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2453523002/diff/20001/components/network_time... components/network_time/network_time_tracker.cc:395: timer_.Stop(); On 2016/10/26 21:56:06, Mustafa Emre Acer wrote: > Since this hasn't landed yet, I think there is a missed opportunity here: "// > Stop trying to make fetch happen." (complimentary to line 430) Hahah. This is indeed a golden opportunity, but... not worth switching branches for. :P
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries Since network time is frequently not available when a certificate date error is encountered, this CL adds a method to start a time fetch on demand and call a callback when the on-demand fetch completes. A follow-up CL will use StartTimeFetch() to start an on-demand time fetch when a certificate date error is encountered and delay the interstitial (up to a maximum timeout) until the fetch completes. On-demand time queries are gated behind a Finch experiment param so that the on-demand fetch behavior can be controlled dynamically. This is a series of patches to add on-demand time fetches when handling cert date invalid errors. CL #1: https://codereview.chromium.org/2447183002/ CL #2: https://codereview.chromium.org/2453523002/ <== this one CL #3: https://codereview.chromium.org/2449193002/ BUG=589700 ========== to ========== Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries Since network time is frequently not available when a certificate date error is encountered, this CL adds a method to start a time fetch on demand and call a callback when the on-demand fetch completes. A follow-up CL will use StartTimeFetch() to start an on-demand time fetch when a certificate date error is encountered and delay the interstitial (up to a maximum timeout) until the fetch completes. On-demand time queries are gated behind a Finch experiment param so that the on-demand fetch behavior can be controlled dynamically. This is a series of patches to add on-demand time fetches when handling cert date invalid errors. CL #1: https://codereview.chromium.org/2447183002/ CL #2: https://codereview.chromium.org/2453523002/ <== this one CL #3: https://codereview.chromium.org/2449193002/ BUG=589700 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries Since network time is frequently not available when a certificate date error is encountered, this CL adds a method to start a time fetch on demand and call a callback when the on-demand fetch completes. A follow-up CL will use StartTimeFetch() to start an on-demand time fetch when a certificate date error is encountered and delay the interstitial (up to a maximum timeout) until the fetch completes. On-demand time queries are gated behind a Finch experiment param so that the on-demand fetch behavior can be controlled dynamically. This is a series of patches to add on-demand time fetches when handling cert date invalid errors. CL #1: https://codereview.chromium.org/2447183002/ CL #2: https://codereview.chromium.org/2453523002/ <== this one CL #3: https://codereview.chromium.org/2449193002/ BUG=589700 ========== to ========== Add NetworkTimeTracker::StartTimeFetch() for on-demand time queries Since network time is frequently not available when a certificate date error is encountered, this CL adds a method to start a time fetch on demand and call a callback when the on-demand fetch completes. A follow-up CL will use StartTimeFetch() to start an on-demand time fetch when a certificate date error is encountered and delay the interstitial (up to a maximum timeout) until the fetch completes. On-demand time queries are gated behind a Finch experiment param so that the on-demand fetch behavior can be controlled dynamically. This is a series of patches to add on-demand time fetches when handling cert date invalid errors. CL #1: https://codereview.chromium.org/2447183002/ CL #2: https://codereview.chromium.org/2453523002/ <== this one CL #3: https://codereview.chromium.org/2449193002/ BUG=589700 Committed: https://crrev.com/7f9d03bdd801f303ae6e5987fd054bde02da779d Cr-Commit-Position: refs/heads/master@{#428537} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7f9d03bdd801f303ae6e5987fd054bde02da779d Cr-Commit-Position: refs/heads/master@{#428537} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
