Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2839213002:
[Synchronization] Fix a crash in WaitableEventWatcher (Closed)
Description[Synchronization] Fix a crash in WaitableEventWatcher.
This CL fix a problem with closing windows event handle when some watcher is
still waiting for that event. The MSDN says that the behaviour is 'undefined',
and it crashes on Win7.
Now WaitableEventWatcher duplicates handle and holds it until watching is
finished.
TEST=base_unittests/WaitableEventWatcherTest*
BUG=704495
Review-Url: https://codereview.chromium.org/2839213002
Cr-Commit-Position: refs/heads/master@{#470502}
Committed: https://chromium.googlesource.com/chromium/src/+/54d831564ddf3b5f69adee4dfc35e75cb8c419d3
Patch Set 1 #Patch Set 2 : [Synchronization] Fix a crash in WaitableEventWatcher. #
Total comments: 18
Patch Set 3 : Review fixes #Patch Set 4 : The sleep is removed in tests #Patch Set 5 : Check the both variants. Change comments. #
Total comments: 13
Patch Set 6 : Rewrite some comments #
Total comments: 9
Patch Set 7 : The last nits is fixed #
Total comments: 1
Patch Set 8 : Rewrite the tests #
Total comments: 2
Patch Set 9 : The sleep() is returned back. #
Total comments: 3
Patch Set 10 : Fix grammar in comments #
Messages
Total messages: 37 (15 generated)
Description was changed from ========== [Synchronization] Fix a crash in WaitableEventWatcher. This CL fix a problem with closing windows event handle when some watcher is still waiting for that event. The MSDN says that the behaviour is 'undefined', and it crashes on Win7. Now WaitableEventWatcher duplicates handle and holds it until watching is finished. TEST=base_unittests/WaitableEventWatcherTest* BUG= ========== to ========== [Synchronization] Fix a crash in WaitableEventWatcher. This CL fix a problem with closing windows event handle when some watcher is still waiting for that event. The MSDN says that the behaviour is 'undefined', and it crashes on Win7. Now WaitableEventWatcher duplicates handle and holds it until watching is finished. TEST=base_unittests/WaitableEventWatcherTest* BUG= ==========
atuchin@yandex-team.ru changed reviewers: + danakj@chromium.org
On 2017/04/26 15:58:01, atuchin wrote:
> mailto:atuchin@yandex-team.ru changed reviewers:
> + mailto:danakj@chromium.org
Hi!
Despite the comments in original code, if you close an event handle when
RegisterWaitForSingleObject is in progress,
the program may crash on Win7 with exception 0xc000070a
(STATUS_THREADPOOL_HANDLE_EXCEPTION) and stack:
0. ntdll.dll@_TppWaiterpThread
1. kernel32.dll@BaseThreadInitThunk
2. ntdll.dll@__RtlUserThreadStart
3. ntdll.dll@_RtlUserThreadStart
I also has a strongly suspicious that the problem is connected with a bunch of
utility crashes with similar stacks.
Here is an another short example that shows the problem. It crashes on Win7
(Release x86, MS VC 2015):
#include "windows.h"
#include <iostream>
void CALLBACK DoneWaiting(void* param, BOOLEAN timed_out) {
std::cout << "callback\n";
}
int main() {
for (int i = 0; i < 100; i++) {
auto handle = ::CreateEvent(nullptr, TRUE, FALSE, nullptr);
HANDLE wait_object = nullptr;
const DWORD wait_flags = WT_EXECUTEINWAITTHREAD | WT_EXECUTEONLYONCE;
::RegisterWaitForSingleObject(&wait_object, handle, DoneWaiting, nullptr,
INFINITE, wait_flags);
::SetEvent(handle);
::CloseHandle(handle);
std::cout << "CloseHandle\n";
}
return 0;
}
Description was changed from ========== [Synchronization] Fix a crash in WaitableEventWatcher. This CL fix a problem with closing windows event handle when some watcher is still waiting for that event. The MSDN says that the behaviour is 'undefined', and it crashes on Win7. Now WaitableEventWatcher duplicates handle and holds it until watching is finished. TEST=base_unittests/WaitableEventWatcherTest* BUG= ========== to ========== [Synchronization] Fix a crash in WaitableEventWatcher. This CL fix a problem with closing windows event handle when some watcher is still waiting for that event. The MSDN says that the behaviour is 'undefined', and it crashes on Win7. Now WaitableEventWatcher duplicates handle and holds it until watching is finished. TEST=base_unittests/WaitableEventWatcherTest* BUG=704495 ==========
scottmg@chromium.org changed reviewers: + scottmg@chromium.org
Nice find atuchin! (I added BUG=704495 to the description)
https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher.h (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher.h:60: // it with a Watcher. But pay attention: if the event was signaled and deleted ISTM that all WaitableEventWatchers' EventCallbacks are only using the WaitableEvent* passed to it for pointer comparison. This is good as it could be deleted as this comment says. What do you think of changing it to a void*? https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:130: auto event = base::MakeUnique<WaitableEvent>( needs ptr_util.h https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:137: PlatformThread::Sleep(TimeDelta::FromMilliseconds(10)); What is this sleep meant to do? https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:158: PlatformThread::Sleep(TimeDelta::FromMilliseconds(10)); Also this sleep? https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_win.cc (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_win.cc:16: // Explicitly run StopWatching to avoid dependency on the members nit: StopWatching() https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_win.cc:17: // destruction order (|event_handle_| must outlive working |watcher_|). Normally I'd just put a comment in the header file on watcher_ saying something like "Makes use of |event_handle_| which must outlive it." instead of writing code in the destructor. Would that work? https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_win.cc:29: HANDLE event_handle = nullptr; Can we avoid having a member and local var with the same name other than then underscore? Maybe duplicate_handle_ for the member?
On 2017/04/27 17:14:43, scottmg wrote: > Nice find atuchin! (I added BUG=704495 to the description) Thanks! But unfortunately, I have no access to it.
Thanks for comments. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher.h (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher.h:60: // it with a Watcher. But pay attention: if the event was signaled and deleted On 2017/04/27 20:01:08, danakj wrote: > ISTM that all WaitableEventWatchers' EventCallbacks are only using the > WaitableEvent* passed to it for pointer comparison. This is good as it could be > deleted as this comment says. What do you think of changing it to a void*? As for me it is better to remove that param at all. I looked on several use cases and found only DCHECKs. I chose to not modify WaitableEventWatcher interface in that CL (it isn't related with the crash). Can you create bug/task to do that? https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:130: auto event = base::MakeUnique<WaitableEvent>( On 2017/04/27 20:01:08, danakj wrote: > needs ptr_util.h Done. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:137: PlatformThread::Sleep(TimeDelta::FromMilliseconds(10)); On 2017/04/27 20:01:08, danakj wrote: > What is this sleep meant to do? Unfortunately, without that sleep the test fails to reproduce the race. My suggestion: the crash happens when started thread in win thread pool failed tries to look a invalidated handle. And that sleep changes the execution order between the current thread and the win thread pool thread (who watches for the handle). Yes, putting sleep in tests don't make me happy, but I don't know an another way to catch the race here. We cannot rely on windows internals (and all works fine on win10). My approach is to check both variants (with sleep() and without). I added a comment. May be you suggest an another? https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:158: PlatformThread::Sleep(TimeDelta::FromMilliseconds(10)); On 2017/04/27 20:01:08, danakj wrote: > Also this sleep? See the answer in another one. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_win.cc (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_win.cc:16: // Explicitly run StopWatching to avoid dependency on the members On 2017/04/27 20:01:08, danakj wrote: > nit: StopWatching() Done. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_win.cc:17: // destruction order (|event_handle_| must outlive working |watcher_|). On 2017/04/27 20:01:08, danakj wrote: > Normally I'd just put a comment in the header file on watcher_ saying something > like "Makes use of |event_handle_| which must outlive it." instead of writing > code in the destructor. Would that work? Ok, removed that code and added comments to header. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_win.cc:29: HANDLE event_handle = nullptr; On 2017/04/27 20:01:08, danakj wrote: > Can we avoid having a member and local var with the same name other than then > underscore? Maybe duplicate_handle_ for the member? Ok, I renamed: local event_handle -> handle event_handle_ -> duplicated_event_handle_
The CQ bit was checked by atuchin@yandex-team.ru to run a CQ dry run
The CQ bit was unchecked by atuchin@yandex-team.ru
https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher.h (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher.h:60: // it with a Watcher. But pay attention: if the event was signaled and deleted On 2017/04/28 06:44:29, atuchin wrote: > On 2017/04/27 20:01:08, danakj wrote: > > ISTM that all WaitableEventWatchers' EventCallbacks are only using the > > WaitableEvent* passed to it for pointer comparison. This is good as it could > be > > deleted as this comment says. What do you think of changing it to a void*? > > As for me it is better to remove that param at all. > I looked on several use cases and found only DCHECKs. > > I chose to not modify WaitableEventWatcher interface in that CL (it isn't > related with the crash). > Can you create bug/task to do that? I would like to do it together because the pointer was safe to use before but now it's not, if you think its reasonable. Keeping the DCHECKs doesn't seem harmful and they'd work with a void* too. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:137: PlatformThread::Sleep(TimeDelta::FromMilliseconds(10)); On 2017/04/28 06:44:29, atuchin wrote: > On 2017/04/27 20:01:08, danakj wrote: > > What is this sleep meant to do? > > Unfortunately, without that sleep the test fails to reproduce the race. > > My suggestion: the crash happens when started thread in win thread pool failed > tries to look a invalidated handle. IIUC we don't have the ability to run code on this thread? Else we could have it signal the test to continue. What if you make 2 WaitableEvents, and delete the first when the 2nd is signalled.. or vice versa? Maybe that misses the race? > And that sleep changes the execution order between the current thread and the > win thread pool thread (who watches for the handle). > > Yes, putting sleep in tests don't make me happy, but I don't know an another way > to catch the race here. > We cannot rely on windows internals (and all works fine on win10). > > My approach is to check both variants (with sleep() and without). I added a > comment. > May be you suggest an another?
https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher.h (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher.h:60: // it with a Watcher. But pay attention: if the event was signaled and deleted On 2017/04/28 17:54:49, danakj wrote: > I would like to do it together because the pointer was safe to use before but > now it's not, if you think its reasonable. Keeping the DCHECKs doesn't seem > harmful and they'd work with a void* too. Unfortunately, the pointer wasn't safe before. As you can see I don't change the behaviour of WaitableEventWatcher. I change only watching handle. And using void* here doesn't look a good solution for me because after deleting such pointers are not unique (a new WaitableEvent may have the same address). That why I would like to not change it here. https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/20001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:137: PlatformThread::Sleep(TimeDelta::FromMilliseconds(10)); On 2017/04/28 17:54:49, danakj wrote: > IIUC we don't have the ability to run code on this thread? Else we could have it > signal the test to continue. What if you make 2 WaitableEvents, and delete the > first when the 2nd is signalled.. or vice versa? Maybe that misses the race? Yes, we don't have a such ability. But I have found another solution. The problem with crash reproduction in the test is connected with dtor of WaitableEventWatcher (it calls UnregisterWaitEx on the handle: ~WaitableEventWatcher() -> ~ObjectWatcher() -> StopWatching() -> UnregisterWaitEx()). I moved dtor away from the current stack and tests started to work fine without sleep (crashed on Win7 without the fix). That approach didn't work for my short example above, but now I think it was because the test program immediately shuts down after the handle manipulations (I should add waiting after). Thanks!
https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... File base/synchronization/waitable_event_watcher.h (right): https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher.h:98: // A watcher for |duplicated_event_handle_|. It MUST outlive that handle. The order is backwards here, watcher_ is created after, and destroyed before the handle_ if it appears below it in the class. https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:129: // of the test) nit: . https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:143: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, nit: SequencedTaskRunnerHandle https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:153: // callstack. (Calling the dtor can make a difference for the behavior It would be nice if you instead explained how it makes a difference, what ordering it changes. Like if you do this then something else occurs before the destructor is deleted etc. https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:154: // of the test) nit: . https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:169: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, nit: SequencedTaskRunnerHandle
https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... File base/synchronization/waitable_event_watcher.h (right): https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher.h:98: // A watcher for |duplicated_event_handle_|. It MUST outlive that handle. On 2017/05/02 14:46:21, danakj wrote: > The order is backwards here, watcher_ is created after, and destroyed before the > handle_ if it appears below it in the class. The comment was wrong: of course the handle must outlive the watcher. Thanks! https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:129: // of the test) On 2017/05/02 14:46:22, danakj wrote: > nit: . Done. https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:143: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, On 2017/05/02 14:46:22, danakj wrote: > nit: SequencedTaskRunnerHandle Done. https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:153: // callstack. (Calling the dtor can make a difference for the behavior On 2017/05/02 14:46:21, danakj wrote: > It would be nice if you instead explained how it makes a difference, what > ordering it changes. Like if you do this then something else occurs before the > destructor is deleted etc. Ok, I rewrote the comment, trying to avoid platform specific details (because the tests is common for Windows and POSIX). https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:154: // of the test) On 2017/05/02 14:46:22, danakj wrote: > nit: . Done. https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:169: base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, On 2017/05/02 14:46:22, danakj wrote: > nit: SequencedTaskRunnerHandle Done.
Thanks, the comments are better now. LGTM w/ a few spelling/grammer fixes. https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/80001/base/synchronization/wa... base/synchronization/waitable_event_watcher_unittest.cc:153: // callstack. (Calling the dtor can make a difference for the behavior On 2017/05/03 13:01:58, atuchin wrote: > On 2017/05/02 14:46:21, danakj wrote: > > It would be nice if you instead explained how it makes a difference, what > > ordering it changes. Like if you do this then something else occurs before the > > destructor is deleted etc. > Ok, I rewrote the comment, trying to avoid platform specific details (because > the tests is common for Windows and POSIX). FWIW, IMO it's fine to say "On windows, ..." in a test comment. https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:127: // If |deferred_watcher_delete| is false the watcher will be deleted right deletion https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:128: // after deleting the event, which leads to immediately cancel the wait cancelling https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:130: // If |deferred_watcher_delete| is true the watcher will be deleted deletion https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:131: // in another callstack. That checks that the watcher can live with deleted with a deleted https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:155: // If |deferred_watcher_delete| is true the watcher will be deleted in another this sentence is a duplicate from below https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:157: // If |deferred_watcher_delete| is false the watcher will be deleted right deletion https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:158: // after deleting the event, which leads to immediately cancel the wait cancelling https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:160: // If |deferred_watcher_delete| is true the watcher will be deleted deletion https://codereview.chromium.org/2839213002/diff/100001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:161: // in another callstack. That checks that the watcher can live with deleted with a deleted
Ok, I fixed the last nits. Thanks for the review!
The CQ bit was checked by atuchin@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2839213002/#ps120001 (title: "The last nits is fixed")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by atuchin@yandex-team.ru
Houston, we have a problem=) Unfortunately, DeleteSoon for the watcher was an illusion. The message loop wasn't in the running state and all posted tasks are simply ignored. As the result |watcher| is never destroyed. I have found another approach and have rewritten the SignalAndDelete test. Now it usually catches the race on my single-core VirtualBox. For a multiple core system I should run the test multiple time before it fails. Even with added sleep()... Of course after adding a loop with enough iterations it fails on each run. But it's a unit test, not a stress.. What do you think about it? In conclusion, I should admit that its hard to writes a good test that stably reproduces the race on a multicore system. Do we really have a Win7 x32 test agents to run such tests? Or maybe the current tests coverage (with the added test) is enough?
https://codereview.chromium.org/2839213002/diff/120001/base/synchronization/w... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/120001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:149: } What if you RunLoop().Run() here to let the watcher delete, and also post it with a delay? https://codereview.chromium.org/2839213002/diff/140001/base/synchronization/w... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/140001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:132: auto event = new WaitableEvent(WaitableEvent::ResetPolicy::AUTOMATIC, auto* https://codereview.chromium.org/2839213002/diff/140001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:156: // Wait for the watcher callback: nit: s/:/./
On Thu, May 4, 2017 at 1:02 PM, <atuchin@yandex-team.ru> wrote: > > > 23:09, May 4, 2017, "danakj@chromium.org" <danakj@chromium.org>: > > https://codereview.chromium.org/2839213002/diff/120001/ > base/synchronization/waitable_event_watcher_unittest.cc#newcode149 > base/synchronization/waitable_event_watcher_unittest.cc:149: } > What if you RunLoop().Run() here to let the watcher delete, and also > post it with a delay? > > Sounds like a solution, but what is the difference from sleep()? > I tried to post it without a delay, of course it didn't work. > I was wondering if RunLoop::Run() would make this fail but still clean up correctly. Using the delay would then help make it less flaky, giving the OS thread some time to run? I guess it's really no different than sleep. Maybe we should go with the sleep and a comment explaining there is a thread under OS control we are racing with and trying to give it a chance to run etc? > -- > Mikhail Atuchin, > http://staff.yandex-team.ru/atuchin > Sent from Yandex.Mail for mobile -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/05/04 17:17:04, danakj wrote: > On Thu, May 4, 2017 at 1:02 PM, <mailto:atuchin@yandex-team.ru> wrote: > > > > > > > 23:09, May 4, 2017, mailto:"danakj@chromium.org" <mailto:danakj@chromium.org>: > > > > https://codereview.chromium.org/2839213002/diff/120001/ > > base/synchronization/waitable_event_watcher_unittest.cc#newcode149 > > base/synchronization/waitable_event_watcher_unittest.cc:149: } > > What if you RunLoop().Run() here to let the watcher delete, and also > > post it with a delay? > > > > Sounds like a solution, but what is the difference from sleep()? > > I tried to post it without a delay, of course it didn't work. > > > > I was wondering if RunLoop::Run() would make this fail but still clean up > correctly. Using the delay would then help make it less flaky, giving the > OS thread some time to run? I guess it's really no different than sleep. > Maybe we should go with the sleep and a comment explaining there is a > thread under OS control we are racing with and trying to give it a chance > to run etc? Yes, the sleep improves the chance to catch the race. RunLoop::Run() allows the test to clean up correctly but without delay it doesn't fail. Ok, I have returned the sleep back and added comments. PTAL. > > > -- > > Mikhail Atuchin, > > http://staff.yandex-team.ru/atuchin > > Sent from Yandex.Mail for mobile > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
LGTM https://codereview.chromium.org/2839213002/diff/160001/base/synchronization/w... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/160001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:142: // Unfortunately, that thread is under OS control and some can't and we can't https://codereview.chromium.org/2839213002/diff/160001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:172: // Unfortunately, that thread is under OS control and some can't and we can't
https://codereview.chromium.org/2839213002/diff/160001/base/synchronization/w... File base/synchronization/waitable_event_watcher_unittest.cc (right): https://codereview.chromium.org/2839213002/diff/160001/base/synchronization/w... base/synchronization/waitable_event_watcher_unittest.cc:138: if (delay_after_delete) { Oh, maybe it's worth but I'm not clear what the point is to run the test with this as false. Is it going to catch something we wouldn't with it set to true?
On Fri, May 5, 2017 at 2:26 PM, <atuchin@yandex-team.ru> wrote: > > > 21:38, May 5, 2017, "danakj@chromium.org" <danakj@chromium.org>: > > > https://codereview.chromium.org/2839213002/diff/160001/ > base/synchronization/waitable_event_watcher_unittest.cc#newcode138 > base/synchronization/waitable_event_watcher_unittest.cc:138: if > (delay_after_delete) { > Oh, maybe it's worth but I'm not clear what the point is to run the test > with this as false. Is it going to catch something we wouldn't with it > set to true? > > Why not? As we saw before, it can make sense. For example, the watcher > dtor calls UnregisterWaitEx, which behavior depends on the background > thread state. > Inaddition, the DeleteAndSignal test never calls that winapi function, > because it waits for the callback. > Ok yeah sounds good, thanks! > -- > Mikhail Atuchin > http://staff.yandex-team.ru/atuchin > Sent from Yandex.Mail for mobile > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by atuchin@yandex-team.ru
The CQ bit was unchecked by atuchin@yandex-team.ru
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by atuchin@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2839213002/#ps180001 (title: "Fix grammar in comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1494388609376950,
"parent_rev": "7a5ce801c1afa20f33bca1d8f6b059d20b76fc40", "commit_rev":
"54d831564ddf3b5f69adee4dfc35e75cb8c419d3"}
Message was sent while issue was closed.
Description was changed from ========== [Synchronization] Fix a crash in WaitableEventWatcher. This CL fix a problem with closing windows event handle when some watcher is still waiting for that event. The MSDN says that the behaviour is 'undefined', and it crashes on Win7. Now WaitableEventWatcher duplicates handle and holds it until watching is finished. TEST=base_unittests/WaitableEventWatcherTest* BUG=704495 ========== to ========== [Synchronization] Fix a crash in WaitableEventWatcher. This CL fix a problem with closing windows event handle when some watcher is still waiting for that event. The MSDN says that the behaviour is 'undefined', and it crashes on Win7. Now WaitableEventWatcher duplicates handle and holds it until watching is finished. TEST=base_unittests/WaitableEventWatcherTest* BUG=704495 Review-Url: https://codereview.chromium.org/2839213002 Cr-Commit-Position: refs/heads/master@{#470502} Committed: https://chromium.googlesource.com/chromium/src/+/54d831564ddf3b5f69adee4dfc35... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/54d831564ddf3b5f69adee4dfc35... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
