Access current frame counter carefully and remove m_cachedSampleFrame.

Access current frame counter carefully and remove m_cachedSampleFrame.

m_cachedSampleFrame was added in https://codereview.chromium.org/773273002 to 
handle a race condition on accessing the the current frame counter that is being
updated in the audio thread.  Use acquireLoad and releaseStore so no race can
happen while it is being updated.

BUG=488211
TEST=Ran the TSAN test case from issue 431874 without problems.

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195816

[Raymond Toy, 2015-05-21 16:14:35]

rtoy@chromium.org changed reviewers:
+ hongchan@chromium.org, kbr@chromium.org

[Raymond Toy, 2015-05-21 16:14:35]

PTAL

[hongchan, 2015-05-21 16:21:34]

LGTM with one suggestion: can we have a short explanation why this can't be
tested with the layout test? Or perhaps a unit test might be appropriate for
this?

[Raymond Toy, 2015-05-21 16:51:48]

On 2015/05/21 16:21:34, hoch wrote:
> LGTM with one suggestion: can we have a short explanation why this can't be
> tested with the layout test? Or perhaps a unit test might be appropriate for
> this?

I don't we can actually test this because there's no way to determine if the
value is correct or not. The original issue was only discovered in a TSAN build.

[Ken Russell (switch to Gerrit), 2015-05-21 20:45:16]

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:113: return m_destinationNode ?
m_destinationNode->audioDestinationHandler().currentSampleFrame() : 0;
Is it safe to access the destination node's AudioDestinationHandler from the
main thread? The previous code only accessed it from the audio thread.

[Raymond Toy, 2015-05-21 21:19:06]

rtoy@chromium.org changed reviewers:
+ tkent@chromium.org

[Raymond Toy, 2015-05-21 21:19:06]

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:113: return m_destinationNode ?
m_destinationNode->audioDestinationHandler().currentSampleFrame() : 0;
On 2015/05/21 20:45:16, Ken Russell wrote:
> Is it safe to access the destination node's AudioDestinationHandler from the
> main thread? The previous code only accessed it from the audio thread.

I believe this is safe. The purpose of the handler is to implement the guts of
the node and the audio node (which encapsulates the JS object) is just a thin
wrapper to the handler.

+tkent to verify.

[tkent, 2015-05-22 00:47:12]

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
File Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
Source/modules/webaudio/AudioContext.h:113: return m_destinationNode ?
m_destinationNode->audioDestinationHandler().currentSampleFrame() : 0;
On 2015/05/21 21:19:06, Raymond Toy wrote:
> On 2015/05/21 20:45:16, Ken Russell wrote:
> > Is it safe to access the destination node's AudioDestinationHandler from the
> > main thread? The previous code only accessed it from the audio thread.
> 
> I believe this is safe. The purpose of the handler is to implement the guts of
> the node and the audio node (which encapsulates the JS object) is just a thin
> wrapper to the handler.
> 
> +tkent to verify.

Yes, it's ok to touch handlers from the main thread if we correctly avoid races.

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
File Source/modules/webaudio/AudioDestinationNode.cpp (right):

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
Source/modules/webaudio/AudioDestinationNode.cpp:94:
atomicAdd(&m_currentSampleFrame, numberOfFrames);
I don't think using atomicAdd() here is correct.
atomicAdd() is implemented by InterlockedExchangeAdd() on Windows, and 
InterlockedExchangeAdd() avoid races with other Interlocked* functions.  So,
accessing m_currentSampleFrame directly from the main thread is not safe.
We have only one audio thread for a single AudioDestinationHandler.  We don't
need to use atomicAdd() here.

Instead, we should do:

AudioDestinationHandler::render():
    size_t sampleFrame = m_currentSampleFrame + numberOfFrames;
    releaseStore(&m_currentSampleFrame, sampleFrame);

AudioDestinationHandler::rcurrentSampleFrame():
    return acquireLoad(&m_currentSampleFrame);

[Raymond Toy, 2015-05-22 16:13:48]

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
File Source/modules/webaudio/AudioDestinationNode.cpp (right):

https://codereview.chromium.org/1149913002/diff/20001/Source/modules/webaudio...
Source/modules/webaudio/AudioDestinationNode.cpp:94:
atomicAdd(&m_currentSampleFrame, numberOfFrames);
On 2015/05/22 00:47:12, tkent wrote:
> I don't think using atomicAdd() here is correct.
> atomicAdd() is implemented by InterlockedExchangeAdd() on Windows, and 
> InterlockedExchangeAdd() avoid races with other Interlocked* functions.  So,
> accessing m_currentSampleFrame directly from the main thread is not safe.
> We have only one audio thread for a single AudioDestinationHandler.  We don't
> need to use atomicAdd() here.
> 
> Instead, we should do:
> 
> AudioDestinationHandler::render():
>     size_t sampleFrame = m_currentSampleFrame + numberOfFrames;
>     releaseStore(&m_currentSampleFrame, sampleFrame);
> 
> AudioDestinationHandler::rcurrentSampleFrame():
>     return acquireLoad(&m_currentSampleFrame);

Done.  Thanks for the hint.

[Ken Russell (switch to Gerrit), 2015-05-22 18:23:21]

Thanks tkent@ for the suggestion. LGTM

[Raymond Toy, 2015-05-22 21:26:45]

The CQ bit was checked by rtoy@chromium.org

[Raymond Toy, 2015-05-22 21:26:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/1149913002/#ps60001
(title: "Remove workaround in scriptprocessornode test")

[commit-bot: I haz the power, 2015-05-22 21:26:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149913002/60001

[commit-bot: I haz the power, 2015-05-22 22:16:48]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195816