Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1953393002:
Stop using WaitMessage in MessagePumpForUI::WaitForWork (Closed)
DescriptionStop using WaitMessage in MessagePumpForUI::WaitForWork
When WaitMessage is called it ignores the current delay
timeout and might end up waiting an arbitrary time. See the
bug for the explanation.
The fix is to loop back and call MsgWaitForMultipleObjectEx
again with the remaining timeout.
BUG=610005
Committed: https://crrev.com/05e1178680e47f4263f9c7087dad1d8e099a5027
Cr-Commit-Position: refs/heads/master@{#393375}
Patch Set 1 : #
Total comments: 2
Messages
Total messages: 19 (7 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== WaitMessage fix Instrumentation BUG= ========== to ========== Stop using WaitMessage in MessagePumpForUI::WaitForWork When WaitMessage is called it ignores the current delay timeout and might end up waiting an arbitrary time. See the bug for the explanation. The fix is to loop back and call MsgWaitForMultipleObjectEx again with the remaining timeout. BUG=610005 ==========
stanisc@chromium.org changed reviewers: + danakj@chromium.org
PTAL!
danakj@chromium.org changed reviewers: + brucedawson@chromium.org, jbauman@chromium.org
+jbauman,brucedawson
lgtm
https://codereview.chromium.org/1953393002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1953393002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_win.cc:226: while ((delay = GetCurrentDelay()) != 0) { The old code when delay was 0 would call MsgWaitForMultipleObjectsEx still and just return immediately regardless, which would cause |result| to be set, and if WAIT_OBJECT_0 was the result, it would wait for a new message. The new code will return if delay is 0. Is this intentional, why?
https://codereview.chromium.org/1953393002/diff/20001/base/message_loop/messa... File base/message_loop/message_pump_win.cc (right): https://codereview.chromium.org/1953393002/diff/20001/base/message_loop/messa... base/message_loop/message_pump_win.cc:226: while ((delay = GetCurrentDelay()) != 0) { On 2016/05/11 01:13:36, danakj wrote: > The old code when delay was 0 would call MsgWaitForMultipleObjectsEx still and > just return immediately regardless, which would cause |result| to be set, and if > WAIT_OBJECT_0 was the result, it would wait for a new message. > > The new code will return if delay is 0. Is this intentional, why? You are right. This is a suble difference with the previous implementation that I forgot to mention. If the delay is 0 MsgWaitForMultupleObjectsEx would return promptly with either WAIT_OBJECT_0 or WAIT_TIMEOUT. The zero delay means that a pending delayed task is overdue. The right way to handle that is to exit WaitForWork and iterate through the loop allowing the delayed task to be done. The previous implementation could theoretically enter WaitMessage and get stuck there for a while (if there was a pending message belonging to another thread) which in my opinion isn't the right thing to do. Another difference of this version with the previous version is that it would also enter MsgWaitForMultupleObjectsEx repeatedly if a previous call to MsgWaitForMultupleObjectsEx ended with WAIT_TIMEOUT too early. In this situation we know for sure that there is nothing to do - no messages to dispatch, no new work, and no delayed work due. So going back to wait saves the trouble of reloading the work queue.
lgtm
Ping! I hope this affects crbug.com/596190 which is Pri 1 bug.
danakj@, I need an owner approval. Could you take another look?
On 2016/05/12 20:41:36, stanisc wrote: > danakj@, I need an owner approval. Could you take another look? Oh sorry I missed your reply. LGTM, thanks for the explanation.
The CQ bit was checked by stanisc@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953393002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953393002/20001
Message was sent while issue was closed.
Description was changed from ========== Stop using WaitMessage in MessagePumpForUI::WaitForWork When WaitMessage is called it ignores the current delay timeout and might end up waiting an arbitrary time. See the bug for the explanation. The fix is to loop back and call MsgWaitForMultipleObjectEx again with the remaining timeout. BUG=610005 ========== to ========== Stop using WaitMessage in MessagePumpForUI::WaitForWork When WaitMessage is called it ignores the current delay timeout and might end up waiting an arbitrary time. See the bug for the explanation. The fix is to loop back and call MsgWaitForMultipleObjectEx again with the remaining timeout. BUG=610005 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Stop using WaitMessage in MessagePumpForUI::WaitForWork When WaitMessage is called it ignores the current delay timeout and might end up waiting an arbitrary time. See the bug for the explanation. The fix is to loop back and call MsgWaitForMultipleObjectEx again with the remaining timeout. BUG=610005 ========== to ========== Stop using WaitMessage in MessagePumpForUI::WaitForWork When WaitMessage is called it ignores the current delay timeout and might end up waiting an arbitrary time. See the bug for the explanation. The fix is to loop back and call MsgWaitForMultipleObjectEx again with the remaining timeout. BUG=610005 Committed: https://crrev.com/05e1178680e47f4263f9c7087dad1d8e099a5027 Cr-Commit-Position: refs/heads/master@{#393375} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/05e1178680e47f4263f9c7087dad1d8e099a5027 Cr-Commit-Position: refs/heads/master@{#393375} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
