⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 275933 in webkit


Ignore:
Timestamp:
Apr 13, 2021, 11:12:15 PM (5 years ago)
Author:
commit-queue@webkit.org
Message:

AudioSourceProviderAVFObjC should lock all multithreaded objects
​https://bugs.webkit.org/show_bug.cgi?id=224230

Patch by Kimmo Kinnunen <​kkinnunen@apple.com> on 2021-04-13
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):

Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r275931 r275933  
     12021-04-13  Kimmo Kinnunen  <kkinnunen@apple.com>
     2
     3        AudioSourceProviderAVFObjC should lock all multithreaded objects
     4        https://bugs.webkit.org/show_bug.cgi?id=224230
     5
     6        Reviewed by Eric Carlson.
     7
     8        Lock following objects with TapStorage::lock:
     9          AudioSourceProviderAVFObjC::m_tapStorage
     10          AudioSourceProviderAVFObjC::m_avPlayerItem
     11
     12        Do not clear the tapStorage nor the _this pointer from
     13        the audio thread. This is a race that cannot be overcome
     14        since the main thread needs to access the m_tapStorage pointer to
     15        lock the m_tapStorage pointer for modification.
     16
     17        Initialize m_tap in more robust way, in case initialization fails.
     18
     19        Do not leave stale m_tapStorage present when destroying the mixer.
     20
     21        * platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h:
     22        * platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:
     23        (WebCore::AudioSourceProviderAVFObjC::~AudioSourceProviderAVFObjC):
     24        (WebCore::AudioSourceProviderAVFObjC::destroyMixIfNeeded):
     25        (WebCore::AudioSourceProviderAVFObjC::createMixIfNeeded):
     26        (WebCore::AudioSourceProviderAVFObjC::initCallback):
     27        (WebCore::AudioSourceProviderAVFObjC::finalizeCallback):
     28
    1292021-04-13  Fujii Hironori  <Hironori.Fujii@sony.com>
    230

  • trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.h

    r275897 r275933  
    8686    static void processCallback(MTAudioProcessingTapRef, CMItemCount, MTAudioProcessingTapFlags, AudioBufferList*, CMItemCount*, MTAudioProcessingTapFlags*);
    8787
    88     void init(void* clientInfo, void** tapStorageOut);
    89     void finalize();
    9088    void prepare(CMItemCount maxFrames, const AudioStreamBasicDescription *processingFormat);
    9189    void unprepare();

  • trunk/Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm

    r275897 r275933  
    8787AudioSourceProviderAVFObjC::~AudioSourceProviderAVFObjC()
    8888{
     89    // FIXME: this is not correct, as this indicates that there might be simultaneous calls
     90    // to the destructor and a member function. This undefined behavior will be addressed in the future
     91    // commits. https://bugs.webkit.org/show_bug.cgi?id=224480
    8992    setClient(nullptr);
    90     if (m_tapStorage) {
    91         auto locker = holdLock(m_tapStorage->lock);
    92         m_tapStorage->_this = nullptr;
    93     }
    94 
    95     m_tapStorage = nullptr;
    9693}
    9794
    … …  
    186183    if (!m_avAudioMix)
    187184        return;
    188 
     185    ASSERT(m_tapStorage);
     186    auto locker = holdLock(m_tapStorage->lock);
    189187    if (m_avPlayerItem)
    190188        [m_avPlayerItem setAudioMix:nil];
    … …  
    192190    m_avAudioMix.clear();
    193191    m_tap.clear();
     192    m_tapStorage->_this = nullptr;
     193    m_tapStorage = nullptr;
     194    // Call unprepare, since Tap cannot call it after clear.
     195    unprepare();
    194196    m_weakFactory.revokeAll();
    195197}
    … …  
    201203
    202204    ASSERT(!m_avAudioMix);
    203 
    204     m_avAudioMix = adoptNS([PAL::allocAVMutableAudioMixInstance() init]);
     205    ASSERT(!m_tapStorage);
     206    ASSERT(!m_tap);
     207
     208    auto tapStorage = adoptRef(new TapStorage(this));
     209    auto locker = holdLock(tapStorage->lock);
    205210
    206211    MTAudioProcessingTapCallbacks callbacks = {
    207212        0,
    208         this,
     213        tapStorage.get(),
    209214        initCallback,
    210215        finalizeCallback,
    … …  
    216221    MTAudioProcessingTapRef tap = nullptr;
    217222    OSStatus status = MTAudioProcessingTapCreate(kCFAllocatorDefault, &callbacks, 1, &tap);
    218     ASSERT(tap);
    219     ASSERT(m_tap == tap);
    220223    if (status != noErr) {
    221         m_tap = nullptr;
    222         return;
    223     }
     224        if (tap)
     225            CFRelease(tap);
     226        return;
     227    }
     228    m_tap = adoptCF(tap);
     229    m_tapStorage = WTFMove(tapStorage);
     230    m_avAudioMix = adoptNS([PAL::allocAVMutableAudioMixInstance() init]);
    224231
    225232    RetainPtr<AVMutableAudioMixInputParameters> parameters = adoptNS([PAL::allocAVMutableAudioMixInputParametersInstance() init]);
    … …  
    236243void AudioSourceProviderAVFObjC::initCallback(MTAudioProcessingTapRef tap, void* clientInfo, void** tapStorageOut)
    237244{
    238     ASSERT(tap);
    239     AudioSourceProviderAVFObjC* _this = static_cast<AudioSourceProviderAVFObjC*>(clientInfo);
    240     _this->m_tap = adoptCF(tap);
    241     _this->m_tapStorage = adoptRef(new TapStorage(_this));
    242     _this->init(clientInfo, tapStorageOut);
    243     *tapStorageOut = _this->m_tapStorage.get();
    244 
     245    ASSERT_UNUSED(tap, tap);
     246    TapStorage* tapStorage = static_cast<TapStorage*>(clientInfo);
     247    *tapStorageOut = tapStorage;
    245248    // ref balanced by deref in finalizeCallback:
    246     _this->m_tapStorage->ref();
     249    tapStorage->ref();
    247250}
    248251
    … …  
    251254    ASSERT(tap);
    252255    TapStorage* tapStorage = static_cast<TapStorage*>(MTAudioProcessingTapGetStorage(tap));
    253 
    254     {
    255         auto locker = holdLock(tapStorage->lock);
    256         if (tapStorage->_this)
    257             tapStorage->_this->finalize();
    258     }
    259256    tapStorage->deref();
    260257}
    … …  
    291288    if (tapStorage->_this)
    292289        tapStorage->_this->process(tap, numberFrames, flags, bufferListInOut, numberFramesOut, flagsOut);
    293 }
    294 
    295 void AudioSourceProviderAVFObjC::init(void* clientInfo, void** tapStorageOut)
    296 {
    297     ASSERT(clientInfo == this);
    298     UNUSED_PARAM(clientInfo);
    299     *tapStorageOut = this;
    300 }
    301 
    302 void AudioSourceProviderAVFObjC::finalize()
    303 {
    304     if (m_tapStorage) {
    305         m_tapStorage->_this = nullptr;
    306         m_tapStorage = nullptr;
    307     }
    308290}
    309291
Note: See TracChangeset for help on using the changeset viewer.