|
|
Created:
4 years, 5 months ago by maksims (do not use this acc) Modified:
3 years, 12 months ago CC:
chromium-reviews, certificate-transparency-chrome_googlegroups.com, cbentzel+watch_chromium.org, Eran Messeri, Paweł Hajdan Jr., kinuko+cache_chromium.org, gavinp+disk_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake callers of FromUTC(Local)Exploded in net/ use new time API.
Use new time conversion API in accordance with
https://codereview.chromium.org/1988663002/
BUG=601905, 601903, 601900
Committed: https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648
Cr-Commit-Position: refs/heads/master@{#439789}
Patch Set 1 #Patch Set 2 : //net #Patch Set 3 : rebased #
Total comments: 1
Patch Set 4 : Make callers of FromUTC(Local)Exploded in net/ use new time API. #
Total comments: 4
Patch Set 5 : revert #
Total comments: 9
Patch Set 6 : Comments from eroman@ #
Total comments: 10
Patch Set 7 : commetns from eroman #Patch Set 8 : fix typo #Patch Set 9 : ... #Patch Set 10 : revert ftp_util #
Total comments: 4
Patch Set 11 : remove redundant line #
Total comments: 4
Patch Set 12 : fix X509 parse date test on 32-bit machines #Patch Set 13 : fix accidently changed line while rebasing - use UTCExploded instead of LocalExploded #Patch Set 14 : fix accidently changed line while rebasing - use UTCExploded instead of LocalExploded #Patch Set 15 : use EXPECT_TRUE #
Total comments: 6
Patch Set 16 : comments #Patch Set 17 : typo #
Total comments: 2
Patch Set 18 : undo comment change #Messages
Total messages: 132 (107 generated)
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== net time callers BUG= ========== to ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905, 601900 ==========
Description was changed from ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905, 601900 ========== to ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601900 ==========
Patchset #2 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601900 ========== to ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601903,601900 ==========
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by maksim.sisov@intel.com 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_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 maksim.sisov@intel.com to run a CQ dry run
The CQ bit was unchecked by maksim.sisov@intel.com
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 maksim.sisov@intel.com 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 maksim.sisov@intel.com 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: Exceeded global retry quota
maksim.sisov@intel.com changed reviewers: + eroman@chromium.org, mmenke@chromium.org
eroman@, mmenke@, basically, this cl had issues as well. I've just forgotten about this.
The CQ bit was checked by maksim.sisov@intel.com 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/2090713003/diff/160001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/160001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:288: // We don't know the time zone of the listing, so just use local time. Problem is with this one. The guess is bad, actually... https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:288: // We don't know the time zone of the listing, so try both. So, problem was here - it had been using a bad guess... It's better to try both functions. Then, I guess, it might fix another bug, that has been reported here - https://bugs.chromium.org/p/chromium/issues/detail?id=641096
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:288: // We don't know the time zone of the listing, so try both. On 2016/10/05 12:59:02, maksims wrote: > So, problem was here - it had been using a bad guess... It's better to try both > functions. Then, I guess, it might fix another bug, that has been reported here > - https://bugs.chromium.org/p/chromium/issues/detail?id=641096 Why is one succeeding, but the other failing? Seems like ExplodedMostlyEquals(to_exploded, exploded) shouldn't depend on time zone.
https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:288: // We don't know the time zone of the listing, so try both. On 2016/10/05 16:53:38, mmenke wrote: > On 2016/10/05 12:59:02, maksims wrote: > > So, problem was here - it had been using a bad guess... It's better to try > both > > functions. Then, I guess, it might fix another bug, that has been reported > here > > - https://bugs.chromium.org/p/chromium/issues/detail?id=641096 > > Why is one succeeding, but the other failing? Seems like > ExplodedMostlyEquals(to_exploded, exploded) shouldn't depend on time zone. ExplodedMostlyEquals() doesn't depend on that. The problem is with decoding times back to exploded. For some reason, hours are lost. It seems like that original solution wasn't that precise.
https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/180001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:288: // We don't know the time zone of the listing, so try both. On 2016/10/06 12:07:07, maksims wrote: > On 2016/10/05 16:53:38, mmenke wrote: > > On 2016/10/05 12:59:02, maksims wrote: > > > So, problem was here - it had been using a bad guess... It's better to try > > both > > > functions. Then, I guess, it might fix another bug, that has been reported > > here > > > - https://bugs.chromium.org/p/chromium/issues/detail?id=641096 > > > > Why is one succeeding, but the other failing? Seems like > > ExplodedMostlyEquals(to_exploded, exploded) shouldn't depend on time zone. > > ExplodedMostlyEquals() doesn't depend on that. The problem is with decoding > times back to exploded. For some reason, hours are lost. It seems like that > original solution wasn't that precise. I'm not comfortable with workarounding around the issue without some understanding of it first. If it's just that a local time of January 1, 1970 can be too early when we try and create a base::Time from it, this solution seems reasonable (And possibly not testable). If it's something else, it may be that there's some other bug we're running into.
The CQ bit was checked by maksim.sisov@intel.com 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 maksim.sisov@intel.com 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...
Patchset #5 (id:200001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by maksim.sisov@intel.com 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 maksim.sisov@intel.com 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...
Patchset #7 (id:260001) has been deleted
Patchset #6 (id:240001) has been deleted
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
In general I am supportive of the changes https://codereview.chromium.org/2090713003/diff/220001/net/cert/cert_verify_p... File net/cert/cert_verify_proc.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/cert_verify_p... net/cert/cert_verify_proc.cc:697: if (!base::Time::FromUTCExploded({2012, 7, 0, 1, 0, 0, 0, 0}, This translation is not the same -- before FromUTCExploded() was run a single time, whereas now it is run on each invocation of the function. And in fact neither is correct, explained the problem here: https://bugs.chromium.org/p/chromium/issues/detail?id=657903 I will fix this (https://codereview.chromium.org/2441713002/) so won't require any changes in this CL. https://codereview.chromium.org/2090713003/diff/220001/net/cert/ct_policy_enf... File net/cert/ct_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/ct_policy_enf... net/cert/ct_policy_enforcer_unittest.cc:448: EXPECT_TRUE(base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}, This pattern is common enough in this file that I suggest creating a helper function that assumes these will all succeed (since it makes it less readable modeling these as failing each time). base::Time CreateTime(...) { base::Time result; if (!base::Time::FromUTCExploded(..., &result)) { ADD_FAILURE() << "Failed FromUTCExploded"; } return result; } https://codereview.chromium.org/2090713003/diff/220001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:225: base::Time::FromUTCExploded(test_data_.expected_result, &out_time)); Based on the comment above, I wouldn't expect this to always succeed (for some of the test inputs shouldn't this fail?) https://codereview.chromium.org/2090713003/diff/220001/net/cookies/cookie_uti... File net/cookies/cookie_util.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cookies/cookie_uti... net/cookies/cookie_util.cc:203: if (exploded.day_of_month >= 1 && exploded.day_of_month <= 31 && Can you remove this "if" statement? FromUTCExploded() should already be doing this check internally. https://codereview.chromium.org/2090713003/diff/220001/net/disk_cache/blockfi... File net/disk_cache/blockfile/eviction.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/disk_cache/blockfi... net/disk_cache/blockfile/eviction.cc:284: bool conversion_success = Time::FromLocalExploded(old, &out_time); The existing code is not great. My suggestion is to re-write this using a hardcoded timestamp, see for instance: https://codereview.chromium.org/2441713002/ Note when doing the conversion that base::Time::FromInternalValue takes the time from the windows epoch (note unix one). The easiest approach is to just run the old code in a test and call ToInternalValue() to see what it should be, and then use that as a literal value in the code.
The CQ bit was checked by maksim.sisov@intel.com 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_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 maksim.sisov@intel.com 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...
Patchset #6 (id:280001) has been deleted
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_...)
mmenke@, eroman@, there is a problem with Time::FromExploded. Especially with the following line - https://cs.chromium.org/chromium/src/base/time/time_posix.cc?sq=package:chrom... SysTimeFromTimeStruct changes tm_isdst to 0 for some reason, when FtpBrowserTest.DirectoryListingNavigation is executed, and then ExplodedMostlyEquals() fails, because a day becomes old + 1 day. I've even tried to hard-code the following structure (base::Time::Exploded exploded = {2016, 9, 0, 26, 21, 5, 0, 0};) to the FromExploded and execute with different tests in order to see if it succeeds or not. And I found out that when FtpBrowserTest.DirectoryListingNavigation is executed, SysTimeFromTimeStruct sets tm_isdst to 0, and ExplodedMostlyEquals fails, but when other tests are executed, tm_isdst is set to 1 and ExplodedMostlyEquals succeeds with the same exploded time I mentioned above. https://codereview.chromium.org/2090713003/diff/220001/net/cert/ct_policy_enf... File net/cert/ct_policy_enforcer_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/ct_policy_enf... net/cert/ct_policy_enforcer_unittest.cc:448: EXPECT_TRUE(base::Time::FromUTCExploded({2015, 3, 0, 25, 11, 25, 0, 0}, On 2016/10/20 18:05:21, eroman wrote: > This pattern is common enough in this file that I suggest creating a helper > function that assumes these will all succeed (since it makes it less readable > modeling these as failing each time). > > base::Time CreateTime(...) { > base::Time result; > if (!base::Time::FromUTCExploded(..., &result)) { > ADD_FAILURE() << "Failed FromUTCExploded"; > } > return result; > } Done. https://codereview.chromium.org/2090713003/diff/220001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:225: base::Time::FromUTCExploded(test_data_.expected_result, &out_time)); On 2016/10/20 18:05:21, eroman wrote: > Based on the comment above, I wouldn't expect this to always succeed (for some > of the test inputs shouldn't this fail?) No, it must always succeed. There is a check above that stops execution if time is not valid, otherwise all the supplied times from the CertDateTestData are correct and must succeed - if (!test_data_.is_valid) return; https://codereview.chromium.org/2090713003/diff/220001/net/disk_cache/blockfi... File net/disk_cache/blockfile/eviction.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/disk_cache/blockfi... net/disk_cache/blockfile/eviction.cc:284: bool conversion_success = Time::FromLocalExploded(old, &out_time); On 2016/10/20 18:05:21, eroman wrote: > The existing code is not great. My suggestion is to re-write this using a > hardcoded timestamp, see for instance: > https://codereview.chromium.org/2441713002/ > > Note when doing the conversion that base::Time::FromInternalValue takes the time > from the windows epoch (note unix one). > > The easiest approach is to just run the old code in a test and call > ToInternalValue() to see what it should be, and then use that as a literal value > in the code. Done.
looks good after these comments. I didn't quite understand your question regarding SysTimeFromTimeStruct -- could you phrase that a bit differently for me? https://codereview.chromium.org/2090713003/diff/220001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/220001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:225: base::Time::FromUTCExploded(test_data_.expected_result, &out_time)); On 2016/10/26 08:16:40, maksims wrote: > On 2016/10/20 18:05:21, eroman wrote: > > Based on the comment above, I wouldn't expect this to always succeed (for some > > of the test inputs shouldn't this fail?) > > No, it must always succeed. There is a check above that stops execution if > time is not valid, otherwise all the supplied times from the CertDateTestData > are correct and must succeed - > > if (!test_data_.is_valid) > return; The test data includes this test case: {2049, 1, 0, 1, 0, 0, 0}}, The test data is valid, so we should not be early returning. Instead I would expect execution to reach FromUTCExploded() on that date (2049). And that conversion will surely fail, since 32-bit POSIX only goes up to 2038. I have gone ahead and sent this to a 32-bit Linux trybot (linux_chromium_dbg_32_ng), we will see what happens... https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); This line is redundant with the call to FromUTCExploded() below right? https://codereview.chromium.org/2090713003/diff/300001/net/disk_cache/blockfi... File net/disk_cache/blockfile/eviction.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/disk_cache/blockfi... net/disk_cache/blockfile/eviction.cc:281: header_->create_time = time_2009_3_1.ToInternalValue(); For the record -- this is a bit different from what was happening before (it is now using a fixed time stamp, whereas before it was using a timestamp based on the user's timezone). However I believe using a fixed timestamp as being done now is the correct approach. Also I wonder if we should simplify this to: // 2009/03/01 (midnight UTC) header_->create_time = 12985574400000000; .. But on the other h and the documentation for |create_time| is pretty bad and mentions nothing about the granularity or epoch of the timestamp, so perhaps just using base::Time's "internal value" as you have done here is more consistent with the surrounding code. https://codereview.chromium.org/2090713003/diff/300001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:289: if (!base::Time::FromLocalExploded(time_exploded, result)) { [optional] I suggest simplifying this to: return base::Time::FromLocalExploded(...); https://codereview.chromium.org/2090713003/diff/300001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:354: if (!base::Time::FromLocalExploded(time_exploded, result)) { Same
The CQ bit was checked by maksim.sisov@intel.com 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_...) linux_chromium_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 maksim.sisov@intel.com 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 maksim.sisov@intel.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by maksim.sisov@intel.com 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_...)
On 2016/10/27 21:44:35, eroman (slow) wrote: > looks good after these comments. > > I didn't quite understand your question regarding SysTimeFromTimeStruct -- could > you phrase that a bit differently for me? > For some reason Time::FromExploded fails when FtpBrowserTest.DirectoryListingNavigation is run. Time is converted strangely - hours becomes +2 after conversion is done. I cannot figure out why it happens.
I believe the problem you are running into has to do with the sandbox on Linux. I can reproduce the same thing when parsing local dates (but only in that browser_test). In my case the time is off by 7 hours, which is my offset from GMT. So perhaps under the sandbox it is failing to properly determine the local timezone. I confirmed that running browser_tests with --no-sandbox resolves the issue. I'll move that discussion to a separate bug thread, since that is something we will need to resolve or FromLocalExploded() is pretty broken from renderer process.
The CQ bit was checked by maksim.sisov@intel.com 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/2090713003/diff/300001/net/cert/x509_cert_typ... File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); On 2016/10/27 21:44:34, eroman (vacation - Nov 14) wrote: > This line is redundant with the call to FromUTCExploded() below right? No. FromUTCExploded doesn't perform this kind of check. https://codereview.chromium.org/2090713003/diff/300001/net/disk_cache/blockfi... File net/disk_cache/blockfile/eviction.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/disk_cache/blockfi... net/disk_cache/blockfile/eviction.cc:281: header_->create_time = time_2009_3_1.ToInternalValue(); On 2016/10/27 21:44:34, eroman (vacation - Nov 14) wrote: > For the record -- this is a bit different from what was happening before (it is > now using a fixed time stamp, whereas before it was using a timestamp based on > the user's timezone). > > However I believe using a fixed timestamp as being done now is the correct > approach. > > Also I wonder if we should simplify this to: > > // 2009/03/01 (midnight UTC) > header_->create_time = 12985574400000000; > > .. But on the other h and the documentation for |create_time| is pretty bad and > mentions nothing about the granularity or epoch of the timestamp, so perhaps > just using base::Time's "internal value" as you have done here is more > consistent with the surrounding code. Ok, thanks. Let it be like this. https://codereview.chromium.org/2090713003/diff/300001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:289: if (!base::Time::FromLocalExploded(time_exploded, result)) { On 2016/10/27 21:44:34, eroman (vacation - Nov 14) wrote: > [optional] I suggest simplifying this to: > > return base::Time::FromLocalExploded(...); Done. https://codereview.chromium.org/2090713003/diff/300001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:354: if (!base::Time::FromLocalExploded(time_exploded, result)) { On 2016/10/27 21:44:34, eroman (vacation - Nov 14) wrote: > Same Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:290: return true; eroman@, can we leave these parts as they are for now?
https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); On 2016/11/29 07:38:56, maksims wrote: > On 2016/10/27 21:44:34, eroman (vacation - Nov 14) wrote: > > This line is redundant with the call to FromUTCExploded() below right? > > No. FromUTCExploded doesn't perform this kind of check. FromUTCExploded() returns a boolean indicating success. It shouldn't be possible for it to return true for inputs where !HasValidValues(), hence this is redundant. If you are aware of a situation where this is necessary please raise it as a bug, since it means we aren't properly checking errors in FromUTCExploded() https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:290: return true; On 2016/11/29 08:41:18, maksims wrote: > eroman@, can we leave these parts as they are for now? Sure leave it as is -- you can disable the failing browser_test on Linux and point to crbug.com/641096 or crbug.com/666535. I need to follow-up on on that.
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/300001/net/cert/x509_cert_typ... net/cert/x509_cert_types.cc:74: valid &= exploded.HasValidValues(); On 2016/11/30 00:21:05, eroman (slow) wrote: > On 2016/11/29 07:38:56, maksims wrote: > > On 2016/10/27 21:44:34, eroman (vacation - Nov 14) wrote: > > > This line is redundant with the call to FromUTCExploded() below right? > > > > No. FromUTCExploded doesn't perform this kind of check. > > FromUTCExploded() returns a boolean indicating success. It shouldn't be possible > for it to return true for inputs where !HasValidValues(), hence this is > redundant. > > If you are aware of a situation where this is necessary please raise it as a > bug, since it means we aren't properly checking errors in FromUTCExploded() Sorry, I was wrong. it's ok to remove this line. https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:290: return true; On 2016/11/30 00:21:05, eroman (slow) wrote: > On 2016/11/29 08:41:18, maksims wrote: > > eroman@, can we leave these parts as they are for now? > > Sure leave it as is -- you can disable the failing browser_test on Linux and > point to crbug.com/641096 or crbug.com/666535. I need to follow-up on on that. If I leave it as is here, the test is not failing. I will subscribe to those issues as well and fix these lines here once the problem is sorted.
https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc File net/ftp/ftp_util.cc (right): https://codereview.chromium.org/2090713003/diff/380001/net/ftp/ftp_util.cc#ne... net/ftp/ftp_util.cc:290: return true; On 2016/12/05 13:15:49, maksims wrote: > On 2016/11/30 00:21:05, eroman (slow) wrote: > > On 2016/11/29 08:41:18, maksims wrote: > > > eroman@, can we leave these parts as they are for now? > > > > Sure leave it as is -- you can disable the failing browser_test on Linux and > > point to crbug.com/641096 or crbug.com/666535. I need to follow-up on on that. > > If I leave it as is here, the test is not failing. I will subscribe to those > issues as well and fix these lines here once the problem is sorted. Sure, sounds good! https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... net/cert/x509_cert_types.cc:77: // FromUTCExploded() can fail even though exploded.HasValidValues() This comment is no longer relevant, please remove https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:225: base::Time::FromUTCExploded(test_data_.expected_result, &out_time)); Please see my earlier comments on this matter. This cannot work, as dates that are too big to work on 32-bit systems will result in ParseCertificateDate() failing, and not truncating here. See this (32-bit) trybot failure: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... This will likely need to be addressed by making changes to the test itself (if time_t is 32-bits, then expect a failure from ParseCertificateDate for the affected data).
The CQ bit was checked by maksim.sisov@intel.com to run a CQ dry run
https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... File net/cert/x509_cert_types.cc (right): https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... net/cert/x509_cert_types.cc:77: // FromUTCExploded() can fail even though exploded.HasValidValues() On 2016/12/08 01:39:19, eroman (slow) wrote: > This comment is no longer relevant, please remove Done. https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/400001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:225: base::Time::FromUTCExploded(test_data_.expected_result, &out_time)); On 2016/12/08 01:39:19, eroman (slow) wrote: > Please see my earlier comments on this matter. > > This cannot work, as dates that are too big to work on 32-bit systems will > result in ParseCertificateDate() failing, and not truncating here. > > See this (32-bit) trybot failure: > > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... > > This will likely need to be addressed by making changes to the test itself (if > time_t is 32-bits, then expect a failure from ParseCertificateDate for the > affected data). Done.
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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com 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_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by maksim.sisov@intel.com 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by maksim.sisov@intel.com 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/2090713003/diff/480001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) Won't "out_time.ToInternalValue() <= max_time.ToInternalValue()" is always true, since the max_value is just std::numeric_limits<int64_t>::max() See my comment below -- I don't think we need this helper. https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:230: if (sizeof(time_t) <= 4 && Rather than the ExplodedExceedsBitsLimit() function, how about the following (no helper function, and no extra if/else other than at the start. base::Time parsed_date; bool parsed = ParseCertificateDate(...); if (!parsed && test_data.is_valid && test_data_.expected_result.year >= 2038 && sizeof(time_t) == 4 ) { // Some of the valid test data will fail on 32-bit POSIX systems return; } if (!test_data_.is_valid) return; <check expected_result against parsed_date using essentially what existing code does>
sisov.maksim@gmail.com changed reviewers: + sisov.maksim@gmail.com
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) On 2016/12/17 02:39:30, eroman (slow) wrote: > Won't "out_time.ToInternalValue() <= max_time.ToInternalValue()" is always true, > since the max_value is just std::numeric_limits<int64_t>::max() > > See my comment below -- I don't think we need this helper. Nope, on 32-bit build int64_t resolves to 2^32. And this function helps to make sure 32bits value still not overflow and resolve to 0 https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:230: if (sizeof(time_t) <= 4 && On 2016/12/17 02:39:30, eroman (slow) wrote: > Rather than the ExplodedExceedsBitsLimit() function, how about the following (no > helper function, and no extra if/else other than at the start. > > base::Time parsed_date; > bool parsed = ParseCertificateDate(...); > > if (!parsed && test_data.is_valid && test_data_.expected_result.year >= 2038 && > sizeof(time_t) == 4 ) { > // Some of the valid test data will fail on 32-bit POSIX systems > return; > } > > if (!test_data_.is_valid) > return; > > <check expected_result against parsed_date using essentially what existing code > does> See my comment above
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) On 2016/12/19 15:45:13, maksims_ wrote: > On 2016/12/17 02:39:30, eroman (slow) wrote: > > Won't "out_time.ToInternalValue() <= max_time.ToInternalValue()" is always > true, > > since the max_value is just std::numeric_limits<int64_t>::max() > > > > See my comment below -- I don't think we need this helper. > > Nope, on 32-bit build int64_t resolves to 2^32. And this function helps to make > sure 32bits value still not overflow and resolve to 0 These are both int64_t values, this isn't going to change whether compiled on 32-bit or 64-bit build.
https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... File net/cert/x509_cert_types_unittest.cc (right): https://codereview.chromium.org/2090713003/diff/480001/net/cert/x509_cert_typ... net/cert/x509_cert_types_unittest.cc:213: if (converted && out_time.ToInternalValue() <= max_time.ToInternalValue()) On 2016/12/19 19:22:31, eroman (slow) wrote: > On 2016/12/19 15:45:13, maksims_ wrote: > > On 2016/12/17 02:39:30, eroman (slow) wrote: > > > Won't "out_time.ToInternalValue() <= max_time.ToInternalValue()" is always > > true, > > > since the max_value is just std::numeric_limits<int64_t>::max() > > > > > > See my comment below -- I don't think we need this helper. > > > > Nope, on 32-bit build int64_t resolves to 2^32. And this function helps to > make > > sure 32bits value still not overflow and resolve to 0 > > These are both int64_t values, this isn't going to change whether compiled on > 32-bit or 64-bit build. Ok, thx. Done.
The CQ bit was checked by maksim.sisov@intel.com 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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by maksim.sisov@intel.com 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.
lgtm https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_... File net/ftp/ftp_directory_listing_parser_vms.cc (right): https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_... net/ftp/ftp_directory_listing_parser_vms.cc:195: // We don't know the time zone of the server, so just use local UTC. Please undo this comment change.
The CQ bit was checked by maksim.sisov@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org Link to the patchset: https://codereview.chromium.org/2090713003/#ps540001 (title: "undo comment change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_... File net/ftp/ftp_directory_listing_parser_vms.cc (right): https://codereview.chromium.org/2090713003/diff/520001/net/ftp/ftp_directory_... net/ftp/ftp_directory_listing_parser_vms.cc:195: // We don't know the time zone of the server, so just use local UTC. On 2016/12/19 22:42:35, eroman (slow) wrote: > Please undo this comment change. Done.
CQ is committing da patch. Bot data: {"patchset_id": 540001, "attempt_start_ts": 1482237148006660, "parent_rev": "1c437630a8f474cd35b2531601b580b81fae449f", "commit_rev": "e2301d87cdf3dd2d7f45264ebb637b08820a39ea"}
Message was sent while issue was closed.
Description was changed from ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601903,601900 ========== to ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601903,601900 Review-Url: https://codereview.chromium.org/2090713003 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:540001)
Message was sent while issue was closed.
Description was changed from ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601903,601900 Review-Url: https://codereview.chromium.org/2090713003 ========== to ========== Make callers of FromUTC(Local)Exploded in net/ use new time API. Use new time conversion API in accordance with https://codereview.chromium.org/1988663002/ BUG=601905,601903,601900 Committed: https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648 Cr-Commit-Position: refs/heads/master@{#439789} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/c8c6cd130d570c7b269e6c10e6716631afd3a648 Cr-Commit-Position: refs/heads/master@{#439789} |