Closed Bug 1234838 Opened 8 years ago Closed 8 years ago

Update windows backwards-time-skew fixing code to use PostMessage

Categories

(Core :: Widget: Win32, defect)

46 Branch
Unspecified
Windows 10
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- unaffected
firefox46 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

In bug 1222190 I landed an implementation of GetTimeAsyncForPossibleBackwardsSkew in the windows widget that used NS_DispatchToMainThread. However as karl pointed out in bug 1222190 comment 33 this has a few issues. This follow-up bug is to update the code to use ::PostMessage instead, which should resolve the issues.
Attached patch Patch (obsolete) — Splinter Review
So calling PostMessage with a nullptr HWND didn't seem to work, it never got the message back. It might be possible to listen for it elsewhere; I'm not familiar with the API either. However I found a simpler fix - the GetMessageTimeStamp function doesn't need to be static, so I can access the mWnd and send it in.

Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=7429c8fe55bc
Attachment #8701538 - Flags: review?(karlt)
Comment on attachment 8701538 [details] [diff] [review]
Patch

This looks like the right kind of thing, but I suspect the single global sBackwardsSkewStamp variable will come into some trouble with dropped messages.
If someone more familiar with the message processing can be sure that messages won't be dropped, then this is good as is.

>+    sBackwardsSkewStamp = Some(aNow);
>+    ::PostMessage(mWnd, MOZ_WM_SKEWFIX, 0, 0);

https://hg.mozilla.org/mozilla-central/rev/3015d5cb3a9c#l1.51 implies that
some messages can get dropped.  We didn't have that problem with the GTK implementation.  I think we need to get Jim to look over this if we use this approach.  Either ensure that messages will be received, or that
new messages can be sent after situations that cause messages to be dropped.

Another way I imagine the message can be dropped is if the window is destroyed
before the message is processed.  The GTK implementation addressed that by
making the window own the CurrentWindowsTimeGetter and each getter keep track of a time stamp for the message in flight on the window.  If a thread message queue could be used, then that would be another solution, but I don't know whether or not that would address the modal window / third party event loop situation.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #1)
> So calling PostMessage with a nullptr HWND didn't seem to work, it never got
> the message back. It might be possible to listen for it elsewhere; I'm not
> familiar with the API either.

I wonder whether nsAppShell::ProcessNextNativeEvent() received the message but
::DispatchMessageW() did not pass it on to the window procedure.

https://msdn.microsoft.com/en-us/library/windows/desktop/ms644927%28v=vs.85%29.aspx#loop
Attachment #8701538 - Flags: review?(karlt) → review-
Fair enough. I'll update the patch so that new messages can still be sent after a certain timeout if something gets lost. That seems more robust than trying to catch every case where the message gets dropped.
Perhaps kBackwardsSkewCheckInterval is a long enough timeout, the most recent message can be used instead of the oldest, and an integer in the message can be used to confirm that it is the one corresponding to the timestamp.  Or maybe the whole timestamp can be split into the two integers message - I don't know whether or not TimeStamp supports conversion to int64_t and back.
Attached patch Patch v2 (obsolete) — Splinter Review
Sorry for the delay. Here's the updated patch which allows a new request to clobber an old request. It also uses the post time as an id so that if a clobbered request does eventually come in it is ignored.
Attachment #8701538 - Attachment is obsolete: true
Attachment #8706502 - Flags: review?(karlt)
Comment on attachment 8706502 [details] [diff] [review]
Patch v2

>+  Time
>+  GetBackwardsSkewCheckInterval() const {
>+    return kBackwardsSkewCheckInterval;
>+  }

>+    if (sBackwardsSkewStamp) {
>+      // There's already one inflight
>+      if (GetCurrentTime() - sLastPostTime <= TimeConverter().GetBackwardsSkewCheckInterval()) {
>+        // ... and it's recent. Don't send another one.
>+        return;
>+      }
>+      // Otherwise, it's an old request; the PostMessage for it may have
>+      // gotten dropped. Kick off a new request.
>+    }

GetTimeStampFromSystemTime() already performs a similar check for us to limit
the number of times this will be called, so I wouldn't bother with
kBackwardsSkewCheckInterval again here.

https://dxr.mozilla.org/mozilla-central/rev/5acc2a44834ce0614f98466475e674517daf0041/widget/SystemTimeConverter.h#111

The only remaining need for this check is so that the new value of
sLastPostTime will not match more than one message leaving
GetAndClearBackwardsSkewStamp() to find an empty sBackwardsSkewStamp.

That could be addressed by merely checking that GetCurrentTime() !=
sLastPostTime, but the current patch works too so keep its logic if you
prefer.

>+    sLastPostTime = GetCurrentTime();

However, whatever you choose, please only call GetCurrentTime() once in the
function and store the value in a local variable.

>+  static bool GetAndClearBackwardsSkewStamp(DWORD aPostTime, TimeStamp& aOutSkewStamp)

Please use a pointer for out-parameter aOutSkewStamp in line with gecko style
guidelines.  No need to null-check or assert aOutSkewStamp.
Attachment #8706502 - Flags: review?(karlt) → review+
Attached patch Patch v3Splinter Review
Update to address review comments. I ended up doing the sLastPostTime != currentTime check like you suggested since it meant I didn't need to expose the kBackwardsSkewCheckInterval value from SystemTimeConverter.
Attachment #8706502 - Attachment is obsolete: true
Attachment #8706980 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/d081f975bf17
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: