Bug 236279 – AudioBuffer should take a lock while visiting m_channelWrappers

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Yusuke Suzuki]

Since the vector is cleared in releaseMemory.

[Yusuke Suzuki]

Created attachment 451347 [details]
Patch

[Keith Miller]

Comment on attachment 451347 [details]
Patch

r=me.

[Chris Dumez]

Comment on attachment 451347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451347&action=review

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:95
> +    channels.reserveCapacity(numberOfChannels);

should be reserveInitialCapacity()

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:107
> +        channels.append(WTFMove(channelDataArray));

could use uncheckedAppend() since you reserve capacity.

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:126
> +    channels.reserveCapacity(numberOfChannels);

ditto.

> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:135
> +        channels.append(WTFMove(channelDataArray));

ditto.

> Source/WebCore/Modules/webaudio/AudioBuffer.h:102
> +    FixedVector<RefPtr<Float32Array>> m_channels;

Would be nice to use WTF_GUARDED_BY_LOCK(m_channelsLock);

> Source/WebCore/Modules/webaudio/AudioBuffer.h:103
> +    FixedVector<JSValueInWrappedObject> m_channelWrappers;

ditto.

> Source/WebCore/dom/MessageEvent.h:104
> +    mutable Lock m_concurrentDataAccessLock;

Can we use clang thread safety annotations?

[Yusuke Suzuki]

Comment on attachment 451347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451347&action=review

>> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:95
>> +    channels.reserveCapacity(numberOfChannels);
> 
> should be reserveInitialCapacity()

Fixed.

>> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:107
>> +        channels.append(WTFMove(channelDataArray));
> 
> could use uncheckedAppend() since you reserve capacity.

Fixed.

>> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:126
>> +    channels.reserveCapacity(numberOfChannels);
> 
> ditto.

Fixed.

>> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:135
>> +        channels.append(WTFMove(channelDataArray));
> 
> ditto.

Fixed.

>> Source/WebCore/Modules/webaudio/AudioBuffer.h:102
>> +    FixedVector<RefPtr<Float32Array>> m_channels;
> 
> Would be nice to use WTF_GUARDED_BY_LOCK(m_channelsLock);

Probably, TSan does not work well here since,
1. We are not taking a lock in the main thread read since we know that any other threads except the main thread will not change it.
2. But we take a lock when reading it from concurrent threads since it can be modified from the main thread.

[Yusuke Suzuki]

Committed r289476 (247019@trunk): <https://commits.webkit.org/247019@trunk>

[Radar WebKit Bug Importer]

<rdar://problem/88693354>

[Chris Dumez]

(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 451347 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=451347&action=review
> 
> >> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:95
> >> +    channels.reserveCapacity(numberOfChannels);
> > 
> > should be reserveInitialCapacity()
> 
> Fixed.
> 
> >> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:107
> >> +        channels.append(WTFMove(channelDataArray));
> > 
> > could use uncheckedAppend() since you reserve capacity.
> 
> Fixed.
> 
> >> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:126
> >> +    channels.reserveCapacity(numberOfChannels);
> > 
> > ditto.
> 
> Fixed.
> 
> >> Source/WebCore/Modules/webaudio/AudioBuffer.cpp:135
> >> +        channels.append(WTFMove(channelDataArray));
> > 
> > ditto.
> 
> Fixed.
> 
> >> Source/WebCore/Modules/webaudio/AudioBuffer.h:102
> >> +    FixedVector<RefPtr<Float32Array>> m_channels;
> > 
> > Would be nice to use WTF_GUARDED_BY_LOCK(m_channelsLock);
> 
> Probably, TSan does not work well here since,
> 1. We are not taking a lock in the main thread read since we know that any
> other threads except the main thread will not change it.
> 2. But we take a lock when reading it from concurrent threads since it can
> be modified from the main thread.

For what it's worth, this is an example of case where I would use annotations but add WTF_IGNORES_THREAD_SAFETY_ANALYSIS on the functions that fail to lock (with a comment explaining WHY this is ok/safe). I worked hard on adopting annotations in existing code, we should use them in new code too.

[Darin Adler]

Comment on attachment 451347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=451347&action=review

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:41
> +    WTF_MAKE_NONMOVABLE(JSValueInWrappedObject);

This does nothing. WTF_MAKE_NONCOPYABLE already makes things non-movable. There should not be a second macro.

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:-45
> -    // FIXME: Remove them once AudioBuffer's m_channelWrappers bug is fixed.
> -    // https://bugs.webkit.org/show_bug.cgi?id=236279
> -    JSValueInWrappedObject(JSValueInWrappedObject&&) = default;
> -    JSValueInWrappedObject& operator=(JSValueInWrappedObject&&) = default;

Removing this is sufficient to make the thing non-movable.