Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2628423002:
Call base::Time::NowFromSystemTime() on WM_TIMECHANGE (Closed)
DescriptionCall base::Time::NowFromSystemTime() on WM_TIMECHANGE
On Windows, base::Time only consults the system clock once every 60
seconds. Thus, prior to this CL, if the system clock changed,
base::Time::Now() would continue to reflect the old system clock for up
to 60 seconds. This was somewhat weird and caused problems for UI that
instructed the user to fix their clock. (A warning would tell the user
to fix their clock in order to make a secure connection, but after
fixing their clock and refreshing, they still would not be able to
establish a connection.)
To fix this, we now listen for WM_TIMECHANGE messages and call
NowFromSystemTime() upon receiving them. NowFromSystemTime()
reinitializes base::Time from the current system clock.
BUG=672906
TEST=Set system clock to the year 2010 and load https://google.com.
Observe an interstitial page prompting you to fix your clock. Fix your
system clock back to the true current date, and refresh. Observe that
the page loads as normal.
Review-Url: https://codereview.chromium.org/2628423002
Cr-Commit-Position: refs/heads/master@{#444228}
Committed: https://chromium.googlesource.com/chromium/src/+/06007208e1229de1a3f7ee70b5a6f2c49589951c
Patch Set 1 #Patch Set 2 : add base/time.h #Patch Set 3 : fix base/time include #
Total comments: 2
Patch Set 4 : msw comment #
Messages
Total messages: 23 (15 generated)
The CQ bit was checked by estark@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was 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.
estark@chromium.org changed reviewers: + miu@chromium.org, msw@chromium.org
msw, PTAL? Also adding miu@ as a base/time OWNER in case there is a better way to force InitializeClock(). See the CL description and/or https://bugs.chromium.org/p/chromium/issues/detail?id=672906#c3 for an explanation of why I'd like to do this. In short, when Chrome detects that the system clock is wrong, it shows an interstitial page prompting the user to fix their clock. But on Windows, fixing the clock and refreshing doesn't make the problem go away because base::Time won't immediately reflect the system clock change. Thus, this change listens for WM_TIMECHANGE messages to pick up on system clock changes. I'm totally new to Windows programming so apologies if I've done anything silly...
Description was changed from ========== Call base::Time::NowFromSystemTime() on WM_TIMECHANGE On Windows, base::Time only consults the system clock once every 60 seconds. Thus, prior to this CL, if the system clock changed, base::Time::Now() would continue to reflect the old system clock for up to 60 seconds. This was somewhat weird and caused problems for UI that instructed the user to fix their clock. (A warning would tell the user to fix their clock in order to make a secure connection, but after fixing their clock and refreshing, they still would not be able to establish a connection.) To fix this, we now listen for WM_TIMECHANGE messages and call NowFromSystemTime() upon receiving them. NowFromSystemTime() reinitializes base::Time from the current system clock. BUG=672906 ========== to ========== Call base::Time::NowFromSystemTime() on WM_TIMECHANGE On Windows, base::Time only consults the system clock once every 60 seconds. Thus, prior to this CL, if the system clock changed, base::Time::Now() would continue to reflect the old system clock for up to 60 seconds. This was somewhat weird and caused problems for UI that instructed the user to fix their clock. (A warning would tell the user to fix their clock in order to make a secure connection, but after fixing their clock and refreshing, they still would not be able to establish a connection.) To fix this, we now listen for WM_TIMECHANGE messages and call NowFromSystemTime() upon receiving them. NowFromSystemTime() reinitializes base::Time from the current system clock. BUG=672906 TEST=Set system clock to the year 2010 and load https://google.com. Observe an interstitial page prompting you to fix your clock. Fix your system clock back to the true current date, and refresh. Observe that the page loads as normal. ==========
Description was changed from ========== Call base::Time::NowFromSystemTime() on WM_TIMECHANGE On Windows, base::Time only consults the system clock once every 60 seconds. Thus, prior to this CL, if the system clock changed, base::Time::Now() would continue to reflect the old system clock for up to 60 seconds. This was somewhat weird and caused problems for UI that instructed the user to fix their clock. (A warning would tell the user to fix their clock in order to make a secure connection, but after fixing their clock and refreshing, they still would not be able to establish a connection.) To fix this, we now listen for WM_TIMECHANGE messages and call NowFromSystemTime() upon receiving them. NowFromSystemTime() reinitializes base::Time from the current system clock. BUG=672906 TEST=Set system clock to the year 2010 and load https://google.com. Observe an interstitial page prompting you to fix your clock. Fix your system clock back to the true current date, and refresh. Observe that the page loads as normal. ========== to ========== Call base::Time::NowFromSystemTime() on WM_TIMECHANGE On Windows, base::Time only consults the system clock once every 60 seconds. Thus, prior to this CL, if the system clock changed, base::Time::Now() would continue to reflect the old system clock for up to 60 seconds. This was somewhat weird and caused problems for UI that instructed the user to fix their clock. (A warning would tell the user to fix their clock in order to make a secure connection, but after fixing their clock and refreshing, they still would not be able to establish a connection.) To fix this, we now listen for WM_TIMECHANGE messages and call NowFromSystemTime() upon receiving them. NowFromSystemTime() reinitializes base::Time from the current system clock. BUG=672906 TEST=Set system clock to the year 2010 and load https://google.com. Observe an interstitial page prompting you to fix your clock. Fix your system clock back to the true current date, and refresh. Observe that the page loads as normal. ==========
lgtm with a nit. I suspect some issues occur when a base::Time user receives an incorrect value before WM_TIMECHANGE, then tries to check the elapsed time after WM_TIMECHANGE, and gets a weird result, eg. -5 years elapsed. We can really only fix issues as they arise and promote base::TimeTicks over base::Time for measuring deltas. Anyway, such issues should only happen on system time changes (infrequent), and be similar to what already occurs in that case. https://codereview.chromium.org/2628423002/diff/40001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2628423002/diff/40001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2175: // https://bugs.chromium.org/p/chromium/issues/detail?id=672906#c5 nit: shorten url: crbug.com/672906#c5 (optionally with http://)
lgtm At first, it seemed weird that "ui/views" code would be the mechanism that triggers the re-sync. However, that's the mechanism the platform provides. ;-)
The CQ bit was checked by miu@chromium.org
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 miu@chromium.org
On 2017/01/17 23:08:57, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > > https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or... Oops. Sorry for checking commit. Wrong CL.
Thanks! https://codereview.chromium.org/2628423002/diff/40001/ui/views/win/hwnd_messa... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2628423002/diff/40001/ui/views/win/hwnd_messa... ui/views/win/hwnd_message_handler.cc:2175: // https://bugs.chromium.org/p/chromium/issues/detail?id=672906#c5 On 2017/01/17 19:26:32, msw wrote: > nit: shorten url: crbug.com/672906#c5 (optionally with http://) Done.
The CQ bit was checked by estark@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, miu@chromium.org Link to the patchset: https://codereview.chromium.org/2628423002/#ps60001 (title: "msw comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484698883358640,
"parent_rev": "ce346ca4212bde35cb9628da2314de326c69ebca", "commit_rev":
"06007208e1229de1a3f7ee70b5a6f2c49589951c"}
Message was sent while issue was closed.
Description was changed from ========== Call base::Time::NowFromSystemTime() on WM_TIMECHANGE On Windows, base::Time only consults the system clock once every 60 seconds. Thus, prior to this CL, if the system clock changed, base::Time::Now() would continue to reflect the old system clock for up to 60 seconds. This was somewhat weird and caused problems for UI that instructed the user to fix their clock. (A warning would tell the user to fix their clock in order to make a secure connection, but after fixing their clock and refreshing, they still would not be able to establish a connection.) To fix this, we now listen for WM_TIMECHANGE messages and call NowFromSystemTime() upon receiving them. NowFromSystemTime() reinitializes base::Time from the current system clock. BUG=672906 TEST=Set system clock to the year 2010 and load https://google.com. Observe an interstitial page prompting you to fix your clock. Fix your system clock back to the true current date, and refresh. Observe that the page loads as normal. ========== to ========== Call base::Time::NowFromSystemTime() on WM_TIMECHANGE On Windows, base::Time only consults the system clock once every 60 seconds. Thus, prior to this CL, if the system clock changed, base::Time::Now() would continue to reflect the old system clock for up to 60 seconds. This was somewhat weird and caused problems for UI that instructed the user to fix their clock. (A warning would tell the user to fix their clock in order to make a secure connection, but after fixing their clock and refreshing, they still would not be able to establish a connection.) To fix this, we now listen for WM_TIMECHANGE messages and call NowFromSystemTime() upon receiving them. NowFromSystemTime() reinitializes base::Time from the current system clock. BUG=672906 TEST=Set system clock to the year 2010 and load https://google.com. Observe an interstitial page prompting you to fix your clock. Fix your system clock back to the true current date, and refresh. Observe that the page loads as normal. Review-Url: https://codereview.chromium.org/2628423002 Cr-Commit-Position: refs/heads/master@{#444228} Committed: https://chromium.googlesource.com/chromium/src/+/06007208e1229de1a3f7ee70b5a6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/06007208e1229de1a3f7ee70b5a6... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
