Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449193002:
Attempt an on-demand time fetch when encountering a date invalid error (Closed)
DescriptionAttempt an on-demand time fetch when encountering a date invalid error
This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date
invalid error is encountered. If network time is not available,
StartTimeFetch() will kick off an on-demand time query, calling a
callback when it completes. Thus, when network time is unavailable,
SSLErrorHandler now waits up to 3 seconds for a time query to complete
before showing an interstitial. This is motivated by the fact that
network time is unavailable (either due to lost sync or due to no
successful time queries) for ~25% of cert date invalid errors.
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/
CL #3: https://codereview.chromium.org/2449193002/ <== this one
BUG=589700
Committed: https://crrev.com/5057f82a13bba2cd2a98539371f21188f3e28500
Cr-Commit-Position: refs/heads/master@{#430720}
Patch Set 1 #Patch Set 2 : add a test for the timer expiring #Patch Set 3 : fix comments #Patch Set 4 : Use WeakPtr so that SSLErrorHandler can be destroyed #
Total comments: 8
Patch Set 5 : meacer comments #Patch Set 6 : rebase #Patch Set 7 : add browser tests (rough draft, not ready for review) #Patch Set 8 : cleanup #
Total comments: 32
Patch Set 9 : rework browser tests, meacer comments #Patch Set 10 : fix unit tests #Patch Set 11 : fix unit tests again #Patch Set 12 : add comments #Patch Set 13 : remove unnecessary EmptyClosure #
Total comments: 26
Patch Set 14 : meacer comments #Patch Set 15 : do not queue a non-delayed request #Patch Set 16 : remove unnecessary allocation causing memory leak #
Total comments: 7
Patch Set 17 : meacer comments #
Total comments: 28
Patch Set 18 : mmenke comments #
Total comments: 1
Patch Set 19 : remove obsolete comment about network delegate #Patch Set 20 : Check that network time query state is as expected before triggering response #
Total comments: 6
Patch Set 21 : meacer nits #Depends on Patchset: Messages
Total messages: 89 (60 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...
Description was changed from ========== Attempt an on-demand time fetch when encountered a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. BUG=589700 ========== to ========== Attempt an on-demand time fetch when encountered a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 ==========
estark@chromium.org changed reviewers: + meacer@chromium.org
meacer: three of three. This uses the new StartTimeFetch() method to kick off time fetches (if necessary) when we encounter a cert date invalid error.
Description was changed from ========== Attempt an on-demand time fetch when encountered a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 ========== to ========== Attempt an on-demand time fetch when encountering a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 ==========
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 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: This issue passed the CQ dry run.
Looking good. I think we might also want to add browser tests for this as well. ssl_browser_tests.cc contains some test cases for name mismatch, so perhaps you could add similar tests in there. https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:155: class CommonNameSSLErrorHandlerTest : public ChromeRenderViewHostTestHarness { nit: maybe rename to SSLErrorHandlerNameMismatchTest? That way we can use --gtest_filter=SSLErrorHandler* for all tests here. https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:191: class DateInvalidSSLErrorHandlerTest : public ChromeRenderViewHostTestHarness { And this one to SSLErrorHandlerDateInvalidTest? https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:472: // should not be shown. Just to confirm: This is assuming we don't have the build time heuristics enabled, right? Otherwise, we'd still show the bad clock ui? https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:519: ClearErrorHandler(); Is the idea here that the test would crash with a null deref if it failed?
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: This issue passed the CQ dry run.
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: This issue passed the CQ dry run.
Added some browser tests, PTAL. In doing so, I changed the NetworkTimeTracker Finch param: now instead of on-demand time queries being on or off, there are 3 behaviors: fetches happen on-demand only, fetches happen in the background only (this is the behavior today), or fetches happen both in the background and on-demand. This was useful for the tests (to be able to isolate the effect of the on-demand fetches) but I think it'll also be useful experimentally. (For example, if on-demand + background fetches doesn't catch any more errors than on-demand only, maybe we should not do background fetches.) I also had to tweak network_time::FieldTrialTest a bit to work in a browser test. https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... File chrome/browser/ssl/ssl_error_handler_unittest.cc (right): https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:155: class CommonNameSSLErrorHandlerTest : public ChromeRenderViewHostTestHarness { On 2016/10/26 20:03:02, Mustafa Emre Acer wrote: > nit: maybe rename to SSLErrorHandlerNameMismatchTest? > > That way we can use --gtest_filter=SSLErrorHandler* for all tests here. Done. https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:191: class DateInvalidSSLErrorHandlerTest : public ChromeRenderViewHostTestHarness { On 2016/10/26 20:03:03, Mustafa Emre Acer wrote: > And this one to SSLErrorHandlerDateInvalidTest? Done. https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:472: // should not be shown. On 2016/10/26 20:03:03, Mustafa Emre Acer wrote: > Just to confirm: This is assuming we don't have the build time heuristics > enabled, right? Otherwise, we'd still show the bad clock ui? Sorta. It's assuming that the build time heuristic doesn't fire because the build timestamp matches the system clock on which the tests are running. (That is, in tests we assume that the test build is fresh relative to the clock of the system on which the tests are running.) https://codereview.chromium.org/2449193002/diff/60001/chrome/browser/ssl/ssl_... chrome/browser/ssl/ssl_error_handler_unittest.cc:519: ClearErrorHandler(); On 2016/10/26 20:03:02, Mustafa Emre Acer wrote: > Is the idea here that the test would crash with a null deref if it failed? Yep
Looking good. Mostly questions and nits, but also a request for an additional test case (when the user closes the tab). https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2860: // certificate date errors. nit: Can you also document the capabilities of this class? e.g. "Can simulate hanging or delayed network requests..." https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2875: network_time::FieldTrialTest* field_trial_test() { const method https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2887: WaitingGoodTimeResponseHandler(base::RunLoop* run_loop, nit: Waiting -> Delayed? (I don't feel strongly about this) https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2972: IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, OnDemandFetchClockWrong) { My understanding is that this test sets the clock so that build time heuristic won't work. That seems a bit fragile if someone decides to change BTH. Can we ensure that BTH doesn't fire here, maybe with an EXPECT_EQ? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2981: base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime)); Is there a particular reason to use JS time and make kGoodTimeResponseHandlerTime double? Can it be a uint64 instead, and can you use base::Time::FromMilliseconds(...) or something like that here? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3010: observer.Wait(); You might also want to put a WaitForInterstitialAttach(contents) here. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3053: WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE); Is there a reason not to use ui_test_utils::NavigateToURL here too? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3066: EXPECT_FALSE(ssl_error_handler); Nit: single line? EXPECT_FALSE(SSLErrorHandler::FromWebContents(contents)) https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3084: WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE); Same as above: ui_test_utils::NavigateToURL? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3093: SSLErrorHandler::FromWebContents(contents); Same as above: single line? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3101: // fetch completes or the timeout expires, then there is no I think we can improve test and the ones above. We don't seem to check what happens when a network fetch actually happens. Can we change the hangs to waits, and then trigger the network time response after navigating away? Not crashing should be fine. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3133: Maybe add another test where the tab is closed? You'll probably need two tabs since closing the only tab would close the browser. https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... File components/network_time/network_time_test_utils.h (right): https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... components/network_time/network_time_test_utils.h:32: extern const double kGoodTimeResponseHandlerTime; Why not an integer in seconds and milliseconds? https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... components/network_time/network_time_test_utils.h:51: // FieldTrialList has already been constructed.) nit: Perhaps you could also add CreateForUnitTest and make the constructor private. https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... components/network_time/network_time_tracker.cc:557: return; nit: Maybe revert the condition? It's a bit more readable: if (param == "on-demand-only" ) { timer_.Start(...) }
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: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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 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 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...
Ok, wphew, I took another pass at the browser tests. As discussed in person, when I was working on your suggestions, I realized that the whole strategy of delaying an EmbeddedTestServer request handler doesn't really work. The test server has its own IO thread, where the request handlers run, and that thread isn't really exposed, so it's hard (impossible?) to communicate between the main test thread and the EmbeddedTestServer IO thread. So, I switched to use a URLRequestInterceptor instead, which is a lot of boilerplate but seems to work. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2860: // certificate date errors. On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > nit: Can you also document the capabilities of this class? > > e.g. "Can simulate hanging or delayed network requests..." Done. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2875: network_time::FieldTrialTest* field_trial_test() { On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > const method Done. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2887: WaitingGoodTimeResponseHandler(base::RunLoop* run_loop, On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > nit: Waiting -> Delayed? (I don't feel strongly about this) Done. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2972: IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, OnDemandFetchClockWrong) { On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > My understanding is that this test sets the clock so that build time heuristic > won't work. That seems a bit fragile if someone decides to change BTH. Can we > ensure that BTH doesn't fire here, maybe with an EXPECT_EQ? The purpose of setting the test clock is actually so that the clock is wrong relative to network time. However, you're right that it's fragile; if the network time fetch in the test didn't work, then the BTH could fire. I'm now using SetBuildTimeForTesting() to set the build time equal to the testing clock time, so that the BTH will never fire. (Unless of course someone makes it so that the BTH fires when build time == system time. That seems unlikely though.) https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2981: base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime)); On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > Is there a particular reason to use JS time and make > kGoodTimeResponseHandlerTime double? Can it be a uint64 instead, and can you use > base::Time::FromMilliseconds(...) or something like that here? My thinking here is that it should be relatively easy to update the time that GoodTimeResponseHandler returns. To do so with the code as is, you run a curl command to the real server (see https://cs.chromium.org/chromium/src/components/network_time/network_time_tes...) and then copy the 'current_time_millis' value into |kGoodTimeResponseHandlerTime|. If I were to make |kGoodTimeResponseHandlerTime| use some other representation, you'd have to convert the 'current_time_millis' value into that representation... which might be easy to do but might also depend on internal implementation details of base::Time. Does that make any sense? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3010: observer.Wait(); On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > You might also want to put a WaitForInterstitialAttach(contents) here. Done. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3053: WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE); On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > Is there a reason not to use ui_test_utils::NavigateToURL here too? This is based on CommonNameMismatchBrowserTest.InterstitialsStopNavigationWhileLoading. I assume the idea here is to Stop() while the page is still loading. So we can't use NavigateToURL() because NavigateToURL() won't return until the page loads (I thought?), which I assume won't be until the timer fires or the network fetch completes, i.e. never. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3066: EXPECT_FALSE(ssl_error_handler); On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > Nit: single line? > EXPECT_FALSE(SSLErrorHandler::FromWebContents(contents)) Done. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3084: WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE); On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > Same as above: ui_test_utils::NavigateToURL? See above https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3093: SSLErrorHandler::FromWebContents(contents); On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > Same as above: single line? Done. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3101: // fetch completes or the timeout expires, then there is no On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > I think we can improve test and the ones above. We don't seem to check what > happens when a network fetch actually happens. > > Can we change the hangs to waits, and then trigger the network time response > after navigating away? Not crashing should be fine. Done. This turns out to be a nice simplification because I can use the same response handler for all the tests, instead of using one that delays and one that hangs indefinitely. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3133: On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > Maybe add another test where the tab is closed? You'll probably need two tabs > since closing the only tab would close the browser. Done. https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... File components/network_time/network_time_test_utils.h (right): https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... components/network_time/network_time_test_utils.h:32: extern const double kGoodTimeResponseHandlerTime; On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > Why not an integer in seconds and milliseconds? See explanation above; I think if this matches the representation that you get back from the real time server, it's easiest to update this value. https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... components/network_time/network_time_test_utils.h:51: // FieldTrialList has already been constructed.) On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > nit: Perhaps you could also add CreateForUnitTest and make the constructor > private. Done. https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2449193002/diff/140001/components/network_tim... components/network_time/network_time_tracker.cc:557: return; On 2016/11/01 22:20:18, Mustafa Emre Acer wrote: > nit: Maybe revert the condition? It's a bit more readable: > > if (param == "on-demand-only" ) { > timer_.Start(...) > } Done.
(Btw you might want to diff against patch set 8)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for adding the browser tests. Overall looking good again, just more questions and nits. https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2981: base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime)); On 2016/11/02 19:58:12, estark wrote: > On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > > Is there a particular reason to use JS time and make > > kGoodTimeResponseHandlerTime double? Can it be a uint64 instead, and can you > use > > base::Time::FromMilliseconds(...) or something like that here? > > My thinking here is that it should be relatively easy to update the time that > GoodTimeResponseHandler returns. To do so with the code as is, you run a curl > command to the real server (see > https://cs.chromium.org/chromium/src/components/network_time/network_time_tes...) > and then copy the 'current_time_millis' value into > |kGoodTimeResponseHandlerTime|. If I were to make |kGoodTimeResponseHandlerTime| > use some other representation, you'd have to convert the 'current_time_millis' > value into that representation... which might be easy to do but might also > depend on internal implementation details of base::Time. > > Does that make any sense? Ah, okay. Would it help to rename kGoodTimeResponseHandlerTime to kGoodTimeResponseHandlerJsTime? https://codereview.chromium.org/2449193002/diff/140001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3053: WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE); On 2016/11/02 19:58:12, estark wrote: > On 2016/11/01 22:20:17, Mustafa Emre Acer wrote: > > Is there a reason not to use ui_test_utils::NavigateToURL here too? > > This is based on > CommonNameMismatchBrowserTest.InterstitialsStopNavigationWhileLoading. I assume > the idea here is to Stop() while the page is still loading. So we can't use > NavigateToURL() because NavigateToURL() won't return until the page loads (I > thought?), which I assume won't be until the timer fires or the network fetch > completes, i.e. never. Hmm, the documentation of this method says that this method blocks until the navigation finishes, but then the code doesn't wait when the last param is BROWSER_TEST_NONE. Thanks ui_test_utils, very helpful!1 I filed https://crbug.com/661804 to rename the param. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2863: class DelayableTimeURLRequestJob : public net::URLRequestJob { nit: rename to Delayable*Network*TimeURLRequestJob? https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2924: void Resume(const base::Closure& callback) { nit: dcheck for io thread? https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2941: class DelayedTimeInterceptor : public net::URLRequestInterceptor { nit: rename to DelayedNetworkTimeInterceptor? https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2950: // then |resumed_| will be true and the request will not delay. nit: Slightly confusing :) How about a "bool delayed = !resumed_;" line? Or perhaps you could rename resumed_ to delay_requests_? I think I like that better. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2957: void ResumeAll(const base::Closure& callback) { nit: Could use a DCHECK for IO thread here. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2977: class SSLNetworkTimeIOThreadHelper { nit: I think you could move the functionality in this class directly to SSLNetworkTimeBrowserTest. It feels slightly better since the reader will need to keep track of one fewer file. Also, one fewer indirection. If you do that, you'll want to make the interceptor supply a weak pointer though. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3039: embedded_test_server()->GetURL("/"))); Since actual requests won't go through, there shouldn't be a need to use the embedded server here. You can use "http://time.test" or some such. And another option: You can change SetTimeServerURLForTesting to GetTimeServerURLForTesting, and pass that to the interceptor. I feel this is slightly better, since it doesn't change any state in the network time tracker. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3097: base::RunLoop wait_for_response; nit: rename wait_for_time_response? (otherwise it reads like we are waiting for the interstitial response) https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3265: TriggerTimeResponse(wait_for_response.QuitClosure()); What happens if there were more than one delayed network time requests? https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3301: // should occur. nit: "Navigate away, then trigger the network time response and wait for the response; no crash should occur." https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3330: // should occur. nit: Fits into previous line https://codereview.chromium.org/2449193002/diff/240001/components/network_tim... File components/network_time/network_time_test_utils.h (right): https://codereview.chromium.org/2449193002/diff/240001/components/network_tim... components/network_time/network_time_test_utils.h:36: extern const char kGoodTimeResponseBody[]; nit: Blank line? (here and below)
https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2977: class SSLNetworkTimeIOThreadHelper { On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: I think you could move the functionality in this class directly to > SSLNetworkTimeBrowserTest. It feels slightly better since the reader will need > to keep track of one fewer file. Also, one fewer indirection. > > If you do that, you'll want to make the interceptor supply a weak pointer > though. And by "one fewer file" I meant "one fewer class".
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 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...
https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2863: class DelayableTimeURLRequestJob : public net::URLRequestJob { On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: rename to Delayable*Network*TimeURLRequestJob? Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2924: void Resume(const base::Closure& callback) { On 2016/11/02 22:43:23, Mustafa Emre Acer wrote: > nit: dcheck for io thread? Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2941: class DelayedTimeInterceptor : public net::URLRequestInterceptor { On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: rename to DelayedNetworkTimeInterceptor? Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2950: // then |resumed_| will be true and the request will not delay. On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: Slightly confusing :) How about a "bool delayed = !resumed_;" line? > > Or perhaps you could rename resumed_ to delay_requests_? I think I like that > better. Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2957: void ResumeAll(const base::Closure& callback) { On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: Could use a DCHECK for IO thread here. Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2977: class SSLNetworkTimeIOThreadHelper { On 2016/11/02 22:44:38, Mustafa Emre Acer wrote: > On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > > nit: I think you could move the functionality in this class directly to > > SSLNetworkTimeBrowserTest. It feels slightly better since the reader will need > > to keep track of one fewer file. Also, one fewer indirection. > > > > If you do that, you'll want to make the interceptor supply a weak pointer > > though. > > And by "one fewer file" I meant "one fewer class". Done. What do you mean by "make the interceptor supply a weak pointer" though? I called these methods with a weak pointer to the test fixture, is that what you mean? https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3039: embedded_test_server()->GetURL("/"))); On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > Since actual requests won't go through, there shouldn't be a need to use the > embedded server here. You can use "http://time.test" or some such. > > And another option: You can change SetTimeServerURLForTesting to > GetTimeServerURLForTesting, and pass that to the interceptor. I feel this is > slightly better, since it doesn't change any state in the network time tracker. > Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3097: base::RunLoop wait_for_response; On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: rename wait_for_time_response? (otherwise it reads like we are waiting for > the interstitial response) Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3265: TriggerTimeResponse(wait_for_response.QuitClosure()); On 2016/11/02 22:43:23, Mustafa Emre Acer wrote: > What happens if there were more than one delayed network time requests? This would only wait for the first one. Subsequent requests would call QuitClosure() again which according to the documentation should have no effect. I think all the tests only expect one request, so maybe I should just assert that there is only one request somewhere in the interceptor or in the URLRequestJob. WDYT? https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3301: // should occur. On 2016/11/02 22:43:23, Mustafa Emre Acer wrote: > nit: > "Navigate away, then trigger the network time response and wait for the > response; no crash should occur." Done. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3330: // should occur. On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > nit: Fits into previous line Done. https://codereview.chromium.org/2449193002/diff/240001/components/network_tim... File components/network_time/network_time_test_utils.h (right): https://codereview.chromium.org/2449193002/diff/240001/components/network_tim... components/network_time/network_time_test_utils.h:36: extern const char kGoodTimeResponseBody[]; On 2016/11/02 22:43:23, Mustafa Emre Acer wrote: > nit: Blank line? (here and below) Done.
(My brain is kind of fried so don't feel pressured to take another look tonight; I'll look it over tomorrow with fresh eyes and might find all kinds of silly mistakes)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
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...
Ok, I think this is ready for another review. I'm getting all confused about the lifetimes/ownership so I think that part in particular needs a sanity check. Right now, the test has the interceptor as a member, and the interceptor is created and accessed on the IO thread only (and owned by the URLRequestFilter instance). Whenever the test needs to do something on the IO thread, it posts a task to a test fixture method, using a weak pointer to the test fixture. I think this will work and is okay, but I feel like maybe it's more complicated than necessary, but I also can't figure out a simpler way...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think this is fine now, but now my brain has fried and I'd like to do final pass early tomorrow before giving the lgt and m. https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/240001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2977: class SSLNetworkTimeIOThreadHelper { On 2016/11/03 01:36:04, estark wrote: > On 2016/11/02 22:44:38, Mustafa Emre Acer wrote: > > On 2016/11/02 22:43:22, Mustafa Emre Acer wrote: > > > nit: I think you could move the functionality in this class directly to > > > SSLNetworkTimeBrowserTest. It feels slightly better since the reader will > need > > > to keep track of one fewer file. Also, one fewer indirection. > > > > > > If you do that, you'll want to make the interceptor supply a weak pointer > > > though. > > > > And by "one fewer file" I meant "one fewer class". > > Done. What do you mean by "make the interceptor supply a weak pointer" though? I > called these methods with a weak pointer to the test fixture, is that what you > mean? Never mind that, you're not posting any tasks in the interceptor so this is fine. You are doing that in DelayableNetworkTimeURLRequestJob which already has a weak ptr factory. But I don't think the weak ptr in the test fixture is necessary. https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3021: weak_factory_.GetWeakPtr(), You can also create the interceptor here, and pass a unique pointer to it to the other thread using base::Passed. You can then move SetUpNetworkTimeInterceptorOnIOThread outside this class. That way, you can get rid of the weak ptr. Something like this: void AddInterceptor(const GURL& time_service_url, std::unique_ptr<Interceptor> interceptor) { // DCHECK IO thread // Your code in SetUpNetworkTimeInterceptorOnIOThread } void SetUpNetworkTimeServer() { interceptor_ = new DelayedNetworkTimeInterceptor(); PostTask(..., base::Bind(&AddInterceptor, time_server_url(), base::Passed(std::unique_ptr<Interceptor>(interceptor))); } But that might complicate things with ResumeDelayedNetworkTimeRequests. I think I now understand why you had the io_thread_helper class in the first place (to wrap all of this).
Two more comments. https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3026: void TriggerTimeResponse(const base::Closure& callback) { Since |callback| is always a quit closure, you might want to rename it to quit_closure, here and elsewhere. https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3052: base::WeakPtrFactory<SSLNetworkTimeBrowserTest> weak_factory_; I don't think this is necessary as the fixture will outlive all the callbacks anyways.
https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3021: weak_factory_.GetWeakPtr(), On 2016/11/04 01:17:26, Mustafa Emre Acer wrote: > You can also create the interceptor here, and pass a unique pointer to it to the > other thread using base::Passed. > > You can then move SetUpNetworkTimeInterceptorOnIOThread outside this class. That > way, you can get rid of the weak ptr. > > Something like this: > > void AddInterceptor(const GURL& time_service_url, > std::unique_ptr<Interceptor> interceptor) { > // DCHECK IO thread > // Your code in SetUpNetworkTimeInterceptorOnIOThread > } > > void SetUpNetworkTimeServer() { > interceptor_ = new DelayedNetworkTimeInterceptor(); > PostTask(..., > base::Bind(&AddInterceptor, > time_server_url(), > base::Passed(std::unique_ptr<Interceptor>(interceptor))); > } > > > But that might complicate things with ResumeDelayedNetworkTimeRequests. I think > I now understand why you had the io_thread_helper class in the first place (to > wrap all of this). Hmm, not sure if you were making a suggestion here or concluded that it wouldn't be good because it would muddle things with ResumeDelayedNetworkTimeRequests()? I think I could do what you're saying but would have to pass base::Unretained(interceptor_) to ResumeDelayedNetworkTimeRequests. WDYT? Is that better than having a test fixture method and passing `base::Unretained(this)` as you suggested below? https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3026: void TriggerTimeResponse(const base::Closure& callback) { On 2016/11/04 19:21:07, Mustafa Emre Acer wrote: > Since |callback| is always a quit closure, you might want to rename it to > quit_closure, here and elsewhere. Done. https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3052: base::WeakPtrFactory<SSLNetworkTimeBrowserTest> weak_factory_; On 2016/11/04 19:21:07, Mustafa Emre Acer wrote: > I don't think this is necessary as the fixture will outlive all the callbacks > anyways. Done.
https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/300001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3021: weak_factory_.GetWeakPtr(), On 2016/11/04 20:43:30, estark wrote: > On 2016/11/04 01:17:26, Mustafa Emre Acer wrote: > > You can also create the interceptor here, and pass a unique pointer to it to > the > > other thread using base::Passed. > > > > You can then move SetUpNetworkTimeInterceptorOnIOThread outside this class. > That > > way, you can get rid of the weak ptr. > > > > Something like this: > > > > void AddInterceptor(const GURL& time_service_url, > > std::unique_ptr<Interceptor> interceptor) { > > // DCHECK IO thread > > // Your code in SetUpNetworkTimeInterceptorOnIOThread > > } > > > > void SetUpNetworkTimeServer() { > > interceptor_ = new DelayedNetworkTimeInterceptor(); > > PostTask(..., > > base::Bind(&AddInterceptor, > > time_server_url(), > > base::Passed(std::unique_ptr<Interceptor>(interceptor))); > > } > > > > > > But that might complicate things with ResumeDelayedNetworkTimeRequests. I > think > > I now understand why you had the io_thread_helper class in the first place (to > > wrap all of this). > > Hmm, not sure if you were making a suggestion here or concluded that it wouldn't > be good because it would muddle things with ResumeDelayedNetworkTimeRequests()? > I think I could do what you're saying but would have to pass > base::Unretained(interceptor_) to ResumeDelayedNetworkTimeRequests. WDYT? Is > that better than having a test fixture method and passing > `base::Unretained(this)` as you suggested below? I started with a suggestion then realized it would muddle things with ResumeDelayedNetworkTimeRequests, precisely because you'd have to pass the interceptor as unretained there. That looks a bit awkward though, to pass the interceptor as unretained to the thread that already owns it. Instead, you could add a |ResumeDelayedNetworkTimeRequestsOnIOThread| to the interceptor, and make |Interceptor::ResumeDelayedNetworkTimeRequests| post to it, and also make the interceptor a weak pointer. It's basically pushing the thread switching to the interceptor while also making it a weak pointer. The reason I'm saying these is that this is a pattern I've seen in other chrome/browser/net code. E.g. see how link_doctor_interceptor is used in https://cs.chromium.org/chromium/src/chrome/browser/net/errorpage_browsertest... But then again, I think you might want to get a second person to review this code in addition to me, as I'm not quite familiar with the lifecycle of interceptors et al.
estark@chromium.org changed reviewers: + mmenke@chromium.org
mmenke, could you please take a look at the newly added test fixture and URLRequestInterceptor code in chrome/browser/ssl/ssl_browser_tests.cc? Mustafa and I have done a few iterations but aren't 100% sure of the right way to manage a URLRequestInterceptor that needs to be accessible on the main thread. Right now I have the test fixture creating the interceptor, and to do stuff on the IO thread I call test fixture methods and pass the fixture to the IO thread with base::Unretained. Thank you!
Didn't do a full in-depth review, just focused on correctness of the Intercept/URLRequestJob stuff. If you want me to look at the particulars, just ask. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2874: /// URLRequestJob: nit: Remove extra slash. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2887: memcpy(buf->data(), network_time::kGoodTimeResponseBody + data_offset_, Should include net/base/io_buffer.h https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2897: raw_headers.append( Terminology is confusing here, but the term "raw headers" is actually used to refer to the format output by AssembleRawHeaders, not the data passed in to it. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2916: if (!request_) Not need to check this - the request owns the URLRequestJob (Way back when, the URLRequestJob was refcounted, but that's since been fixed). https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2923: // called. Putting this in GetResponseInfo isn't right - there's no guarantee on the number of times this method will be called, when it will be called, or if it will be called at all. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2930: weak_factory_.GetWeakPtr())); You're using weak_ptrs here, but DelayedNetworkTimeInterceptor does not remove requests from its list if they are destroyed before ResumeAll is called. Should this code handle premature request destruction or not? https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2930: weak_factory_.GetWeakPtr())); Should handle jobs that haven't yet had Start() called on them yet when Resume() is called. Currently Start() is always called right after job creation, I believe, but there's no API contract about that. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2946: DelayedNetworkTimeInterceptor() : net::URLRequestInterceptor() {} net::URLRequestInterceptor() not needed. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2959: } nit: Remove braces for two-line ifs. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2967: } nit: Remove braces for two-line ifs. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2970: request->Resume(quit_closure); If no requests have been started yet, quit closure will never be called. If there are multiple requests, they'll all call quit closure, so just the first one will be guaranteed to reach the quit point before the message loop is quit. My suggestion is to either only support one delayed request, or if you need/want more, be sure to get all cases correct. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3034: interceptor_ = new DelayedNetworkTimeInterceptor(); This works. My own personal style preference would be to make interceptor_ on the UI thread, keep a raw pointer to it in the fixture, and then make these methods static (i.e., no non-static member methods of the class fixture called on the IO thread), but that's a matter of personal preference, not correctness, or officially preferred style.
Thanks, mmenke! No code changes yet, just a quick clarifying question inline. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3034: interceptor_ = new DelayedNetworkTimeInterceptor(); On 2016/11/07 15:27:45, mmenke wrote: > This works. My own personal style preference would be to make interceptor_ on > the UI thread, keep a raw pointer to it in the fixture, and then make these > methods static (i.e., no non-static member methods of the class fixture called > on the IO thread), but that's a matter of personal preference, not correctness, > or officially preferred style. If I were to do that, would I pass a WeakPtr to the interceptor when calling the static methods? Or is it okay to pass a base::Unretained pointer because the interceptor will stay alive until ClearAllHandlers is called?
https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3034: interceptor_ = new DelayedNetworkTimeInterceptor(); On 2016/11/07 17:17:19, estark wrote: > On 2016/11/07 15:27:45, mmenke wrote: > > This works. My own personal style preference would be to make interceptor_ on > > the UI thread, keep a raw pointer to it in the fixture, and then make these > > methods static (i.e., no non-static member methods of the class fixture called > > on the IO thread), but that's a matter of personal preference, not > correctness, > > or officially preferred style. > > If I were to do that, would I pass a WeakPtr to the interceptor when calling the > static methods? Or is it okay to pass a base::Unretained pointer because the > interceptor will stay alive until ClearAllHandlers is called? It would be an unretained pointer. The interceptor is guaranteed to stay alive long enough. A weak pointer would be nifty, but you can only dereference them on the thread you created them on, so you'd need to create a WeakPtr on the IO thread. You could do that, though then you'd need a PostTaskAndReplyWithResult or something to create the interceptor. Maybe there's a DetachFromThread method for weak pointers or something that I'm not remembering, though.
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...
Thanks again. I addressed mmenke's comments. One thing that came up in doing so, and this might be a question for Mustafa, is that I couldn't figure out a way to wait until the intercepted request completes, and I'm no longer even sure that we need a way to do so. What we really want to test is that if we don't crash when NetworkTimeTracker::OnURLFetchComplete runs (and runs all its callbacks) -- in particular that it doesn't crash if the SSLErrorHandler that provided one of its callbacks is gone. I'm thinking that maybe we don't need to wait until the intercepted request completes to test this situation; if the test gets torn down, the request will presumably get cancelled, which will call OnURLFetchComplete which will call NetworkTimeTracker's callbacks which would crash if the SSLErrorHandler callback wasn't being provided correctly. In other words, if the test doesn't crash before or during tear-down, then we've tested what we want to test, and we don't need to explicitly wait for the intercepted request to complete. Does that sound convincing? https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2874: /// URLRequestJob: On 2016/11/07 15:27:45, mmenke wrote: > nit: Remove extra slash. Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2887: memcpy(buf->data(), network_time::kGoodTimeResponseBody + data_offset_, On 2016/11/07 15:27:45, mmenke wrote: > Should include net/base/io_buffer.h Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2897: raw_headers.append( On 2016/11/07 15:27:45, mmenke wrote: > Terminology is confusing here, but the term "raw headers" is actually used to > refer to the format output by AssembleRawHeaders, not the data passed in to it. Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2916: if (!request_) On 2016/11/07 15:27:45, mmenke wrote: > Not need to check this - the request owns the URLRequestJob (Way back when, the > URLRequestJob was refcounted, but that's since been fixed). Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2923: // called. On 2016/11/07 15:27:45, mmenke wrote: > Putting this in GetResponseInfo isn't right - there's no guarantee on the number > of times this method will be called, when it will be called, or if it will be > called at all. I removed this callback. (See question above -- not sure what, if anything, to do instead.) https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2930: weak_factory_.GetWeakPtr())); On 2016/11/07 15:27:45, mmenke wrote: > Should handle jobs that haven't yet had Start() called on them yet when Resume() > is called. Currently Start() is always called right after job creation, I > believe, but there's no API contract about that. Done. If Resume() is called before Start(), then we now unset |delayed_| so that when Start() is called, it'll start the request immediately. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2930: weak_factory_.GetWeakPtr())); On 2016/11/07 15:27:45, mmenke wrote: > You're using weak_ptrs here, but DelayedNetworkTimeInterceptor does not remove > requests from its list if they are destroyed before ResumeAll is called. Should > this code handle premature request destruction or not? I think this should handle premature request destruction, so I changed DelayedNetworkTimeInterceptor to use a WeakPtr to its request. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2946: DelayedNetworkTimeInterceptor() : net::URLRequestInterceptor() {} On 2016/11/07 15:27:45, mmenke wrote: > net::URLRequestInterceptor() not needed. Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2959: } On 2016/11/07 15:27:45, mmenke wrote: > nit: Remove braces for two-line ifs. Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2967: } On 2016/11/07 15:27:45, mmenke wrote: > nit: Remove braces for two-line ifs. Done. https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2970: request->Resume(quit_closure); On 2016/11/07 15:27:45, mmenke wrote: > If no requests have been started yet, quit closure will never be called. If > there are multiple requests, they'll all call quit closure, so just the first > one will be guaranteed to reach the quit point before the message loop is quit. > > My suggestion is to either only support one delayed request, or if you need/want > more, be sure to get all cases correct. Done; I changed the interceptor to expect only one delayed request. (And now that the callback is being called on NetworkDelegate::OnCompleted, I think this works properly if Resume() is called before a request is intercepted. In this case, the Resume() call sets the callback on the network delegate, which will get called if a request gets intercepted later.) https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:3034: interceptor_ = new DelayedNetworkTimeInterceptor(); On 2016/11/07 17:21:41, mmenke wrote: > On 2016/11/07 17:17:19, estark wrote: > > On 2016/11/07 15:27:45, mmenke wrote: > > > This works. My own personal style preference would be to make interceptor_ > on > > > the UI thread, keep a raw pointer to it in the fixture, and then make these > > > methods static (i.e., no non-static member methods of the class fixture > called > > > on the IO thread), but that's a matter of personal preference, not > > correctness, > > > or officially preferred style. > > > > If I were to do that, would I pass a WeakPtr to the interceptor when calling > the > > static methods? Or is it okay to pass a base::Unretained pointer because the > > interceptor will stay alive until ClearAllHandlers is called? > > It would be an unretained pointer. The interceptor is guaranteed to stay alive > long enough. > > A weak pointer would be nifty, but you can only dereference them on the thread > you created them on, so you'd need to create a WeakPtr on the IO thread. You > could do that, though then you'd need a PostTaskAndReplyWithResult or something > to create the interceptor. Maybe there's a DetachFromThread method for weak > pointers or something that I'm not remembering, though. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2970: request->Resume(quit_closure); On 2016/11/07 23:24:00, estark wrote: > On 2016/11/07 15:27:45, mmenke wrote: > > If no requests have been started yet, quit closure will never be called. If > > there are multiple requests, they'll all call quit closure, so just the first > > one will be guaranteed to reach the quit point before the message loop is > quit. > > > > My suggestion is to either only support one delayed request, or if you > need/want > > more, be sure to get all cases correct. > > Done; I changed the interceptor to expect only one delayed request. (And now > that the callback is being called on NetworkDelegate::OnCompleted, I think this > works properly if Resume() is called before a request is intercepted. In this > case, the Resume() call sets the callback on the network delegate, which will > get called if a request gets intercepted later.) I don't see any waiting on NetworkDelegate::OnCompleted here, am I missing something? You could just wait until the URLRequestJob is destroyed. https://codereview.chromium.org/2449193002/diff/340001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/340001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2957: // DelayedNetworkTimeNetworkDelegate for the request. If Resume() has I don't think there's any such class as DelayedNetworkTimeNetworkDelegate?
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2970: request->Resume(quit_closure); On 2016/11/08 15:24:21, mmenke wrote: > On 2016/11/07 23:24:00, estark wrote: > > On 2016/11/07 15:27:45, mmenke wrote: > > > If no requests have been started yet, quit closure will never be called. If > > > there are multiple requests, they'll all call quit closure, so just the > first > > > one will be guaranteed to reach the quit point before the message loop is > > quit. > > > > > > My suggestion is to either only support one delayed request, or if you > > need/want > > > more, be sure to get all cases correct. > > > > Done; I changed the interceptor to expect only one delayed request. (And now > > that the callback is being called on NetworkDelegate::OnCompleted, I think > this > > works properly if Resume() is called before a request is intercepted. In this > > case, the Resume() call sets the callback on the network delegate, which will > > get called if a request gets intercepted later.) > > I don't see any waiting on NetworkDelegate::OnCompleted here, am I missing > something? You could just wait until the URLRequestJob is destroyed. Oh no, I'm sorry, I uploaded this in a weird intermediate state and must have forgotten to look it over before I sent mail. References to NetworkDelegate are removed now (I was experimenting with that for a while but it didn't work out). I could call a callback when the URLRequestJob is destroyed, but I don't think I actually need this request-completion callback at all. See my comments at the start of https://codereview.chromium.org/2449193002/#msg68. The only purpose of the callback is to ensure that nothing crashes in OnURLFetchComplete for the URLFetcher that initiated these requests... but I don't think I need to wait on anything for that, because OnURLFetchComplete will (I think) get run when the test is torn down if it hasn't been run already.
On 2016/11/08 17:54:01, estark wrote: > https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... > chrome/browser/ssl/ssl_browser_tests.cc:2970: request->Resume(quit_closure); > On 2016/11/08 15:24:21, mmenke wrote: > > On 2016/11/07 23:24:00, estark wrote: > > > On 2016/11/07 15:27:45, mmenke wrote: > > > > If no requests have been started yet, quit closure will never be called. > If > > > > there are multiple requests, they'll all call quit closure, so just the > > first > > > > one will be guaranteed to reach the quit point before the message loop is > > > quit. > > > > > > > > My suggestion is to either only support one delayed request, or if you > > > need/want > > > > more, be sure to get all cases correct. > > > > > > Done; I changed the interceptor to expect only one delayed request. (And now > > > that the callback is being called on NetworkDelegate::OnCompleted, I think > > this > > > works properly if Resume() is called before a request is intercepted. In > this > > > case, the Resume() call sets the callback on the network delegate, which > will > > > get called if a request gets intercepted later.) > > > > I don't see any waiting on NetworkDelegate::OnCompleted here, am I missing > > something? You could just wait until the URLRequestJob is destroyed. > > Oh no, I'm sorry, I uploaded this in a weird intermediate state and must have > forgotten to look it over before I sent mail. References to NetworkDelegate are > removed now (I was experimenting with that for a while but it didn't work out). > > I could call a callback when the URLRequestJob is destroyed, but I don't think I > actually need this request-completion callback at all. See my comments at the > start of https://codereview.chromium.org/2449193002/#msg68. The only purpose of > the callback is to ensure that nothing crashes in OnURLFetchComplete for the > URLFetcher that initiated these requests... but I don't think I need to wait on > anything for that, because OnURLFetchComplete will (I think) get run when the > test is torn down if it hasn't been run already. Ahh, if you don't need it, all the better... You are sure you're waiting long enough to ensure the request to be issued, right? Anyhow, I'll defer to you two on that. LGTM.
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...
On 2016/11/08 17:56:33, mmenke wrote: > On 2016/11/08 17:54:01, estark wrote: > > > https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... > > File chrome/browser/ssl/ssl_browser_tests.cc (right): > > > > > https://codereview.chromium.org/2449193002/diff/320001/chrome/browser/ssl/ssl... > > chrome/browser/ssl/ssl_browser_tests.cc:2970: request->Resume(quit_closure); > > On 2016/11/08 15:24:21, mmenke wrote: > > > On 2016/11/07 23:24:00, estark wrote: > > > > On 2016/11/07 15:27:45, mmenke wrote: > > > > > If no requests have been started yet, quit closure will never be called. > > > If > > > > > there are multiple requests, they'll all call quit closure, so just the > > > first > > > > > one will be guaranteed to reach the quit point before the message loop > is > > > > quit. > > > > > > > > > > My suggestion is to either only support one delayed request, or if you > > > > need/want > > > > > more, be sure to get all cases correct. > > > > > > > > Done; I changed the interceptor to expect only one delayed request. (And > now > > > > that the callback is being called on NetworkDelegate::OnCompleted, I think > > > this > > > > works properly if Resume() is called before a request is intercepted. In > > this > > > > case, the Resume() call sets the callback on the network delegate, which > > will > > > > get called if a request gets intercepted later.) > > > > > > I don't see any waiting on NetworkDelegate::OnCompleted here, am I missing > > > something? You could just wait until the URLRequestJob is destroyed. > > > > Oh no, I'm sorry, I uploaded this in a weird intermediate state and must have > > forgotten to look it over before I sent mail. References to NetworkDelegate > are > > removed now (I was experimenting with that for a while but it didn't work > out). > > > > I could call a callback when the URLRequestJob is destroyed, but I don't think > I > > actually need this request-completion callback at all. See my comments at the > > start of https://codereview.chromium.org/2449193002/#msg68. The only purpose > of > > the callback is to ensure that nothing crashes in OnURLFetchComplete for the > > URLFetcher that initiated these requests... but I don't think I need to wait > on > > anything for that, because OnURLFetchComplete will (I think) get run when the > > test is torn down if it hasn't been run already. > > Ahh, if you don't need it, all the better... You are sure you're waiting long > enough to ensure the request to be issued, right? Anyhow, I'll defer to you two > on that. LGTM. The request will have been issued synchronously before the interstitial is shown; to assert that in the test, I added a check where needed that the network time tracker has a request pending. Thanks for the review!
> In other words, if the test doesn't crash before or during tear-down, then we've tested > what we want to test, and we don't need to explicitly wait for the intercepted > request to complete. Does that sound convincing? I patched the CL and modified ssl_error_handler.cc to pass itself to HandleCertDateInvalidErrorImpl as unretained, and it did indeed crash some of the tests. With that, LGTM too. https://codereview.chromium.org/2449193002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2948: // DelayableNetworkTimeURLRequestJobs. Expects only to intercept only a nit: Multiple "only"s https://codereview.chromium.org/2449193002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2990: bool delay_requests_ = true; very minor nit: this sounds too much like delayed_request_. Perhaps rename to should_delay_requests_? https://codereview.chromium.org/2449193002/diff/380001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2449193002/diff/380001/components/network_tim... components/network_time/network_time_tracker.cc:167: return (param == "background-only" || param == "background-and-on-demand"); minor nit: no need for parenthesis, here and blow.
> I patched the CL and modified ssl_error_handler.cc to pass itself to > HandleCertDateInvalidErrorImpl as unretained, and it did indeed crash some of > the tests. Great, thanks so much for doing that testing and for bearing with me on this review! https://codereview.chromium.org/2449193002/diff/380001/chrome/browser/ssl/ssl... File chrome/browser/ssl/ssl_browser_tests.cc (right): https://codereview.chromium.org/2449193002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2948: // DelayableNetworkTimeURLRequestJobs. Expects only to intercept only a On 2016/11/08 19:39:41, Mustafa Emre Acer wrote: > nit: Multiple "only"s Done. https://codereview.chromium.org/2449193002/diff/380001/chrome/browser/ssl/ssl... chrome/browser/ssl/ssl_browser_tests.cc:2990: bool delay_requests_ = true; On 2016/11/08 19:39:41, Mustafa Emre Acer wrote: > very minor nit: this sounds too much like delayed_request_. Perhaps rename to > should_delay_requests_? Done. https://codereview.chromium.org/2449193002/diff/380001/components/network_tim... File components/network_time/network_time_tracker.cc (right): https://codereview.chromium.org/2449193002/diff/380001/components/network_tim... components/network_time/network_time_tracker.cc:167: return (param == "background-only" || param == "background-and-on-demand"); On 2016/11/08 19:39:41, Mustafa Emre Acer wrote: > minor nit: no need for parenthesis, here and blow. Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, meacer@chromium.org Link to the patchset: https://codereview.chromium.org/2449193002/#ps400001 (title: "meacer nits")
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 ========== Attempt an on-demand time fetch when encountering a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 ========== to ========== Attempt an on-demand time fetch when encountering a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Attempt an on-demand time fetch when encountering a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 ========== to ========== Attempt an on-demand time fetch when encountering a date invalid error This CL calls NetworkTimeTracker::StartTimeFetch() when a cert date invalid error is encountered. If network time is not available, StartTimeFetch() will kick off an on-demand time query, calling a callback when it completes. Thus, when network time is unavailable, SSLErrorHandler now waits up to 3 seconds for a time query to complete before showing an interstitial. This is motivated by the fact that network time is unavailable (either due to lost sync or due to no successful time queries) for ~25% of cert date invalid errors. 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/ CL #3: https://codereview.chromium.org/2449193002/ <== this one BUG=589700 Committed: https://crrev.com/5057f82a13bba2cd2a98539371f21188f3e28500 Cr-Commit-Position: refs/heads/master@{#430720} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/5057f82a13bba2cd2a98539371f21188f3e28500 Cr-Commit-Position: refs/heads/master@{#430720} |
