Fix a race condition in MediaStreamAudioTrackHost.

Fix a race condition in MediaStreamAudioTrackHost.

In MediaStreamAudioTrackHost, the audio buffer is filled with audio data in an audio thread, and it is sent to plugin in the main thread, but the buffer may become invalid between filling and sending. It crashes chrome. This patch fixes this issue.

BUG=396395
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286057

[Peng, 2014-07-23 20:19:03]

Hi David, PTAL. Thanks.

[dmichael (off chromium), 2014-07-24 16:42:56]

amistry@, would you be willing to look at this since you're very familiar with
the problem?

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:120: // We don't
need hold |lock_| during |host->InitBuffers()| call, because
need +to+ hold

[Anand Mistry (off Chromium), 2014-07-25 00:42:54]

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:145: // so we
don't need lock |lock_| here (main thread).
Although I'll agree this is a benign read, I think this will trip up an analysis
tool like asan. Is there a way in Chrome to tag a benign read?

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:104: int32_t
init_buffers_count_;
To me, after reading the code, I think it would be clearer to call this
buffer_generation_ instead (or something like that).

[Peng, 2014-07-25 15:25:31]

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:120: // We don't
need hold |lock_| during |host->InitBuffers()| call, because
On 2014/07/24 16:42:56, dmichael wrote:
> need +to+ hold

Done.

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:145: // so we
don't need lock |lock_| here (main thread).
On 2014/07/25 00:42:54, Anand Mistry wrote:
> Although I'll agree this is a benign read, I think this will trip up an
analysis
> tool like asan. Is there a way in Chrome to tag a benign read?

I think rwlock is better for this case, but I can not find it in chromium. I
don't know how to tag it in code.

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.h:104: int32_t
init_buffers_count_;
On 2014/07/25 00:42:54, Anand Mistry wrote:
> To me, after reading the code, I think it would be clearer to call this
> buffer_generation_ instead (or something like that).

Done.

[Anand Mistry (off Chromium), 2014-07-26 06:18:59]

On 2014/07/25 15:25:31, Peng wrote:
>
https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):
> 
>
https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
> content/renderer/pepper/pepper_media_stream_audio_track_host.cc:120: // We
don't
> need hold |lock_| during |host->InitBuffers()| call, because
> On 2014/07/24 16:42:56, dmichael wrote:
> > need +to+ hold
> 
> Done.
> 
>
https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
> content/renderer/pepper/pepper_media_stream_audio_track_host.cc:145: // so we
> don't need lock |lock_| here (main thread).
> On 2014/07/25 00:42:54, Anand Mistry wrote:
> > Although I'll agree this is a benign read, I think this will trip up an
> analysis
> > tool like asan. Is there a way in Chrome to tag a benign read?
> 
> I think rwlock is better for this case, but I can not find it in chromium. I
> don't know how to tag it in code.
> 
>
https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
> File content/renderer/pepper/pepper_media_stream_audio_track_host.h (right):
> 
>
https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
> content/renderer/pepper/pepper_media_stream_audio_track_host.h:104: int32_t
> init_buffers_count_;
> On 2014/07/25 00:42:54, Anand Mistry wrote:
> > To me, after reading the code, I think it would be clearer to call this
> > buffer_generation_ instead (or something like that).
> 
> Done.

lgtm

[dmichael (off chromium), 2014-07-28 19:30:43]

lgtm

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
File content/renderer/pepper/pepper_media_stream_audio_track_host.cc (right):

https://codereview.chromium.org/417753002/diff/40001/content/renderer/pepper/...
content/renderer/pepper/pepper_media_stream_audio_track_host.cc:145: // so we
don't need lock |lock_| here (main thread).
On 2014/07/25 15:25:31, Peng wrote:
> On 2014/07/25 00:42:54, Anand Mistry wrote:
> > Although I'll agree this is a benign read, I think this will trip up an
> analysis
> > tool like asan. Is there a way in Chrome to tag a benign read?
> 
> I think rwlock is better for this case, but I can not find it in chromium. I
> don't know how to tag it in code.
I don't think TSAN will flag it; I think it's fine.

[Peng, 2014-07-28 19:42:28]

The CQ bit was checked by penghuang@chromium.org

[commit-bot: I haz the power, 2014-07-28 20:36:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/penghuang@chromium.org/417753002/80001

[commit-bot: I haz the power, 2014-07-29 00:13:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-07-29 02:52:08]

Change committed as 286057