Since the vector is cleared in releaseMemory.
Created attachment 451347 [details] Patch
Comment on attachment 451347 [details] Patch r=me.
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?
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.
Committed r289476 (247019@trunk): <https://commits.webkit.org/247019@trunk>
<rdar://problem/88693354>
(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.
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.