Author: sigbjornf
Project: chromium
Changelist: https://chromium.googlesource.com/chromium/src//+/cd465f7624dfd687c7506028a5c0553aa3e4cd1b
Time: Thu May 05 18:25:27 2016
The CL last changed line 286 of file DeferredTaskHandler.cpp, which is stack frame 0.

sigbjornf@: Assigning to you, request you to please take a look into it. Please help us to reassign if not with respect to your change.

Thanks.!

Thanks, interesting that this new assert should fire.

AudioDeviceThread calling into Blink,

#0 0x7f1fa1fa0bb1 in blink::DeferredTaskHandler::setAudioThreadToCurrentThread() third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp:286:9
    #1 0x7f1fa1f0fde3 in blink::AudioDestinationHandler::render(blink::AudioBus*, blink::AudioBus*, unsigned long) third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:64:5
    #2 0x7f1fbd56fdaf in blink::AudioDestination::provideInput(blink::AudioBus*, unsigned long) third_party/WebKit/Source/platform/audio/AudioDestination.cpp:186:5
    #3 0x7f1fbd574997 in blink::AudioPullFIFO::fillBuffer(unsigned long) third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp:61:9
    #4 0x7f1fbd5747fc in blink::AudioPullFIFO::consume(blink::AudioBus*, unsigned long) third_party/WebKit/Source/platform/audio/AudioPullFIFO.cpp:48:9
    #5 0x7f1fbd56fb66 in blink::AudioDestination::render(blink::WebVector<float*> const&, blink::WebVector<float*> const&, unsigned long) third_party/WebKit/Source/platform/audio/AudioDestination.cpp:175:5
    #6 0x7f1fb1688abf in content::RendererWebAudioDeviceImpl::Render(media::AudioBus*, unsigned int, unsigned int) content/renderer/media/renderer_webaudiodevice_impl.cc:123:3
    #7 0x7f1fa5d436cd in media::AudioOutputDevice::AudioThreadCallback::Process(unsigned int) media/audio/audio_output_device.cc:448:3

I am not sure how this can be possible. Also I am curious if this happened at the first call, or sometime later.

The testcase appears to be a verbatim copy of https://github.com/WebKit/webkit/blob/master/LayoutTests/webaudio/audiocontext-state.html

The initial AudioDeviceThread is stopped following context.suspend(), leading to another being run later.. which triggers the assert.

(This bug isn't mine.)

One possible case would be when the context is shutting down. (m_audioThread is not available because the context is gone already.) We had similar issues, but couldn't find a perfect solution for it.

See #6 for when/why it happens.

Stack when the 2nd AudioDeviceThread is started,

#0  media::AudioDeviceThread::Start (this=0x614000053760, callback=0x60f0000b3d70, socket=104, thread_name=0x7fffe250be80 <.str.23> "AudioOutputDevice", synchronized_buffers=true)
    at ../../media/audio/audio_device_thread.cc:80
#1  0x00007fffe1133585 in media::AudioOutputDevice::OnStreamCreated (this=0x614000053640, handle=..., socket_handle=104, length=4112) at ../../media/audio/audio_output_device.cc:367
#2  0x00007fffe11339de in non-virtual thunk to media::AudioOutputDevice::OnStreamCreated(base::FileDescriptor, int, int) () at ../../media/audio/audio_output_device.cc:331
#3  0x00007fffeb04d823 in content::AudioMessageFilter::OnStreamCreated (this=0x61100007f340, stream_id=2, handle=..., socket_descriptor=..., length=4112)
    at ../../content/renderer/media/audio_message_filter.cc:222
#4  0x00007fffeb05f8a1 in base::DispatchToMethodImpl<content::AudioMessageFilter*, void (content::AudioMessageFilter::*)(int, base::FileDescriptor, base::FileDescriptor, unsigned int), int, base::FileDescriptor, base::FileDescriptor, unsigned int, 0ul, 1ul, 2ul, 3ul> (obj=@0x7fff946bb0c0: 0x61100007f340, method=
    (void (content::AudioMessageFilter::*)(content::AudioMessageFilter * const, int, base::FileDescriptor, base::FileDescriptor, unsigned int)) 0x7fffeb04d130 <content::AudioMessageFilter::OnStreamCreated(int, base::FileDescriptor, base::FileDescriptor, unsigned int)>, arg=...) at ../../base/tuple.h:166

Perhaps I misunderstood what exactly what stopping the audio device thread actually does.  It does seems as if the thread is stopped and starting the thread creates a new audio device thread.

I see two options here.  Remove the assert and set the thread to the actual currentThread (but I think that puts us back to where we were).  The second alternative is to plumb through the AudioOutputDevice::Play() and Pause() methods for us to use for resuming and suspending an online context.

+dalecurtis:  Pause() should pause the audio output device so that it no longer pulls on webaudio, right?  And Play() should do the opposite?  And in both cases the audio thread id isn't changed?

Pause/Play shouldn't change the thread, but Stop() followed by Start() will.

Pause() may continue pulling for a couple callbacks, only Stop() is guaranteed to immediately stop.

Is there a way to know when Pause() has actually paused?  We need to resolve the promise from context.suspend() when the the context has actually paused.

No, nothing like that exists.

Thanks Dale.

I think that only leaves us one option (at least for now).  Remove the assert and update m_audioThread.

sigbjornf:  WDYT?

It seems like a lot of problems occurring at this area and it is because 'a couple of callbacks' still pulling the graph through AudioDestinationNode::render().

As an interim solution, we can have an atomic flag like 'AudioContext::isCallbackValid' and check it at the beginning of the render call. When Pause() is invoked (or shutting down a context), we can change the state of this flag so we can ignore the rest of render calls.

Not an elegant solution, but this might solve the issue.

Currently Stop() stops, so there shouldn't be additional callbacks, AIUI.  The thread that is also running the audio graph should also be stopped when the device thread is stopped.  I think.

First time I've seen this code, so keep that in mind :), but 

 https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_output_device.cc&q=AudioOutputDevice::OnStreamCreated&sq=package:chromium&l=331

suggests #15 holds.

As to the assert at hand, having DeferredTaskHandler::clearHandlersToBeDeleted() reset m_audioThread is an option. The assert has proven a bit useful here; it might help explaining  issue 590108 .

Would having some way to check AudioRendererSink::CurrentlyOnAudioRenderingThread() be useful to you here? I'm suggesting such a change on this CL https://codereview.chromium.org/1703473002 for other reasons.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78df581f4e3be4387721661f592026bf92bcd58c

commit 78df581f4e3be4387721661f592026bf92bcd58c
Author: sigbjornf <sigbjornf@opera.com>
Date: Fri May 20 22:21:52 2016

Clear DeferredTaskHandler's audio thread ID upon the thread going away.

Tracking the thread ID of the (current) audio thread is needed to both
sanity check that code paths and methods are performed on the expected
thread, and ensure safe operation.

In the rare cases where the recorded audio thread is stopped and
terminated by the embedder, first clear out the associated thread
ID. This is needed should a new thread be subsequently created.

R=
BUG= 612127 

Review-Url: https://codereview.chromium.org/2001533002
Cr-Commit-Position: refs/heads/master@{#395182}

[modify] https://crrev.com/78df581f4e3be4387721661f592026bf92bcd58c/third_party/WebKit/Source/modules/webaudio/AbstractAudioContext.cpp
[modify] https://crrev.com/78df581f4e3be4387721661f592026bf92bcd58c/third_party/WebKit/Source/modules/webaudio/AudioContext.cpp
[modify] https://crrev.com/78df581f4e3be4387721661f592026bf92bcd58c/third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.cpp
[modify] https://crrev.com/78df581f4e3be4387721661f592026bf92bcd58c/third_party/WebKit/Source/modules/webaudio/DeferredTaskHandler.h

Removing EditIssue view restrictions from ClusterFuzz filed bugs. If you believe that this issue should still be restricted, please reapply the label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot