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

Changeset 276188 in webkit


Ignore:
Timestamp:
Apr 16, 2021, 6:24:04 PM (5 years ago)
Author:
Chris Dumez
Message:

[GPUProcess] Crash under RemoteAudioDestination::render()
​https://bugs.webkit.org/show_bug.cgi?id=224688
<rdar://76643365>

Reviewed by Eric Carlson.

Source/WebKit:

When the connection between the GPUProcess and the WebProcess was severed,
GPUConnectionToWebProcess::didClose() would get called and end up destroying
the RemoteAudioDestination object on the main thread. The issue is that the
RemoteAudioDestination may be playing at the time and we would end up
destroying the RemoteAudioDestination object without stopping rendering
first. As a result, we would crash on the background thread in the
RemoteAudioDestination::render() function, trying to use the m_ringBuffer
data member that got destroyed on the main thread.

To address this, I updated the RemoteAudioDestination destructor so that it
stops rendering if necessary. AudioOutputUnitStop() is synchronous so this
ensures render() is done running on the background thread (and won't be
called again) before proceeding with the destruction of the data members.

Test: webaudio/AudioContext/audiocontext-destruction-crash.html

  • GPUProcess/media/RemoteAudioDestinationManager.cpp:

Updated the class to stop subclassing ThreadSafeRefCounted. This class does
not need RefCounting at all. I updated the call site to use UniqueRef<>.

(WebKit::RemoteAudioDestination::create): Deleted.
Drop this factory function and made the constructor public now that we no longer
subclass ThreadSafeRefCounted and use makeUniqueRef<>() at the call site.

(WebKit::RemoteAudioDestination::scheduleGracefulShutdownIfNeeded): Deleted.
Stop this function now that the destructor takes care of shutting down gracefully.

(WebKit::RemoteAudioDestination::RemoteAudioDestination):
Made the constructor public.

(WebKit::RemoteAudioDestination::render):

  • Stop checking m_protectThisDuringGracefulShutdown on the background thread. This data member is not needed since stop() is synchronous. It was also not thread-safe since m_protectThisDuringGracefulShutdown was set on the main thread and we are on the audio thread here.
  • Similarly, drop the check for m_isPlaying. m_isPlaying is not atomic so the check was not thread safe. Even if m_isPlaying was atomic, m_isPlaying get set to true *after* calling m_audioOutputUnitAdaptor.start() so render() may early return even though we were playing. Also, this check is not needed since we set m_isPlaying to false after calling m_audioOutputUnitAdaptor.stop() and the stop() call is synchronous and should not return until the audio thread stopped rendering.
  • GPUProcess/media/RemoteAudioDestinationManager.h:

LayoutTests:

Add layout test that can flakily reproduce the issue.

  • webaudio/AudioContext/audiocontext-destruction-crash-expected.txt: Added.
  • webaudio/AudioContext/audiocontext-destruction-crash.html: Added.
Location:
trunk
Files:
2 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r276187 r276188  
     12021-04-16  Chris Dumez  <cdumez@apple.com>
     2
     3        [GPUProcess] Crash under RemoteAudioDestination::render()
     4        https://bugs.webkit.org/show_bug.cgi?id=224688
     5        <rdar://76643365>
     6
     7        Reviewed by Eric Carlson.
     8
     9        Add layout test that can flakily reproduce the issue.
     10
     11        * webaudio/AudioContext/audiocontext-destruction-crash-expected.txt: Added.
     12        * webaudio/AudioContext/audiocontext-destruction-crash.html: Added.
     13
    1142021-04-16  Darin Adler  <darin@apple.com>
    215

  • trunk/Source/WebKit/ChangeLog

    r276185 r276188  
     12021-04-16  Chris Dumez  <cdumez@apple.com>
     2
     3        [GPUProcess] Crash under RemoteAudioDestination::render()
     4        https://bugs.webkit.org/show_bug.cgi?id=224688
     5        <rdar://76643365>
     6
     7        Reviewed by Eric Carlson.
     8
     9        When the connection between the GPUProcess and the WebProcess was severed,
     10        GPUConnectionToWebProcess::didClose() would get called and end up destroying
     11        the RemoteAudioDestination object on the main thread. The issue is that the
     12        RemoteAudioDestination may be playing at the time and we would end up
     13        destroying the RemoteAudioDestination object without stopping rendering
     14        first. As a result, we would crash on the background thread in the
     15        RemoteAudioDestination::render() function, trying to use the m_ringBuffer
     16        data member that got destroyed on the main thread.
     17
     18        To address this, I updated the RemoteAudioDestination destructor so that it
     19        stops rendering if necessary. AudioOutputUnitStop() is synchronous so this
     20        ensures render() is done running on the background thread (and won't be
     21        called again) before proceeding with the destruction of the data members.
     22
     23        Test: webaudio/AudioContext/audiocontext-destruction-crash.html
     24
     25        * GPUProcess/media/RemoteAudioDestinationManager.cpp:
     26        Updated the class to stop subclassing ThreadSafeRefCounted. This class does
     27        not need RefCounting at all. I updated the call site to use UniqueRef<>.
     28
     29        (WebKit::RemoteAudioDestination::create): Deleted.
     30        Drop this factory function and made the constructor public now that we no longer
     31        subclass ThreadSafeRefCounted and use makeUniqueRef<>() at the call site.
     32
     33        (WebKit::RemoteAudioDestination::scheduleGracefulShutdownIfNeeded): Deleted.
     34        Stop this function now that the destructor takes care of shutting down gracefully.
     35
     36        (WebKit::RemoteAudioDestination::RemoteAudioDestination):
     37        Made the constructor public.
     38
     39        (WebKit::RemoteAudioDestination::render):
     40        - Stop checking m_protectThisDuringGracefulShutdown on the background thread. This data
     41          member is not needed since stop() is synchronous. It was also not thread-safe since
     42          m_protectThisDuringGracefulShutdown was set on the main thread and we are on the
     43          audio thread here.
     44        - Similarly, drop the check for m_isPlaying. m_isPlaying is not atomic so the check
     45          was not thread safe. Even if m_isPlaying was atomic, m_isPlaying get set to true
     46          *after* calling m_audioOutputUnitAdaptor.start() so render() may early return
     47          even though we were playing. Also, this check is not needed since we set
     48          m_isPlaying to false after calling m_audioOutputUnitAdaptor.stop() and the stop()
     49          call is synchronous and should not return until the audio thread stopped rendering.
     50
     51        * GPUProcess/media/RemoteAudioDestinationManager.h:
     52
    1532021-04-16  Chris Dumez  <cdumez@apple.com>
    254

  • trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.cpp

    r276148 r276188  
    4444namespace WebKit {
    4545
    46 class RemoteAudioDestination
    47     : public ThreadSafeRefCounted<RemoteAudioDestination>
     46class RemoteAudioDestination final
    4847#if PLATFORM(COCOA)
    49     , public WebCore::AudioUnitRenderer
     48    : public WebCore::AudioUnitRenderer
    5049#endif
    5150{
     51    WTF_MAKE_FAST_ALLOCATED;
    5252public:
    53     static Ref<RemoteAudioDestination> create(GPUConnectionToWebProcess& connection, RemoteAudioDestinationIdentifier identifier,
    54         const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
     53    RemoteAudioDestination(GPUConnectionToWebProcess&, RemoteAudioDestinationIdentifier identifier, const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
     54        : m_id(identifier)
     55#if PLATFORM(COCOA)
     56        , m_audioOutputUnitAdaptor(*this)
     57        , m_ringBuffer(makeUniqueRef<WebCore::CARingBuffer>())
     58#endif
     59        , m_renderSemaphore(WTFMove(renderSemaphore))
    5560    {
    56         return adoptRef(*new RemoteAudioDestination(connection, identifier, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore)));
     61        ASSERT(isMainRunLoop());
     62#if PLATFORM(COCOA)
     63        m_audioOutputUnitAdaptor.configure(hardwareSampleRate, numberOfOutputChannels);
     64#endif
    5765    }
    5866
    59     virtual ~RemoteAudioDestination() = default;
    60 
    61     void scheduleGracefulShutdownIfNeeded()
     67    ~RemoteAudioDestination()
    6268    {
    63         if (!m_isPlaying)
    64             return;
    65         m_protectThisDuringGracefulShutdown = this;
    66         stop();
     69        ASSERT(isMainRunLoop());
     70        // Make sure we stop audio rendering and wait for it to finish before destruction.
     71        if (m_isPlaying)
     72            stop();
    6773    }
    6874
    … …  
    9197
    9298        m_isPlaying = false;
    93 
    94         if (m_protectThisDuringGracefulShutdown) {
    95             RELEASE_ASSERT(refCount() == 1);
    96             m_protectThisDuringGracefulShutdown = nullptr;
    97         }
    9899#endif
    99100    }
    … …  
    102103
    103104private:
    104     RemoteAudioDestination(GPUConnectionToWebProcess&, RemoteAudioDestinationIdentifier identifier, const String& inputDeviceId, uint32_t numberOfInputChannels, uint32_t numberOfOutputChannels, float sampleRate, float hardwareSampleRate, IPC::Semaphore&& renderSemaphore)
    105         : m_id(identifier)
    106 #if PLATFORM(COCOA)
    107         , m_audioOutputUnitAdaptor(*this)
    108         , m_ringBuffer(makeUniqueRef<WebCore::CARingBuffer>())
    109 #endif
    110         , m_renderSemaphore(WTFMove(renderSemaphore))
    111     {
    112 #if PLATFORM(COCOA)
    113         m_audioOutputUnitAdaptor.configure(hardwareSampleRate, numberOfOutputChannels);
    114 #endif
    115     }
    116 
    117105#if PLATFORM(COCOA)
    118106    OSStatus render(double sampleTime, uint64_t hostTime, UInt32 numberOfFrames, AudioBufferList* ioData)
    … …  
    121109
    122110        OSStatus status = -1;
    123 
    124         if (m_protectThisDuringGracefulShutdown || !m_isPlaying)
    125             return status;
    126 
    127111        if (m_ringBuffer->fetchIfHasEnoughData(ioData, numberOfFrames, m_startFrame)) {
    128112            m_startFrame += numberOfFrames;
    … …  
    140124
    141125    RemoteAudioDestinationIdentifier m_id;
    142 
    143     RefPtr<RemoteAudioDestination> m_protectThisDuringGracefulShutdown;
    144126
    145127#if PLATFORM(COCOA)
    … …  
    164146{
    165147    auto newID = RemoteAudioDestinationIdentifier::generateThreadSafe();
    166     auto destination = RemoteAudioDestination::create(m_gpuConnectionToWebProcess, newID, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore));
    167     m_audioDestinations.add(newID, destination.copyRef());
     148    auto destination = makeUniqueRef<RemoteAudioDestination>(m_gpuConnectionToWebProcess, newID, inputDeviceId, numberOfInputChannels, numberOfOutputChannels, sampleRate, hardwareSampleRate, WTFMove(renderSemaphore));
     149    m_audioDestinations.add(newID, WTFMove(destination));
    168150    completionHandler(newID);
    169151}
    … …  
    171153void RemoteAudioDestinationManager::deleteAudioDestination(RemoteAudioDestinationIdentifier identifier, CompletionHandler<void()>&& completionHandler)
    172154{
    173     auto destination = m_audioDestinations.take(identifier);
    174     if (destination)
    175         destination->scheduleGracefulShutdownIfNeeded();
     155    m_audioDestinations.remove(identifier);
    176156    completionHandler();
    177157

  • trunk/Source/WebKit/GPUProcess/media/RemoteAudioDestinationManager.h

    r276148 r276188  
    7272#endif
    7373
    74     HashMap<RemoteAudioDestinationIdentifier, Ref<RemoteAudioDestination>> m_audioDestinations;
     74    HashMap<RemoteAudioDestinationIdentifier, UniqueRef<RemoteAudioDestination>> m_audioDestinations;
    7575    GPUConnectionToWebProcess& m_gpuConnectionToWebProcess;
    7676};
Note: See TracChangeset for help on using the changeset viewer.