Bug 181597 – ASSERTION FAILED: m_scriptExecutionContext under WebCore::AudioContext::isPlayingAudioDidChange()

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

Created attachment 331211 [details]
Crash log

This flaky assertion failure is seen on EWS bots with LayoutTest fast/history/page-cache-suspended-audiocontext.html

ASSERTION FAILED: m_scriptExecutionContext
/Volumes/Data/EWS/WebKit/Source/WebCore/Modules/webaudio/AudioContext.cpp(348) : WebCore::Document *WebCore::AudioContext::document() const
1   0x10fc920fd WTFCrash
2   0x11c407087 WebCore::AudioContext::document() const
3   0x11c420e21 WebCore::AudioContext::isPlayingAudioDidChange()::$_4::operator()() const
4   0x11c420d79 WTF::Function<void ()>::CallableWrapper<WebCore::AudioContext::isPlayingAudioDidChange()::$_4>::call()
5   0x10fcae26b WTF::Function<void ()>::operator()() const
6   0x10fcd1544 WTF::dispatchFunctionsFromMainThread()
7   0x10fcd46b1 WTF::timerFired(__CFRunLoopTimer*, void*)
8   0x7fff7d870e04 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__
9   0x7fff7d870a93 __CFRunLoopDoTimer
10  0x7fff7d8705ea __CFRunLoopDoTimers
11  0x7fff7d867fc1 __CFRunLoopRun
12  0x7fff7d867544 CFRunLoopRunSpecific
13  0x10def88b7 runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&)
14  0x10def6d0d runTestingServerLoop()
15  0x10def62a2 dumpRenderTree(int, char const**)
16  0x10def92ed DumpRenderTreeMain(int, char const**)
17  0x10df74b32 main
18  0x7fff935b0235 start
19  0x2

Examples of recent failures on EWS:
https://bugs.webkit.org/show_bug.cgi?id=181466
https://bugs.webkit.org/show_bug.cgi?id=181510

[Radar WebKit Bug Importer]

<rdar://problem/36474088>

[Alexey Proskuryakov]

I see crash logs like this on many different tests, so it may be unfeasible to mark in TestExpectations.

[Chris Dumez]

void AudioContext::isPlayingAudioDidChange()
{
    // Make sure to call Document::updateIsPlayingMedia() on the main thread, since
    // we could be on the audio I/O thread here and the call into WebCore could block.
    callOnMainThread([protectedThis = makeRef(*this)] {
        if (protectedThis->document())
            protectedThis->document()->updateIsPlayingMedia();
    });
}

Document* AudioContext::document() const
{
    ASSERT(m_scriptExecutionContext);
    return downcast<Document>(m_scriptExecutionContext);
}

The code tries to null-check document() but document() asserts that it cannot return null. Even if the AudioContext had a scriptExecutionContext/Document when isPlayingAudioDidChange() was called (which I did not verify), there is definitely NO guarantee it still has one once the lambda passed to callOnMainThread() is called. Therefore, this code is clearly wrong.

[Jer Noble]

Created attachment 336552 [details]
Patch

[Eric Carlson]

Comment on attachment 336552 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioContext.h:91
> +    Document& document() const; // ASSERTs if document no longer exists.

Nit: the comment is no longer accurate.

[Jer Noble]

Created attachment 337007 [details]
Patch

[youenn fablet]

Comment on attachment 337007 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Rather than rely on null-checking Document, we shuold guarantee that no callbacks are acted upon

should

> Source/WebCore/Modules/webaudio/AudioContext.cpp:334
>  {

Maybe we could keep something like ASSERT(!m_isStopScheduled) modulo a small change in AudioContext::stop()

> Source/WebCore/Modules/webaudio/AudioContext.cpp:-354
> -    if (Frame* frame = document ? document->frame() : nullptr) {

I am not clear how we can guarantee that any PlatformMediaSessionClient function can never be called after active DOM objects get stopped.
We have that same if check in MediaStream for instance.

> Source/WebCore/platform/GenericTaskQueue.h:74
> +    void postTask(WTF::Function<void()>&& function)

I think we no longer need WTF::

> Source/WebCore/platform/GenericTaskQueue.h:117
>          cancelAllTasks();

For close we will create a lock twice.
Couldn't it cause issues if we call close, execute cancelAllTasks, exit cancelAllTasks, suspend the thread, and then in another thread call enqueueTask.
Maybe it would be simpler to inline the two lines of cancelAllTask in close and add a lock at the beginning of close.

> Source/WebCore/platform/GenericTaskQueue.h:128
>      }

hasPendingTasks() is probably not useful and misleading in a multithreaded environment without locks.
Should we not expose it?

Discussing with Eric, it might make sense to create a specific GenericTaskQueue<CallOnMainThreadDispatcher> specialization or maybe better a separate class.

[Jer Noble]

Comment on attachment 337007 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        Rather than rely on null-checking Document, we shuold guarantee that no callbacks are acted upon
> 
> should

Whoops. Fixed.

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:334
>>  {
> 
> Maybe we could keep something like ASSERT(!m_isStopScheduled) modulo a small change in AudioContext::stop()

I'm not even sure the comment "Usually ScriptExecutionContext calls stop twice" is even true any longer, which would mean we could get rid of m_isStopScheduled entirely.

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:-354
>> -    if (Frame* frame = document ? document->frame() : nullptr) {
> 
> I am not clear how we can guarantee that any PlatformMediaSessionClient function can never be called after active DOM objects get stopped.
> We have that same if check in MediaStream for instance.

The danger is not that the class will perform work after being stopped, but that it will be called after it's document is destroyed (and thus immediately before it itself is destroyed).  And that would mean that something that's not an ActiveDOMObject is keeping the AudioContext (or the MediaStream) alive longer than the Document. In this case, the AudioContext itself was scheduling work after Document destruction, explicitly by delaying calls to uninitialize() till the next run loop.  Since that's been fixed, there's nothing left that will hold a reference to AudioContext (aside from other ActiveDOMObjects).

>> Source/WebCore/platform/GenericTaskQueue.h:117
>>          cancelAllTasks();
> 
> For close we will create a lock twice.
> Couldn't it cause issues if we call close, execute cancelAllTasks, exit cancelAllTasks, suspend the thread, and then in another thread call enqueueTask.
> Maybe it would be simpler to inline the two lines of cancelAllTask in close and add a lock at the beginning of close.

I don't really like duplicating that code, but it may be necessary.  Another alternative would be to lock around m_isClosed = true first; ensuring no new tasks can be enqueued, and then cancel the remaining ones.

>> Source/WebCore/platform/GenericTaskQueue.h:128
>>      }
> 
> hasPendingTasks() is probably not useful and misleading in a multithreaded environment without locks.
> Should we not expose it?
> 
> Discussing with Eric, it might make sense to create a specific GenericTaskQueue<CallOnMainThreadDispatcher> specialization or maybe better a separate class.

There's no reason to lock on hasPendingTasks() since it doesn't modify state.  I don't think it's useful to try and solve "misleading" threading problems; those are the responsibility of the caller.

[youenn fablet]

> >> Source/WebCore/Modules/webaudio/AudioContext.cpp:-354
> >> -    if (Frame* frame = document ? document->frame() : nullptr) {
> > 
> > I am not clear how we can guarantee that any PlatformMediaSessionClient function can never be called after active DOM objects get stopped.
> > We have that same if check in MediaStream for instance.
> 
> The danger is not that the class will perform work after being stopped, but
> that it will be called after it's document is destroyed (and thus
> immediately before it itself is destroyed).  And that would mean that
> something that's not an ActiveDOMObject is keeping the AudioContext (or the
> MediaStream) alive longer than the Document. In this case, the AudioContext
> itself was scheduling work after Document destruction, explicitly by
> delaying calls to uninitialize() till the next run loop.  Since that's been
> fixed, there's nothing left that will hold a reference to AudioContext
> (aside from other ActiveDOMObjects).

I would guess a page could keep a reference to a MediaStream JS object of one of its sub-frame.
When the sub-frame goes away, the document is gone but the MediaStream is still there.
Maybe PlatformMediaSessionClient is made so that any PlatformMediaSessionClient user goes away with the document?

Also, it seems document() is a public method. Maybe we should make it private if we make it return a Document&.

> There's no reason to lock on hasPendingTasks() since it doesn't modify
> state.  I don't think it's useful to try and solve "misleading" threading
> problems; those are the responsibility of the caller.

The other advantage of a dedicated class is that there is no lock required for the simple case.
The disadvantage may be more code but maybe there is a way to make it small enough.
If the lock-version of the queue has a non-lock version of the queue as a member and we only expose enqueue and close, the added code should be fairly small.

[Jer Noble]

(In reply to youenn fablet from comment #9)
> > >> Source/WebCore/Modules/webaudio/AudioContext.cpp:-354
> > >> -    if (Frame* frame = document ? document->frame() : nullptr) {
> > > 
> > > I am not clear how we can guarantee that any PlatformMediaSessionClient function can never be called after active DOM objects get stopped.
> > > We have that same if check in MediaStream for instance.
> > 
> > The danger is not that the class will perform work after being stopped, but
> > that it will be called after it's document is destroyed (and thus
> > immediately before it itself is destroyed).  And that would mean that
> > something that's not an ActiveDOMObject is keeping the AudioContext (or the
> > MediaStream) alive longer than the Document. In this case, the AudioContext
> > itself was scheduling work after Document destruction, explicitly by
> > delaying calls to uninitialize() till the next run loop.  Since that's been
> > fixed, there's nothing left that will hold a reference to AudioContext
> > (aside from other ActiveDOMObjects).
> 
> I would guess a page could keep a reference to a MediaStream JS object of
> one of its sub-frame.
> When the sub-frame goes away, the document is gone but the MediaStream is
> still there.

By "page" do you mean javascript running in the top-document? Because clearing the sub-document which created the node will clear all the wrappers referenced by the top-document. I.e.:

> var iframe = document.createElement('iframe')
< undefined
> document.body.appendChild(iframe)
< <iframe>…</iframe>
> var video = iframe.contentDocument.createElement('video');
< undefined
> iframe.contentDocument.body.appendChild(video);
< <video></video>
> iframe.src='about:blank'
< "about:blank"
> video
< 

> Maybe PlatformMediaSessionClient is made so that any
> PlatformMediaSessionClient user goes away with the document?

PlatformMediaSession (the object that references PlatformMediaSessionClient) is created and destroyed by the client, so its lifetime that matches the client itself.

> Also, it seems document() is a public method. Maybe we should make it
> private if we make it return a Document&.

Document& document() is also a public method on Node (and a number of other classes). Though it does look like no-one uses it outside AudioContext.

> > There's no reason to lock on hasPendingTasks() since it doesn't modify
> > state.  I don't think it's useful to try and solve "misleading" threading
> > problems; those are the responsibility of the caller.
> 
> The other advantage of a dedicated class is that there is no lock required
> for the simple case.

The WTF::Lock is extremely cheap, both in memory and CPU usage, especially for the simple case.

> The disadvantage may be more code but maybe there is a way to make it small
> enough.
> If the lock-version of the queue has a non-lock version of the queue as a
> member and we only expose enqueue and close, the added code should be fairly
> small.

So, a GenericTaskQueue and a ThreadSafeGenericTaskQueue?

[Jer Noble]

Created attachment 337283 [details]
Patch

[youenn fablet]

Comment on attachment 337283 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Test: webaudio/audiocontext-closed.html

It seems the spec is somehow lagging behind implementations here.
Does this test pass on Chrome and Firefox?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:377
> +    if (isStopped())

Some checks are doing ASSERT(isMainThread()).
Maybe it makes sense to add it to isStopped?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:409
> +    if (isClosed())

Some other similar checks are using isStopped() and not isClosed().
What is the reason for this difference?

The spec does not say to do these checks, maybe we should file a bug?
Providing wpt tests would be also a great way to push for these changes.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:753
> +    ASSERT(isAudioThread || isStopped());

isAudioThread()

> Source/WebCore/Modules/webaudio/AudioContext.h:79
>  // For thread safety between the audio thread and the main thread, it has a rendering graph locking mechanism. 

AudioContext might not need to be ThreadSafeRefCounted anymore with the task queue.

> Source/WebCore/platform/GenericTaskQueue.h:143
> +class ThreadSafeGenericTaskQueue : public GenericTaskQueue<T> {

Mostly a style issue but since we are not using inheritance/polymorphism, I would tend to make GenericTaskQueue a member variable of ThreadSafeGenericTaskQueue.
Maybe make it a friend to get access to enqueueTaskFunction.

If we were to not expose pending task counter, we could simplify our locking. I can see the point of future-bulletproofing it though.


> Source/WebCore/platform/GenericTaskQueue.h:150
> +    ThreadSafeGenericTaskQueue(T& t)

explicit

[Jer Noble]

(In reply to youenn fablet from comment #12)
> Comment on attachment 337283 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337283&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Test: webaudio/audiocontext-closed.html
> 
> It seems the spec is somehow lagging behind implementations here.
> Does this test pass on Chrome and Firefox?

The spec has been updated to state that closed AudioContext's can't create nodes. Chrome matches our behavior, except that they don't match the spec for OfflineAudioContexts. Firefox matches our behavior as well.

> > Source/WebCore/Modules/webaudio/AudioContext.cpp:377
> > +    if (isStopped())
> 
> Some checks are doing ASSERT(isMainThread()).
> Maybe it makes sense to add it to isStopped?
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:409
> > +    if (isClosed())
> 
> Some other similar checks are using isStopped() and not isClosed().
> What is the reason for this difference?

The spec says that closed AudioContext's can't create nodes, but can create buffers. That's the reason for the difference. Ref: <https://webaudio.github.io/web-audio-api/#dom-audiocontextstate-closed>

> The spec does not say to do these checks, maybe we should file a bug?
> Providing wpt tests would be also a great way to push for these changes.
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:753
> > +    ASSERT(isAudioThread || isStopped());
> 
> isAudioThread()

Fixed.

> > Source/WebCore/Modules/webaudio/AudioContext.h:79
> >  // For thread safety between the audio thread and the main thread, it has a rendering graph locking mechanism. 
> 
> AudioContext might not need to be ThreadSafeRefCounted anymore with the task
> queue.

It might not, but I'm not ready to audit all the various AudioNodes to ensure they don't deref the AudioContext on a background thread.

> > Source/WebCore/platform/GenericTaskQueue.h:143
> > +class ThreadSafeGenericTaskQueue : public GenericTaskQueue<T> {
> 
> Mostly a style issue but since we are not using inheritance/polymorphism, I
> would tend to make GenericTaskQueue a member variable of
> ThreadSafeGenericTaskQueue.
> Maybe make it a friend to get access to enqueueTaskFunction.

Actually, that would make sure no one casted ThreadSafeGenericTaskQueue to GenericTaskQueue and the wrong functions get called; I'll do it.

> If we were to not expose pending task counter, we could simplify our
> locking. I can see the point of future-bulletproofing it though.

We can solve this by making the m_pendingActions an atomic type; no locks needed there.

> 
> > Source/WebCore/platform/GenericTaskQueue.h:150
> > +    ThreadSafeGenericTaskQueue(T& t)
> 
> explicit

[Jer Noble]

(In reply to Jer Noble from comment #13)
> (In reply to youenn fablet from comment #12)
> > Comment on attachment 337283 [details]
> > > Source/WebCore/Modules/webaudio/AudioContext.cpp:753
> > > +    ASSERT(isAudioThread || isStopped());
> > 
> > isAudioThread()
> 
> Fixed.

Nope, isAudioThread is a local bool.

[Jer Noble]

Created attachment 337370 [details]
Patch

[EWS Watchlist]

Comment on attachment 337370 [details]
Patch

Attachment 337370 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7227840

New failing tests:
webrtc/getUserMedia-webaudio-autoplay.html

[EWS Watchlist]

Created attachment 337378 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[youenn fablet]

Comment on attachment 337370 [details]
Patch

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

> Source/WebCore/Modules/webaudio/AudioContext.cpp:410
> +    if (isClosed())

ASSERT(isMainThread()) is sometimes but not always called for these calls.
We could remove a lot of these if we would add this assertion in isClosed/isStopped/isInitialized.
It probably is not safe to check for isClosed/isStopped from a background thread anyway.

> Source/WebCore/Modules/webaudio/AudioContext.h:275
> +    const Document* hostingDocument() const override;

final?

> Source/WebCore/Modules/webaudio/AudioContext.h:373
> +    std::unique_ptr<ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>> m_taskQueue;

Could be UniqueRef I guess since we are not nullify it.
Or maybe simply a ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>?

[Jer Noble]

(In reply to youenn fablet from comment #18)
> Comment on attachment 337370 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=337370&action=review
> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:410
> > +    if (isClosed())
> 
> ASSERT(isMainThread()) is sometimes but not always called for these calls.
> We could remove a lot of these if we would add this assertion in
> isClosed/isStopped/isInitialized.
> It probably is not safe to check for isClosed/isStopped from a background
> thread anyway.

Except it's not unsafe to call those in a background thread, since they don't mutate state. It /is/ unsafe to call the methods where the ASSERT(isMainThread()) lives on a background thread, since they do mutate state.

> > Source/WebCore/Modules/webaudio/AudioContext.h:275
> > +    const Document* hostingDocument() const override;
> 
> final?

Sure.

> > Source/WebCore/Modules/webaudio/AudioContext.h:373
> > +    std::unique_ptr<ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>> m_taskQueue;
> 
> Could be UniqueRef I guess since we are not nullify it.
> Or maybe simply a ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>?

UniqueRef would be fine. The reason to use a unique holder is so you don't need to expose GenericTaskQueue.h in the header.

[youenn fablet]

> Except it's not unsafe to call those in a background thread, since they
> don't mutate state. It /is/ unsafe to call the methods where the
> ASSERT(isMainThread()) lives on a background thread, since they do mutate
> state.

Some code doing if (!isStopped()) in a background thread would probably be unsafe.
isInitialized() is also potentially unsafe on its own.

> > > Source/WebCore/Modules/webaudio/AudioContext.h:373
> > > +    std::unique_ptr<ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>> m_taskQueue;
> > 
> > Could be UniqueRef I guess since we are not nullify it.
> > Or maybe simply a ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>?
> 
> UniqueRef would be fine. The reason to use a unique holder is so you don't
> need to expose GenericTaskQueue.h in the header.

A separate allocation seems less efficient.

Do we have to include GenericTaskQueue.h in the header if its (and OfflineAudioContext) constructors/destructors are defined in cpp files (with = default; if needed)?

[Jer Noble]

(In reply to youenn fablet from comment #20)
> > Except it's not unsafe to call those in a background thread, since they
> > don't mutate state. It /is/ unsafe to call the methods where the
> > ASSERT(isMainThread()) lives on a background thread, since they do mutate
> > state.
> 
> Some code doing if (!isStopped()) in a background thread would probably be
> unsafe.

No, it's literally not unsafe. It's probably a bad idea, but if you implemented a lock around a mutable function to ensure that it /was/ safe, then you'd still assert if you called isStopped() on a background thread.

> isInitialized() is also potentially unsafe on its own.

We don't throw ASSERTs everywhere that we null-check pointers.

> > > > Source/WebCore/Modules/webaudio/AudioContext.h:373
> > > > +    std::unique_ptr<ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>> m_taskQueue;
> > > 
> > > Could be UniqueRef I guess since we are not nullify it.
> > > Or maybe simply a ThreadSafeGenericTaskQueue<CallOnMainThreadDispatcher>?
> > 
> > UniqueRef would be fine. The reason to use a unique holder is so you don't
> > need to expose GenericTaskQueue.h in the header.
> 
> A separate allocation seems less efficient.

Seems or is? And by how much?  This line of reasoning leads to inlining everything in the header. Speculative, unnecessary optimization is not a good thing.

> Do we have to include GenericTaskQueue.h in the header if its (and
> OfflineAudioContext) constructors/destructors are defined in cpp files (with
> = default; if needed)?

Yes. Everyone who uses the class must be able to calculate its size, and that requires pulling in the header of every member variable.

[EWS Watchlist]

Comment on attachment 337370 [details]
Patch

Attachment 337370 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7229461

New failing tests:
webrtc/getUserMedia-webaudio-autoplay.html

[EWS Watchlist]

Created attachment 337387 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

(In reply to Jer Noble from comment #21)
> (In reply to youenn fablet from comment #20)
> > > Except it's not unsafe to call those in a background thread, since they
> > > don't mutate state. It /is/ unsafe to call the methods where the
> > > ASSERT(isMainThread()) lives on a background thread, since they do mutate
> > > state.
> > 
> > Some code doing if (!isStopped()) in a background thread would probably be
> > unsafe.
> 
> No, it's literally not unsafe. It's probably a bad idea

Debug assertions are friendly reminders of the model we are trying to push for.
It seems to me the model is that the audio background thread should not need to access these variables.

> > A separate allocation seems less efficient.
> 
> Seems or is? And by how much?  This line of reasoning leads to inlining
> everything in the header. Speculative, unnecessary optimization is not a
> good thing.

I do not have numbers.
Looking at existing WebKit code, GenericTaskQueue is usually allocated as part of its parent object.
I am not clear where to draw the line here, would be good to define some guidelines I guess.

[Jer Noble]

Created attachment 370946 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 370946 [details]
Patch

Clearing flags on attachment: 370946

Committed r245889: <https://trac.webkit.org/changeset/245889>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.