876273 - WebAudio use-after-free crash [@mozilla::::MediaStreamGraphShutDownRunnable::Run]

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

This crash is very similar to bug 875221 but it happened while processing different Audio node objects - see frame #6 in the stack of alloc and free. The re-use condition differs here too.


alloc: ./content/media/webaudio/AudioDestinationNode.cpp:188

AudioDestinationNode::AudioDestinationNode(AudioContext* aContext,
                                           bool aIsOffline,
                                           uint32_t aNumberOfChannels,
                                           uint32_t aLength,
                                           float aSampleRate)
[...]
  MediaStreamGraph* graph = aIsOffline ?
*                           MediaStreamGraph::CreateNonRealtimeInstance() :
                            MediaStreamGraph::GetInstance();


free: ./content/media/MediaStreamGraph.cpp:1525

void
MediaStream::Destroy()
{
[...]
*  GraphImpl()->AppendMessage(new Message(this));
[...]
}


re-use: ./content/media/MediaStreamGraph.cpp:1199

class MediaStreamGraphShutDownRunnable : public nsRunnable {
public:
  MediaStreamGraphShutDownRunnable(MediaStreamGraphImpl* aGraph) : mGraph(aGraph) {}
  NS_IMETHOD Run()
  {
    NS_ASSERTION(mGraph->mDetectedNotRunning,
                 "We should know the graph thread control loop isn't running!");

*   mGraph->ShutdownThreads();
[...]


Tested with m-i changeset: 132982:ce25da24ba1c

Comment 1

[(no longer active)]

I have a dollar here which says this is a dupe of bug 875221.  :-)

Comment 2

[Christoph Diehl [:posidron]]

The fuzzer is still hitting this crash.

Comment 3

[(no longer active)]

I owe you a dollar then.  :-)

Comment 4

[(no longer active)]

So this crash is caused because we start to use the code in MediaStreamGraphShutDownRunnable to delete the graph, but then we end up in the case where the last media stream is deleted and so we delete the graph, and later on when we get to run MediaStreamGraphShutDownRunnable the graph is already deleted.

roc, what is the reliable condition to check for if I want to know which one of the shutdown paths will be taken by the graph?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think in MediaStreamGraphImpl::AppendMessage, the check for IsEmpty() should also check mLifecycleState >= LIFECYCLE_WAITING_FOR_STREAM_DESTRUCTION before deleting the graph.

Comment 7

[(no longer active)]

Attachment: Patch (v1)

You're right!

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08523c04d61a

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/08523c04d61a

Comment 10

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

No longer tracking for FF23