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)
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)
5.58 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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-
Assignee | ||
Comment 3•8 years ago
|
||
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.
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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 6•8 years ago
|
||
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+
Assignee | ||
Comment 7•8 years ago
|
||
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+
Comment 9•8 years ago
|
||
bugherder |
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.
Description
•