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

Status: UNCONFIRMED

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

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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:
https://dxr.mozilla.org/mozilla-central/rev/b3774461acc6bee2216c5f57e167f9e5795fb09d/widget/windows/nsWindow.cpp#298,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.

Comment 1

[Karl Tomlinson (:karlt)]

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.

Comment 2

[Karl Tomlinson (:karlt)]

GetAndClearBackwardsSkewStamp() currently assumes that there will be only a single call with aPostTime == sLastPostTime.