Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8659040:
There is a racing between SyncSocket::Receive in audio_thread_ and SyncSocket::Close in renderer ... (Closed)
|
Created:
9 years ago by no longer working on chromium Modified:
9 years ago Reviewers:
nfullagar, tommi (sloooow) - chröme, Raymond Toy (Google), enal1, henrika (OOO until Aug 14), enal, brettw, scherkus (not reviewing) CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org, Chris Rogers Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionThere is a racing between SyncSocket::Receive in audio_thread_ and SyncSocket::Close in renderer thread.
This patch fixes it by using a waitable event to signal the audio thread that it should stop.
Test=content_unittests by running Valgrind
BUG=103711
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113386
Patch Set 1 #
Total comments: 13
Patch Set 2 : new proposal from timmi #
Total comments: 13
Patch Set 3 : update #
Messages
Total messages: 40 (0 generated)
Hi all, This CL is trying to fix a racing condition between SyncSocket::Close and SyncSocket::Receive when we are calling Audio(Input)DeviceStop(). This race has been there before, but we did not have any unittest to address it before the WebRTCAudioDeviceTest. The problem can be reproduce by running Valgrind with command: sh tools/valgrind/chrome_tests.sh --tool tsan -t content_unittests --gtest_filter=*WebRTC* Please review. Thanks, BR, /SX
http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.cc:29: audio_event_(true, false) { Can you please explain what exactly arguments to event constructor mean... http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.cc:215: while (!audio_event_.IsSignaled() && I don't fully understand how event helps in a case when thread is blocked on socket wait. I.e. suggested solution works if there is consistent stream of data coming in, but not for case when there is no data... http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.h:169: base::WaitableEvent audio_event_; Maybe include "stop" in the name? Something like "audio_thread_should_stop"?
http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.cc:29: audio_event_(true, false) { I think it's ok to set |manual_reset| to false in this case (first argument). http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.cc:189: audio_event_.Reset(); if you set manual_reset to false, you don't have to do this step. http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.cc:215: while (!audio_event_.IsSignaled() && On 2011/11/29 16:14:48, enal1 wrote: > I don't fully understand how event helps in a case when thread is blocked on > socket wait. I.e. suggested solution works if there is consistent stream of data > coming in, but not for case when there is no data... good point! Is there a way to read from the socket asynchronously? http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... File content/renderer/media/audio_device.h (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.h:169: base::WaitableEvent audio_event_; audio_event_ -> stop_event_ or stop_audio_event_
drive-by for threading alert. http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:292: while (!audio_event_.IsSignaled() && I'm not super-familiar with the audio codebase, but this *looks like* a fundamentally broken idiom. The return value of IsSignaled() is irrelevant *immediately* after it returns (before the && is eval'd) because no locks are held. I can't tell from the CL description, but it *sounds* like you think SyncSocket can't be used "to perform rudimentary cross-process synchronization with low latency" (from sync_socket.h) without a data race, which sounds like a bug in SyncSocket. Can you fix it there?
http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:292: while (!audio_event_.IsSignaled() && Hey Ami, A couple of questions inline: On 2011/11/29 17:48:32, Ami Fischman wrote: > I'm not super-familiar with the audio codebase, but this *looks like* a > fundamentally broken idiom. > The return value of IsSignaled() is irrelevant *immediately* after it returns > (before the && is eval'd) because no locks are held. Can you explain this in a bit more detail? No locks should be needed since if the event is signaled, the loop breaks. The event object is itself, thread safe. > > I can't tell from the CL description, but it *sounds* like you think SyncSocket > can't be used "to perform rudimentary cross-process synchronization with low > latency" (from sync_socket.h) without a data race, which sounds like a bug in > SyncSocket. Can you fix it there? The race is that the socket handle is being closed and set to invalid from one thread while another is waiting on it. Looking closer at the code though, I see that there is a check that Start() is always called on the IO thread but there is no such check for Stop(). Is it simply missing or can Stop() be called from a different thread than the IO thread? If so, then we still have a race between Start and Stop, but if Stop is always called from the same thread as Start, then I think the event object fixes the race. Then there's also the problem that was pointed out if no data is sent to the socket - which could cause a hang.
http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:102: LOG(ERROR) << "Failed to shut down audio input on IO thread"; This is incorrect code - quoting WaitableEvent::TimedWait's .h comment: // If this method returns false, then it // does not necessarily mean that max_time was exceeded. I only mention it for context on Tommi's other comment about Stop(). http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:292: while (!audio_event_.IsSignaled() && On 2011/11/29 20:11:32, tommi wrote: > Can you explain this in a bit more detail? No locks should be needed since if > the event is signaled, the loop breaks. The event object is itself, thread > safe. The problem is with the other case; IsSignaled() may return false and immediately thereafter a signal comes in (before the subsequent Receive is run). IIUC the bug this CL is trying to fix, you're back to square one in this case. > The race is that the socket handle is being closed and set to invalid from one > thread while another is waiting on it. What does "set to invalid" mean? Merely reading on one thread and closing on another thread should work fine (in vanilla socket(2)). If SyncSocket doesn't support that that's a bug IMO. Can anyone repro a tsan alert on a SyncSocket test (not using Audio*Device)? > Looking closer at the code though, I see that there is a check that Start() is > always called on the IO thread but there is no such check for Stop(). Start() posts to the IO thread. Stop() posts to the IO thread for shutdown and then waits for completion before returning. Neither check that they are "always called on the IO thread". > Then there's also the problem that was pointed out if no data is sent to the > socket - which could cause a hang. This goes to the crux of my point about fixing races in SyncSocket; SyncSocket is a synchronization abstraction which makes me strongly doubt it's possible to work *around* bugs in it from the outside without giving up serious latency concessions.
I am not specialist in sockets, but is it possible to send something to it from the same process, not from different one? Then you can just send negative value and thread will stop automatically... Otherwise you are back to square one trying to simulate functionality of WaitForMultilpleObjects()... On Tue, Nov 29, 2011 at 12:38 PM, <fischman@chromium.org> wrote: > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > File content/renderer/media/audio_input_device.cc (right): > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:102: LOG(ERROR) << "Failed > to shut down audio input on IO thread"; > This is incorrect code - quoting WaitableEvent::TimedWait's .h comment: > // If this method returns false, then it > // does not necessarily mean that max_time was exceeded. > > I only mention it for context on Tommi's other comment about Stop(). > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:292: while > (!audio_event_.IsSignaled() && > On 2011/11/29 20:11:32, tommi wrote: >> >> Can you explain this in a bit more detail? No locks should be needed > > since if >> >> the event is signaled, the loop breaks. The event object is itself, > > thread >> >> safe. > > > The problem is with the other case; IsSignaled() may return false and > immediately thereafter a signal comes in (before the subsequent Receive > is run). > IIUC the bug this CL is trying to fix, you're back to square one in this > case. > >> The race is that the socket handle is being closed and set to invalid > > from one >> >> thread while another is waiting on it. > > > What does "set to invalid" mean? > Merely reading on one thread and closing on another thread should work > fine (in vanilla socket(2)). If SyncSocket doesn't support that that's > a bug IMO. > Can anyone repro a tsan alert on a SyncSocket test (not using > Audio*Device)? > >> Looking closer at the code though, I see that there is a check that > > Start() is >> >> always called on the IO thread but there is no such check for Stop(). > > > Start() posts to the IO thread. Stop() posts to the IO thread for > shutdown and then waits for completion before returning. Neither check > that they are "always called on the IO thread". > >> Then there's also the problem that was pointed out if no data is sent > > to the >> >> socket - which could cause a hang. > > > This goes to the crux of my point about fixing races in SyncSocket; > SyncSocket is a synchronization abstraction which makes me strongly > doubt it's possible to work *around* bugs in it from the outside without > giving up serious latency concessions. > > http://codereview.chromium.org/8659040/
On 2011/11/29 20:45:08, enal1 wrote: > I am not specialist in sockets, but is it possible to send something > to it from the same process, not from different one? Then you can just > send negative value and thread will stop automatically... Socket pairs don't care about processes. They only care about their pair-partners. Writing to one of the pair results in reading from the other, and vice versa. If you're asking whether it's possible to write to one half of the pair and read the data back from that same half, then the answer is no. It is possible to simulate the above through a custom ping-pong protocol, by sending magical values and checking for them on the other side, and then mirroring them back to the sender. But that seems like crazy talk to me, not knowing what the bug being fixed really is.
Then I'd say proper fix would be to pass second half of the pair through all the constructors, proxies, delegates, and glue code, and just send negative value when stopping. On Tue, Nov 29, 2011 at 12:49 PM, <fischman@chromium.org> wrote: > On 2011/11/29 20:45:08, enal1 wrote: >> >> I am not specialist in sockets, but is it possible to send something >> to it from the same process, not from different one? Then you can just >> send negative value and thread will stop automatically... > > > Socket pairs don't care about processes. They only care about their > pair-partners. Writing to one of the pair results in reading from the > other, > and vice versa. > If you're asking whether it's possible to write to one half of the pair and > read > the data back from that same half, then the answer is no. > > It is possible to simulate the above through a custom ping-pong protocol, by > sending magical values and checking for them on the other side, and then > mirroring them back to the sender. But that seems like crazy talk to me, > not > knowing what the bug being fixed really is. > > http://codereview.chromium.org/8659040/
On 2011/11/29 21:05:26, enal1 wrote: > Then I'd say proper fix would be to pass second half of the pair > through all the constructors, proxies, delegates, and glue code, and > just send negative value when stopping. I still don't know what's being fixed, so I don't have an opinion on your suggestion :)
i just looked at the Windows base::SyncSocket implementation. It definitely can crash if one thread deletes the object while other waits on base::Receive(). I believe I fixed such bug in the past... In this case we can hit exactly such a problem if AudioDevice::ShutDownOnIOThread() would not finish in 1 second. Other problem would happen if other side of the connection would not receive AudioHostMsg_CloseStream or react to it, so socket would not be closed. Then both AudioDevice::Run() and would audio_thread_->Join() would wait forever. I believe native client recently saw exactly such situation... Thanks, Eugene On Tue, Nov 29, 2011 at 1:06 PM, <fischman@chromium.org> wrote: > On 2011/11/29 21:05:26, enal1 wrote: >> >> Then I'd say proper fix would be to pass second half of the pair >> through all the constructors, proxies, delegates, and glue code, and >> just send negative value when stopping. > > > I still don't know what's being fixed, so I don't have an opinion on your > suggestion :) > > > http://codereview.chromium.org/8659040/
Re-sending adding Nicholas, he may give more details about native client problem... On Tue, Nov 29, 2011 at 1:29 PM, Eugene Nalimov <enal@google.com> wrote: > i just looked at the Windows base::SyncSocket implementation. It > definitely can crash if one thread deletes the object while other > waits on base::Receive(). I believe I fixed such bug in the past... > > In this case we can hit exactly such a problem if > AudioDevice::ShutDownOnIOThread() would not finish in 1 second. > > Other problem would happen if other side of the connection would not > receive AudioHostMsg_CloseStream or react to it, so socket would not > be closed. Then both AudioDevice::Run() and would > audio_thread_->Join() would wait forever. I believe native client > recently saw exactly such situation... > > Thanks, > Eugene > > On Tue, Nov 29, 2011 at 1:06 PM, <fischman@chromium.org> wrote: >> On 2011/11/29 21:05:26, enal1 wrote: >>> >>> Then I'd say proper fix would be to pass second half of the pair >>> through all the constructors, proxies, delegates, and glue code, and >>> just send negative value when stopping. >> >> >> I still don't know what's being fixed, so I don't have an opinion on your >> suggestion :) >> >> >> http://codereview.chromium.org/8659040/
So, a few notes, some going back aways: SyncSocket was added way back when, as a method for trusted code to wake up a sleeping thread in untrusted code. It is actually more feature than we need, but NaCl system runtime already supported sockets, so it was easier to add SyncSocket than a new primitive such as a waitable event. The untrusted code (Native Client application) does a blocking read on the socket. When chrome needed more audio data, it would scoop out the samples in the shared memory buffer, then drop another word into the sync socket, then waking up the sleeping nacl thread, which then goes and fills in the shm with the next audio buffer. The sync-socket is one-way communication, so the untrusted code should not be able to control chrome's behavior. Untrusted code can write anything it wants into the shared memory, any time it wants, and can (or perhaps be evil and decide not to) do blocking reads on the sync socket. If Chrome wrote a -1 to the SyncSocket, this indicated to the untrusted code that it is time to quit the audio loop and join the audio thread (but evil code could decide to ignore this and do something else...) If Chrome should fail to write to the SyncSocket, then the other side has been closed (terminated) and Chrome should assume that audio playback should be stopped. (looking at AudioSyncReader::UpdatePendingBytes it doesn't look like this is happening now?) We took a lot of care to avoid any other system calls in the Native Client side, to avoid swapping out the audio thread and disrupting the low latency delivery. But it looks like on the Chrome / trusted side, there are still a lot of mutex locks, yields, and critical sections going on, which, if on the back end audio thread, could swap it out, causing hard-to-track-down glitches in audio playback when smaller buffers are used. Much of this depends on the OS, number and frequency of cpu cores and latency (buffer size) We don't have any evidence just yet, but are concerned some older machines (that used to work at a given latency) might now have trouble consistently hitting real-time requirements if too many locks are going on. The best way is to do both front and back end with no locking primitives at all, if possible. There's also the race conditions during setup and tear-down, and these have been notoriously difficult to track down and fix. Part of this is because of the asynchronous nature of ipc and the many threads across multiple processes involved. In NaCl, we're seeing a condition now where if a nexe crashes while playing back audio, the chrome back end continues dropping bytes into the SyncSocket. After a short while, the buffer fills up and the write becomes a blocking write, and the audio thread in chrome stops. Since the nacl process has crashed, it will never read more from the SyncSocket, and the back end thread in chrome remains blocked. From this point on, chrome is unable to dole out more audio resources for any additional nacl applications the user attempts to run. It seems at the moment the PPAPI resources used by a NaCl process aren't cleaned out until surf-away from the offending page. If the user lingers too long without surfing away, the condition above will occur. I'm currently looking at either 1) make sure all handles on the receiving end of the SyncSocket get closed, then make sure back end bails if a write to SyncSocket fails. 2) or, if we detect a dead nexe, release all resources for that instance (don't wait until page surf-away to tear down.) This should in practice also stop the playback before the SyncSocket buffer fills up. Still debugging this one but if you guys have any insight, greatly appreciated. Sorry for the long historical notes and the thread hijack.
Hi everyone,
Here's a new proposal that fixes the race issue, avoids the "no data" problem
that Eugene pointed out and does not require special synchronization primitives
such as an event.
1) Remove the shared_memory(), shared_memory_data() and socket() accessors.
2) Change the socket_ member variable from being a scoped_ptr<base::SyncSocket>
to be base::SyncSocket::Handle and change shared_memory_ to be
base::SharedMemoryHandle.
3) Assign to these variables as is done today inside OnLowLatencyCreated before
the audio thread is started. After this point, only the audio thread may touch
these variables except for one place below (see last step below).
4) Inside the Run() method, create two local variables, one SyncSocket and one
SharedMemory. These local variables take over ownership of the socket_ and
shared_memory_ member variables.
5) Change FireCaptureCallback to accept a pointer to the data (int16*) and
provide this data from the local variable that belongs to the audio thread.
6) To stop the audio thread, we close the socket from within Stop(). We don't
do it the way we used to via the SyncSocket variable (that was the race).
Instead, we close it by creating a local temp inside stop: |{base::SyncSocket
socket(socket_);}|. Note both the scope and that socket_ is of type
base::SyncSocket::Handle. This closes the socket, which breaks any pending and
future Receive attempts on the socket. The side effect that this will have is
that the socket will be closed twice, but that's fine as the second attempt to
close, won't do anything.
On 2011/11/30 10:09:40, tommi wrote:
> Hi everyone,
>
> Here's a new proposal that fixes the race issue, avoids the "no data" problem
> that Eugene pointed out and does not require special synchronization
primitives
> such as an event.
>
> 1) Remove the shared_memory(), shared_memory_data() and socket() accessors.
>
> 2) Change the socket_ member variable from being a
scoped_ptr<base::SyncSocket>
> to be base::SyncSocket::Handle and change shared_memory_ to be
> base::SharedMemoryHandle.
>
> 3) Assign to these variables as is done today inside OnLowLatencyCreated
before
> the audio thread is started. After this point, only the audio thread may
touch
> these variables except for one place below (see last step below).
>
> 4) Inside the Run() method, create two local variables, one SyncSocket and one
> SharedMemory. These local variables take over ownership of the socket_ and
> shared_memory_ member variables.
>
> 5) Change FireCaptureCallback to accept a pointer to the data (int16*) and
> provide this data from the local variable that belongs to the audio thread.
>
> 6) To stop the audio thread, we close the socket from within Stop(). We don't
> do it the way we used to via the SyncSocket variable (that was the race).
> Instead, we close it by creating a local temp inside stop: |{base::SyncSocket
> socket(socket_);}|. Note both the scope and that socket_ is of type
> base::SyncSocket::Handle. This closes the socket, which breaks any pending
and
> future Receive attempts on the socket. The side effect that this will have is
> that the socket will be closed twice, but that's fine as the second attempt to
> close, won't do anything.
Looks that you did not upload new version of the code...
Some reply in line. Eugene, I tried Tommi's proposal, but there is still some racing conditions on the Stop() and Run(), the strange thing is that the race only exists in the input side, but it is fixed in the output. I am guessing that the race is because we are trying to access to the same socket_handle_ at different threads. If this is true, which means that we can't directly close the socket when it is being used, then we need another way to quit the loop. I am still not sure why WaitableEvent is not acceptable here, except it is a bit slow, but if this is a problem, can we consider using Barrier_AtomicIncrement? Thanks for all the input. BR, /SX http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... content/renderer/media/audio_device.cc:215: while (!audio_event_.IsSignaled() && On 2011/11/29 16:14:48, enal1 wrote: > I don't fully understand how event helps in a case when thread is blocked on > socket wait. I.e. suggested solution works if there is consistent stream of data > coming in, but not for case when there is no data... Really good question. No, event does not help at all when the socket is waiting for receiving, but the current solution, which calls socket_.Close() does not fix the problem either, the Close() can only jumps in after socket finishes receiving. http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... content/renderer/media/audio_input_device.cc:292: while (!audio_event_.IsSignaled() && On 2011/11/29 20:38:16, Ami Fischman wrote: > On 2011/11/29 20:11:32, tommi wrote: > > Can you explain this in a bit more detail? No locks should be needed since if > > the event is signaled, the loop breaks. The event object is itself, thread > > safe. > > The problem is with the other case; IsSignaled() may return false and > immediately thereafter a signal comes in (before the subsequent Receive is run). > IIUC the bug this CL is trying to fix, you're back to square one in this case. > Event is thread safe. In the case you mentioned above, the socket will continue to work, but it will quite the loop next time it check if audio_event_.IsSignaled(). This is fine since our Stop() is synchronous. The only concern is that event is not "quick", my understanding here is that when the event is signal, it may take a short interval to have IsSignaled() return true. > > The race is that the socket handle is being closed and set to invalid from one > > thread while another is waiting on it. > > What does "set to invalid" mean? > Merely reading on one thread and closing on another thread should work fine (in > vanilla socket(2)). If SyncSocket doesn't support that that's a bug IMO. > Can anyone repro a tsan alert on a SyncSocket test (not using Audio*Device)? > > > Looking closer at the code though, I see that there is a check that Start() is > > always called on the IO thread but there is no such check for Stop(). > > Start() posts to the IO thread. Stop() posts to the IO thread for shutdown and > then waits for completion before returning. Neither check that they are "always > called on the IO thread". > > > Then there's also the problem that was pointed out if no data is sent to the > > socket - which could cause a hang. > > This goes to the crux of my point about fixing races in SyncSocket; SyncSocket > is a synchronization abstraction which makes me strongly doubt it's possible to > work *around* bugs in it from the outside without giving up serious latency > concessions. I think @nfullagar has covered the situation well, I am not sure if there is any perfect solution to handle the race during the tear down, the current Close() in syncsocket just tries to close the handle, and this is allowed to fail. I believe the client should take care of the case when the socket should be closed. Furthermore, I think there might be two races here, one is that we access to socket_ object in two threads, another is that we try to close the handle of the socket in Stop() while receiving data in Run().
(All my remarks were about output device, I did not look at the input device... I assume it has similar problems, but cannot be sure) There should be no problem as long as thread deleting the object waits for audio thread to stop. If there is some problem, we either did not call Join(), or audio thread newer exit because it is blocked on socket read. First possibility: interesting thing can happen if Stop() is called on I/O thread -- Stop() posts a task on that thread and then waits for its completion (and assumes it is done in 1 second). In this case we would never call Join(). Maybe we should check if we are running on the same thread, and directly call the function instead of posting the task? Second possibility: problem is reported under TSAN/ASAN/whatever -- can it be that it delays our receiving the event, to test that we correctly handle bad timing? If so, it really uncovered the problem -- if we did not get confirmation in 1 second, we would not call Join(), and eventually will delete the socket while audio thread reads from it. Moving ownership of used objects to audio thread should fix the crash, but we still can be left with thread that waits forever and will not exit, i.e. with leaked resources, and lot of leaked resources. That is exactly the situation I regularly hit debugging different issues. Current design depends on the cooperation of 2nd IPC side when stopping, and expects it to be done in fixed time, and if something goes wrong we hang. Can we somehow solve that problem? On Windows I'd say to use stopping event, and call WaitForMultipleObjects() to wait for either stopping event or data from the socket, but we don't have portable WaitForMultipleObjects(). So I suggested to store 2nd part of socked pair, and use it to tell the audio thread it's time to stop. Maybe the right approach is just to fix crash by transferring ownership of socket and shared memory to the audio thread, and open separate bug, "force audio thread to stop even if process it cooperates with hangs". Thanks, Eugene On Wed, Nov 30, 2011 at 9:02 AM, <xians@chromium.org> wrote: > Some reply in line. > > Eugene, I tried Tommi's proposal, but there is still some racing conditions > on > the Stop() and Run(), the strange thing is that the race only exists in the > input side, but it is fixed in the output. > I am guessing that the race is because we are trying to access to the same > socket_handle_ at different threads. > If this is true, which means that we can't directly close the socket when it > is > being used, then we need another way to quit the loop. I am still not sure > why > WaitableEvent is not acceptable here, except it is a bit slow, but if this > is a > problem, can we consider using Barrier_AtomicIncrement? > > Thanks for all the input. > > BR, > /SX > > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... > File content/renderer/media/audio_device.cc (right): > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... > content/renderer/media/audio_device.cc:215: while > (!audio_event_.IsSignaled() && > On 2011/11/29 16:14:48, enal1 wrote: >> >> I don't fully understand how event helps in a case when thread is > > blocked on >> >> socket wait. I.e. suggested solution works if there is consistent > > stream of data >> >> coming in, but not for case when there is no data... > > > Really good question. No, event does not help at all when the socket is > waiting for receiving, but the current solution, which calls > socket_.Close() does not fix the problem either, the Close() can only > jumps in after socket finishes receiving. > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > File content/renderer/media/audio_input_device.cc (right): > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > content/renderer/media/audio_input_device.cc:292: while > (!audio_event_.IsSignaled() && > On 2011/11/29 20:38:16, Ami Fischman wrote: >> >> On 2011/11/29 20:11:32, tommi wrote: >> > Can you explain this in a bit more detail? No locks should be > > needed since if >> >> > the event is signaled, the loop breaks. The event object is itself, > > thread >> >> > safe. > > >> The problem is with the other case; IsSignaled() may return false and >> immediately thereafter a signal comes in (before the subsequent > > Receive is run). >> >> IIUC the bug this CL is trying to fix, you're back to square one in > > this case. > > > Event is thread safe. In the case you mentioned above, the socket will > continue to work, but it will quite the loop next time it check if > audio_event_.IsSignaled(). This is fine since our Stop() is synchronous. > > > The only concern is that event is not "quick", my understanding here is > that when the event is signal, it may take a short interval to have > IsSignaled() return true. > > >> > The race is that the socket handle is being closed and set to > > invalid from one >> >> > thread while another is waiting on it. > > >> What does "set to invalid" mean? >> Merely reading on one thread and closing on another thread should work > > fine (in >> >> vanilla socket(2)). If SyncSocket doesn't support that that's a bug > > IMO. >> >> Can anyone repro a tsan alert on a SyncSocket test (not using > > Audio*Device)? > >> > Looking closer at the code though, I see that there is a check that > > Start() is >> >> > always called on the IO thread but there is no such check for > > Stop(). > >> Start() posts to the IO thread. Stop() posts to the IO thread for > > shutdown and >> >> then waits for completion before returning. Neither check that they > > are "always >> >> called on the IO thread". > > >> > Then there's also the problem that was pointed out if no data is > > sent to the >> >> > socket - which could cause a hang. > > >> This goes to the crux of my point about fixing races in SyncSocket; > > SyncSocket >> >> is a synchronization abstraction which makes me strongly doubt it's > > possible to >> >> work *around* bugs in it from the outside without giving up serious > > latency >> >> concessions. > > > I think @nfullagar has covered the situation well, I am not sure if > there is any perfect solution to handle the race during the tear down, > the current Close() in syncsocket just tries to close the handle, and > this is allowed to fail. I believe the client should take care of the > case when the socket should be closed. > > Furthermore, I think there might be two races here, one is that we > access to socket_ object in two threads, another is that we try to close > the handle of the socket in Stop() while receiving data in Run(). > > http://codereview.chromium.org/8659040/
On Wed, Nov 30, 2011 at 6:48 PM, Eugene Nalimov <enal@google.com> wrote: > (All my remarks were about output device, I did not look at the input > device... I assume it has similar problems, but cannot be sure) > > There should be no problem as long as thread deleting the object waits > for audio thread to stop. If there is some problem, we either did not > call Join(), or audio thread newer exit because it is blocked on > socket read. > > First possibility: interesting thing can happen if Stop() is called on > I/O thread -- Stop() posts a task on that thread and then waits for > its completion (and assumes it is done in 1 second). In this case we > would never call Join(). Maybe we should check if we are running on > the same thread, and directly call the function instead of posting the > task? > > Second possibility: problem is reported under TSAN/ASAN/whatever -- > can it be that it delays our receiving the event, to test that we > correctly handle bad timing? If so, it really uncovered the problem -- > if we did not get confirmation in 1 second, we would not call Join(), > and eventually will delete the socket while audio thread reads from > it. Moving ownership of used objects to audio thread should fix the > crash, but we still can be left with thread that waits forever and > will not exit, i.e. with leaked resources, and lot of leaked > resources. > > That is exactly the situation I regularly hit debugging different > issues. Current design depends on the cooperation of 2nd IPC side when > stopping, and expects it to be done in fixed time, and if something > goes wrong we hang. Can we somehow solve that problem? On Windows I'd > say to use stopping event, and call WaitForMultipleObjects() to wait > for either stopping event or data from the socket, but we don't have > portable WaitForMultipleObjects(). So I suggested to store 2nd part of > socked pair, and use it to tell the audio thread it's time to stop. > > Maybe the right approach is just to fix crash by transferring > ownership of socket and shared memory to the audio thread, and open > separate bug, "force audio thread to stop even if process it > cooperates with hangs". > I think that's the right approach. Shijing is trying out the proposal I sent right now but he hasn't uploaded the code yet because valgrind is still reporting a race when we close the socket from within Stop(). Note that this is not the same as was previously done which changed the state of a SyncSocket variable. This is purely calling close() on Linux and CloseHandle on Windows. The SyncSocket variable sits on the stack of the audio thread and its state should never be touched, so I'm going to take a closer look at it tomorrow with Shijing to figure out what's going on. Cheers, Tommi > > Thanks, > Eugene > > On Wed, Nov 30, 2011 at 9:02 AM, <xians@chromium.org> wrote: > > Some reply in line. > > > > Eugene, I tried Tommi's proposal, but there is still some racing > conditions > > on > > the Stop() and Run(), the strange thing is that the race only exists in > the > > input side, but it is fixed in the output. > > I am guessing that the race is because we are trying to access to the > same > > socket_handle_ at different threads. > > If this is true, which means that we can't directly close the socket > when it > > is > > being used, then we need another way to quit the loop. I am still not > sure > > why > > WaitableEvent is not acceptable here, except it is a bit slow, but if > this > > is a > > problem, can we consider using Barrier_AtomicIncrement? > > > > Thanks for all the input. > > > > BR, > > /SX > > > > > > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... > > File content/renderer/media/audio_device.cc (right): > > > > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_de... > > content/renderer/media/audio_device.cc:215: while > > (!audio_event_.IsSignaled() && > > On 2011/11/29 16:14:48, enal1 wrote: > >> > >> I don't fully understand how event helps in a case when thread is > > > > blocked on > >> > >> socket wait. I.e. suggested solution works if there is consistent > > > > stream of data > >> > >> coming in, but not for case when there is no data... > > > > > > Really good question. No, event does not help at all when the socket is > > waiting for receiving, but the current solution, which calls > > socket_.Close() does not fix the problem either, the Close() can only > > jumps in after socket finishes receiving. > > > > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > > File content/renderer/media/audio_input_device.cc (right): > > > > > http://codereview.chromium.org/8659040/diff/1/content/renderer/media/audio_in... > > content/renderer/media/audio_input_device.cc:292: while > > (!audio_event_.IsSignaled() && > > On 2011/11/29 20:38:16, Ami Fischman wrote: > >> > >> On 2011/11/29 20:11:32, tommi wrote: > >> > Can you explain this in a bit more detail? No locks should be > > > > needed since if > >> > >> > the event is signaled, the loop breaks. The event object is itself, > > > > thread > >> > >> > safe. > > > > > >> The problem is with the other case; IsSignaled() may return false and > >> immediately thereafter a signal comes in (before the subsequent > > > > Receive is run). > >> > >> IIUC the bug this CL is trying to fix, you're back to square one in > > > > this case. > > > > > > Event is thread safe. In the case you mentioned above, the socket will > > continue to work, but it will quite the loop next time it check if > > audio_event_.IsSignaled(). This is fine since our Stop() is synchronous. > > > > > > The only concern is that event is not "quick", my understanding here is > > that when the event is signal, it may take a short interval to have > > IsSignaled() return true. > > > > > >> > The race is that the socket handle is being closed and set to > > > > invalid from one > >> > >> > thread while another is waiting on it. > > > > > >> What does "set to invalid" mean? > >> Merely reading on one thread and closing on another thread should work > > > > fine (in > >> > >> vanilla socket(2)). If SyncSocket doesn't support that that's a bug > > > > IMO. > >> > >> Can anyone repro a tsan alert on a SyncSocket test (not using > > > > Audio*Device)? > > > >> > Looking closer at the code though, I see that there is a check that > > > > Start() is > >> > >> > always called on the IO thread but there is no such check for > > > > Stop(). > > > >> Start() posts to the IO thread. Stop() posts to the IO thread for > > > > shutdown and > >> > >> then waits for completion before returning. Neither check that they > > > > are "always > >> > >> called on the IO thread". > > > > > >> > Then there's also the problem that was pointed out if no data is > > > > sent to the > >> > >> > socket - which could cause a hang. > > > > > >> This goes to the crux of my point about fixing races in SyncSocket; > > > > SyncSocket > >> > >> is a synchronization abstraction which makes me strongly doubt it's > > > > possible to > >> > >> work *around* bugs in it from the outside without giving up serious > > > > latency > >> > >> concessions. > > > > > > I think @nfullagar has covered the situation well, I am not sure if > > there is any perfect solution to handle the race during the tear down, > > the current Close() in syncsocket just tries to close the handle, and > > this is allowed to fail. I believe the client should take care of the > > case when the socket should be closed. > > > > Furthermore, I think there might be two races here, one is that we > > access to socket_ object in two threads, another is that we try to close > > the handle of the socket in Stop() while receiving data in Run(). > > > > http://codereview.chromium.org/8659040/ >
Hi, Tommi helped me found out the reason why I still had race with his proposal: the binary was not built with my changes. I think the problem is because I opened the file in eclipse and modified it outside eclipse, some old cache was used and my changes was not written to the file thought it looked like it did. Anyway, race conditions are fixed by the new proposal. Besides that, I added a check on the Stop() not to allow being called by IO thread. And also modified the code to continue to close the socket and audio thread when a timeout happens. I can also confirm here that this solution is able to close the socket when the socket is waiting. A small problem as Tommi has already pointed out, an error meessage will pop up after Stop() since we will close the socket twice. But this does not hurt, hope it is fine. Please take the second round. BR, /SX
lgtm. good job Shijing. As a general comment, these classes look like twins. We should refactor them so that the share code instead of being copy/paste of each other. That way we can fix problems like this one in one place instead of two. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:76: LOG(WARNING) << "Failed to shut down audio output on IO thread"; LOG(ERROR)? http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:189: audio_thread_.reset( maybe we should dcheck at the top that the audio thread doesn't already exist. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:213: FireRenderCallback(static_cast<int16*>(shared_memory.memory())); reinterpret_cast? http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:227: FireRenderCallback(static_cast<int16*>(shared_memory.memory())); same here http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... File content/renderer/media/audio_input_device.cc (right): http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_input_device.cc:310: FireCaptureCallback(static_cast<int16*>(shared_memory.memory())); reinterpret_cast
Looks great. LGTM.
http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:81: base::SyncSocket socket(socket_handle_); A comment perhaps?
On 2011/12/01 14:57:12, henrika1 wrote: > http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... > File content/renderer/media/audio_device.cc (right): > > http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... > content/renderer/media/audio_device.cc:81: base::SyncSocket > socket(socket_handle_); > A comment perhaps? lgtm.
Removing myself as reviewer. Preventing multiple threads from sharing non-const state is a huge improvement - thanks for doing it! http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:87: return true; FWIW, this function can't return false anymore after your change; change its signature?
Updated the code based on the comments from reviewers. +@rtoy since this will affect the content/renderer/media/webrtc_audio_device_impl.cc Raymond, could you please take a quick look at the CL, I will also notify Chris on this CL. Thanks, SX http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:76: LOG(WARNING) << "Failed to shut down audio output on IO thread"; On 2011/12/01 14:40:45, tommi wrote: > LOG(ERROR)? Done. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:81: base::SyncSocket socket(socket_handle_); On 2011/12/01 14:57:13, henrika1 wrote: > A comment perhaps? Done. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:87: return true; On 2011/12/01 17:25:28, Ami Fischman wrote: > FWIW, this function can't return false anymore after your change; change its > signature? Done. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:189: audio_thread_.reset( On 2011/12/01 14:40:45, tommi wrote: > maybe we should dcheck at the top that the audio thread doesn't already exist. Done. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:213: FireRenderCallback(static_cast<int16*>(shared_memory.memory())); On 2011/12/01 14:40:45, tommi wrote: > reinterpret_cast? Done. http://codereview.chromium.org/8659040/diff/21002/content/renderer/media/audi... content/renderer/media/audio_device.cc:227: FireRenderCallback(static_cast<int16*>(shared_memory.memory())); On 2011/12/01 14:40:45, tommi wrote: > same here Done.
lgtm++
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8659040/23004
Presubmit check for 8659040-23004 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** If this change requires manual test instructions to QA team, add TEST=[instructions]. ** Presubmit ERRORS ** Missing LGTM from an OWNER for: content/renderer/media/audio_input_device.h,content/renderer/media/audio_device.h,content/renderer/media/audio_input_device.cc,content/renderer/media/audio_device.cc,content/renderer/renderer_webaudiodevice_impl.cc,content/renderer/media/webrtc_audio_device_impl.cc Presubmit checks took 2.7s to calculate.
content/renderer LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8659040/23004
Try job failure for 8659040-23004 (retry) on win_rel for step "unit_tests". It's a second try, previously, step "unit_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/8659040/23004
Change committed as 113386
I assume you've used Memcheck? This is not the right tool to debug data races, use ThreadSanitizer instead next time: http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer Also, please watch the waterfall after commiting as TSan bots may bark.
On 2011/12/07 14:21:48, Timur Iskhodzhanov wrote: > I assume you've used Memcheck? > This is not the right tool to debug data races, use ThreadSanitizer instead next > time: > http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer > > Also, please watch the waterfall after commiting as TSan bots may bark. Did you try to run your change under debugger? On Windows, when running under debugger, I am getting exception "First-chance exception at 0x77aff9e2 in chrome.exe: 0xC0000008: An invalid handle was specified."... That makes it extremely difficult to debug almost any audio-related code...
You should be able to configure the debugger to not break on that exception - although it's not optimal of course. On Sat, Dec 10, 2011 at 12:03 AM, <enal@google.com> wrote: > On 2011/12/07 14:21:48, Timur Iskhodzhanov wrote: > >> I assume you've used Memcheck? >> This is not the right tool to debug data races, use ThreadSanitizer >> instead >> > next > >> time: >> http://dev.chromium.org/**developers/how-tos/using-** >> valgrind/threadsanitizer<http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer> >> > > Also, please watch the waterfall after commiting as TSan bots may bark. >> > > Did you try to run your change under debugger? > > On Windows, when running under debugger, I am getting exception > "First-chance > exception at 0x77aff9e2 in chrome.exe: 0xC0000008: An invalid handle was > specified."... > > That makes it extremely difficult to debug almost any audio-related code... > > http://codereview.chromium.**org/8659040/<http://codereview.chromium.org/8659... >
I can, though can every Chrome developer from now to eternity? Is it possible to encapsulate socket into ref-counted object that keeps track of its state? That will also fix potential problem with OS reusing handle before we closed it for 2nd time, and us closing absolutely unrelated handle... On Fri, Dec 9, 2011 at 4:13 PM, Tommi <tommi@chromium.org> wrote: > You should be able to configure the debugger to not break on that exception > - although it's not optimal of course. > > On Sat, Dec 10, 2011 at 12:03 AM, <enal@google.com> wrote: >> >> On 2011/12/07 14:21:48, Timur Iskhodzhanov wrote: >>> >>> I assume you've used Memcheck? >>> This is not the right tool to debug data races, use ThreadSanitizer >>> instead >> >> next >>> >>> time: >>> http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer >> >> >>> Also, please watch the waterfall after commiting as TSan bots may bark. >> >> >> Did you try to run your change under debugger? >> >> On Windows, when running under debugger, I am getting exception >> "First-chance >> exception at 0x77aff9e2 in chrome.exe: 0xC0000008: An invalid handle was >> specified."... >> >> That makes it extremely difficult to debug almost any audio-related >> code... >> >> http://codereview.chromium.org/8659040/ > >
That's a good idea. I can do it if you want or if you want, you can send me the review. On Sat, Dec 10, 2011 at 2:25 AM, Eugene Nalimov <enal@google.com> wrote: > I can, though can every Chrome developer from now to eternity? > > Is it possible to encapsulate socket into ref-counted object that > keeps track of its state? That will also fix potential problem with OS > reusing handle before we closed it for 2nd time, and us closing > absolutely unrelated handle... > > On Fri, Dec 9, 2011 at 4:13 PM, Tommi <tommi@chromium.org> wrote: > > You should be able to configure the debugger to not break on that > exception > > - although it's not optimal of course. > > > > On Sat, Dec 10, 2011 at 12:03 AM, <enal@google.com> wrote: > >> > >> On 2011/12/07 14:21:48, Timur Iskhodzhanov wrote: > >>> > >>> I assume you've used Memcheck? > >>> This is not the right tool to debug data races, use ThreadSanitizer > >>> instead > >> > >> next > >>> > >>> time: > >>> > http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer > >> > >> > >>> Also, please watch the waterfall after commiting as TSan bots may bark. > >> > >> > >> Did you try to run your change under debugger? > >> > >> On Windows, when running under debugger, I am getting exception > >> "First-chance > >> exception at 0x77aff9e2 in chrome.exe: 0xC0000008: An invalid handle was > >> specified."... > >> > >> That makes it extremely difficult to debug almost any audio-related > >> code... > >> > >> http://codereview.chromium.org/8659040/ > > > > >
I am pretty busy working on fixing recent regression... On Sat, Dec 10, 2011 at 2:06 AM, Tommi <tommi@chromium.org> wrote: > That's a good idea. I can do it if you want or if you want, you can send me > the review. > > On Sat, Dec 10, 2011 at 2:25 AM, Eugene Nalimov <enal@google.com> wrote: >> >> I can, though can every Chrome developer from now to eternity? >> >> Is it possible to encapsulate socket into ref-counted object that >> keeps track of its state? That will also fix potential problem with OS >> reusing handle before we closed it for 2nd time, and us closing >> absolutely unrelated handle... >> >> On Fri, Dec 9, 2011 at 4:13 PM, Tommi <tommi@chromium.org> wrote: >> > You should be able to configure the debugger to not break on that >> > exception >> > - although it's not optimal of course. >> > >> > On Sat, Dec 10, 2011 at 12:03 AM, <enal@google.com> wrote: >> >> >> >> On 2011/12/07 14:21:48, Timur Iskhodzhanov wrote: >> >>> >> >>> I assume you've used Memcheck? >> >>> This is not the right tool to debug data races, use ThreadSanitizer >> >>> instead >> >> >> >> next >> >>> >> >>> time: >> >>> >> >>> http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer >> >> >> >> >> >>> Also, please watch the waterfall after commiting as TSan bots may >> >>> bark. >> >> >> >> >> >> Did you try to run your change under debugger? >> >> >> >> On Windows, when running under debugger, I am getting exception >> >> "First-chance >> >> exception at 0x77aff9e2 in chrome.exe: 0xC0000008: An invalid handle >> >> was >> >> specified."... >> >> >> >> That makes it extremely difficult to debug almost any audio-related >> >> code... >> >> >> >> http://codereview.chromium.org/8659040/ >> > >> > > >
I can take a look at it when I have time. I think ref count is a good idea, but we also need to be able to force closing the socket when there is no data, so likely an async socket implementation is needed. BR, /SX On Mon, Dec 12, 2011 at 6:17 PM, Eugene Nalimov <enal@google.com> wrote: > I am pretty busy working on fixing recent regression... > > On Sat, Dec 10, 2011 at 2:06 AM, Tommi <tommi@chromium.org> wrote: > > That's a good idea. I can do it if you want or if you want, you can > send me > > the review. > > > > On Sat, Dec 10, 2011 at 2:25 AM, Eugene Nalimov <enal@google.com> wrote: > >> > >> I can, though can every Chrome developer from now to eternity? > >> > >> Is it possible to encapsulate socket into ref-counted object that > >> keeps track of its state? That will also fix potential problem with OS > >> reusing handle before we closed it for 2nd time, and us closing > >> absolutely unrelated handle... > >> > >> On Fri, Dec 9, 2011 at 4:13 PM, Tommi <tommi@chromium.org> wrote: > >> > You should be able to configure the debugger to not break on that > >> > exception > >> > - although it's not optimal of course. > >> > > >> > On Sat, Dec 10, 2011 at 12:03 AM, <enal@google.com> wrote: > >> >> > >> >> On 2011/12/07 14:21:48, Timur Iskhodzhanov wrote: > >> >>> > >> >>> I assume you've used Memcheck? > >> >>> This is not the right tool to debug data races, use ThreadSanitizer > >> >>> instead > >> >> > >> >> next > >> >>> > >> >>> time: > >> >>> > >> >>> > http://dev.chromium.org/developers/how-tos/using-valgrind/threadsanitizer > >> >> > >> >> > >> >>> Also, please watch the waterfall after commiting as TSan bots may > >> >>> bark. > >> >> > >> >> > >> >> Did you try to run your change under debugger? > >> >> > >> >> On Windows, when running under debugger, I am getting exception > >> >> "First-chance > >> >> exception at 0x77aff9e2 in chrome.exe: 0xC0000008: An invalid handle > >> >> was > >> >> specified."... > >> >> > >> >> That makes it extremely difficult to debug almost any audio-related > >> >> code... > >> >> > >> >> http://codereview.chromium.org/8659040/ > >> > > >> > > > > > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
