Open Bug 1332187 Opened 4 years ago Updated 4 years ago

Is it necessary to check |currentTime == sLastPostTime| in CurrentWindowsTimeGetter::GetTimeAsyncForPossibleBackwardsSkew()?


(Core :: Widget: Win32, defect, P3)






(Reporter: masayuki, Unassigned)



(Whiteboard: tpi:+)

When we tried to fix bug 1256562, we have a question. Why CurrentWindowsTimeGetter::GetTimeAsyncForPossibleBackwardsSkew() checks |currentTime == sLastPostTime|, is it really necessary?

The code is here:,301
>   void GetTimeAsyncForPossibleBackwardsSkew(const TimeStamp& aNow)
>   {
>     DWORD currentTime = GetCurrentTime();
>     if (sBackwardsSkewStamp && currentTime == sLastPostTime) {
>       // There's already one inflight with this timestamp. Don't
>       // send a duplicate.
>       return;
>     }
>     sBackwardsSkewStamp = Some(aNow);
>     sLastPostTime = currentTime;
>     static_assert(sizeof(WPARAM) >= sizeof(DWORD), "Can't fit a DWORD in a WPARAM");
>     ::PostMessage(mWnd, MOZ_WM_SKEWFIX, sLastPostTime, 0);
>   }

Looks like this won't become true but if it's not so, I'd love to explain the reason with comment.
Priority: -- → P3
Whiteboard: tpi:+
Although kBackwardsSkewCheckInterval can limit calls to GetTimeAsyncForPossibleBackwardsSkew(), that test is performed using times from events, which may be queued, and so it is theoretically possible that GetTimeAsyncForPossibleBackwardsSkew() is called more than once in a short period.

Perhaps it is not essential to keep the test (I haven't thought through carefully or checked history), but it still seems useful to keep the GetTimeAsyncForPossibleBackwardsSkew() implementation robust against potential changes to SystemTimeConverter.
GetAndClearBackwardsSkewStamp() currently assumes that there will be only a single call with aPostTime == sLastPostTime.
You need to log in before you can comment on or make changes to this bug.