Modified: trunk/Source/WebCore/ChangeLog (275932 => 275933)
--- trunk/Source/WebCore/ChangeLog 2021-04-14 05:36:31 UTC (rev 275932)
+++ trunk/Source/WebCore/ChangeLog 2021-04-14 06:12:15 UTC (rev 275933)
@@ -1,3 +1,31 @@
+2021-04-13 Kimmo Kinnunen <[email protected]>
+
+ AudioSourceProviderAVFObjC should lock all multithreaded objects
+ https://bugs.webkit.org/show_bug.cgi?id=224230
+
+ Reviewed by Eric Carlson.
+
+ Lock following objects with TapStorage::lock:
+ AudioSourceProviderAVFObjC::m_tapStorage
+ AudioSourceProviderAVFObjC::m_avPlayerItem
+
+ Do not clear the tapStorage nor the _this pointer from
+ the audio thread. This is a race that cannot be overcome
+ since the main thread needs to access the m_tapStorage pointer to
+ lock the m_tapStorage pointer for modification.
+
+ Initialize m_tap in more robust way, in case initialization fails.
+
+ Do not leave stale m_tapStorage present when destroying the mixer.
+
+ * platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h:
+ * platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:
+ (WebCore::AudioSourceProviderAVFObjC::~AudioSourceProviderAVFObjC):
+ (WebCore::AudioSourceProviderAVFObjC::destroyMixIfNeeded):
+ (WebCore::AudioSourceProviderAVFObjC::createMixIfNeeded):
+ (WebCore::AudioSourceProviderAVFObjC::initCallback):
+ (WebCore::AudioSourceProviderAVFObjC::finalizeCallback):
+
2021-04-13 Fujii Hironori <[email protected]>
Table caption jumps to the bottom of the table after simplified table relayout
Modified: trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h (275932 => 275933)
--- trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h 2021-04-14 05:36:31 UTC (rev 275932)
+++ trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h 2021-04-14 06:12:15 UTC (rev 275933)
@@ -85,8 +85,6 @@
static void unprepareCallback(MTAudioProcessingTapRef);
static void processCallback(MTAudioProcessingTapRef, CMItemCount, MTAudioProcessingTapFlags, AudioBufferList*, CMItemCount*, MTAudioProcessingTapFlags*);
- void init(void* clientInfo, void** tapStorageOut);
- void finalize();
void prepare(CMItemCount maxFrames, const AudioStreamBasicDescription *processingFormat);
void unprepare();
void process(MTAudioProcessingTapRef, CMItemCount numberFrames, MTAudioProcessingTapFlags flagsIn, AudioBufferList *bufferListInOut, CMItemCount *numberFramesOut, MTAudioProcessingTapFlags *flagsOut);
Modified: trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm (275932 => 275933)
--- trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm 2021-04-14 05:36:31 UTC (rev 275932)
+++ trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm 2021-04-14 06:12:15 UTC (rev 275933)
@@ -86,13 +86,10 @@
AudioSourceProviderAVFObjC::~AudioSourceProviderAVFObjC()
{
+ // FIXME: this is not correct, as this indicates that there might be simultaneous calls
+ // to the destructor and a member function. This undefined behavior will be addressed in the future
+ // commits. https://bugs.webkit.org/show_bug.cgi?id=224480
setClient(nullptr);
- if (m_tapStorage) {
- auto locker = holdLock(m_tapStorage->lock);
- m_tapStorage->_this = nullptr;
- }
-
- m_tapStorage = nullptr;
}
void AudioSourceProviderAVFObjC::provideInput(AudioBus* bus, size_t framesToProcess)
@@ -185,12 +182,17 @@
{
if (!m_avAudioMix)
return;
-
+ ASSERT(m_tapStorage);
+ auto locker = holdLock(m_tapStorage->lock);
if (m_avPlayerItem)
[m_avPlayerItem setAudioMix:nil];
[m_avAudioMix setInputParameters:@[ ]];
m_avAudioMix.clear();
m_tap.clear();
+ m_tapStorage->_this = nullptr;
+ m_tapStorage = nullptr;
+ // Call unprepare, since Tap cannot call it after clear.
+ unprepare();
m_weakFactory.revokeAll();
}
@@ -200,12 +202,15 @@
return;
ASSERT(!m_avAudioMix);
+ ASSERT(!m_tapStorage);
+ ASSERT(!m_tap);
- m_avAudioMix = adoptNS([PAL::allocAVMutableAudioMixInstance() init]);
+ auto tapStorage = adoptRef(new TapStorage(this));
+ auto locker = holdLock(tapStorage->lock);
MTAudioProcessingTapCallbacks callbacks = {
0,
- this,
+ tapStorage.get(),
initCallback,
finalizeCallback,
prepareCallback,
@@ -215,12 +220,14 @@
MTAudioProcessingTapRef tap = nullptr;
OSStatus status = MTAudioProcessingTapCreate(kCFAllocatorDefault, &callbacks, 1, &tap);
- ASSERT(tap);
- ASSERT(m_tap == tap);
if (status != noErr) {
- m_tap = nullptr;
+ if (tap)
+ CFRelease(tap);
return;
}
+ m_tap = adoptCF(tap);
+ m_tapStorage = WTFMove(tapStorage);
+ m_avAudioMix = adoptNS([PAL::allocAVMutableAudioMixInstance() init]);
RetainPtr<AVMutableAudioMixInputParameters> parameters = adoptNS([PAL::allocAVMutableAudioMixInputParametersInstance() init]);
[parameters setAudioTapProcessor:m_tap.get()];
@@ -235,15 +242,11 @@
void AudioSourceProviderAVFObjC::initCallback(MTAudioProcessingTapRef tap, void* clientInfo, void** tapStorageOut)
{
- ASSERT(tap);
- AudioSourceProviderAVFObjC* _this = static_cast<AudioSourceProviderAVFObjC*>(clientInfo);
- _this->m_tap = adoptCF(tap);
- _this->m_tapStorage = adoptRef(new TapStorage(_this));
- _this->init(clientInfo, tapStorageOut);
- *tapStorageOut = _this->m_tapStorage.get();
-
+ ASSERT_UNUSED(tap, tap);
+ TapStorage* tapStorage = static_cast<TapStorage*>(clientInfo);
+ *tapStorageOut = tapStorage;
// ref balanced by deref in finalizeCallback:
- _this->m_tapStorage->ref();
+ tapStorage->ref();
}
void AudioSourceProviderAVFObjC::finalizeCallback(MTAudioProcessingTapRef tap)
@@ -250,12 +253,6 @@
{
ASSERT(tap);
TapStorage* tapStorage = static_cast<TapStorage*>(MTAudioProcessingTapGetStorage(tap));
-
- {
- auto locker = holdLock(tapStorage->lock);
- if (tapStorage->_this)
- tapStorage->_this->finalize();
- }
tapStorage->deref();
}
@@ -292,21 +289,6 @@
tapStorage->_this->process(tap, numberFrames, flags, bufferListInOut, numberFramesOut, flagsOut);
}
-void AudioSourceProviderAVFObjC::init(void* clientInfo, void** tapStorageOut)
-{
- ASSERT(clientInfo == this);
- UNUSED_PARAM(clientInfo);
- *tapStorageOut = this;
-}
-
-void AudioSourceProviderAVFObjC::finalize()
-{
- if (m_tapStorage) {
- m_tapStorage->_this = nullptr;
- m_tapStorage = nullptr;
- }
-}
-
void AudioSourceProviderAVFObjC::prepare(CMItemCount maxFrames, const AudioStreamBasicDescription *processingFormat)
{
ASSERT(maxFrames >= 0);
