875221 - WebAudio use-after-free crash [@mozilla::MediaStreamGraphImpl::AppendMessage]

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[Christoph Diehl [:posidron]]

Attachment: callstack

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

  MediaStreamGraph* graph = aIsOffline ?
*                           MediaStreamGraph::CreateNonRealtimeInstance() :
                            MediaStreamGraph::GetInstance();


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

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


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

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


and then:


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

void
MediaStreamGraphImpl::AppendMessage(ControlMessage* aMessage)
{
[...]
* if (mDetectedNotRunning &&
      mLifecycleState > LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP) {
[...]



Tested with m-i changeset: 132700:cf2106c1f0c7 and the patches in bug 874915 and bug 874869.

Comment 1

[(no longer active)]

So the stack here tells me that we first call AppendMessage() which results in |delete this;|, then we call another AppendMessage() which tries to read mDetectedNotRunning, which is now garbage.

Now, for the first call to have deleted the graph, IsEmpty() must have returned true, which means that there should _not_ be an outstanding MediaStream associated with this graph.

So this got me curious to see if I can come up with cases where mStreams _could_ be empty, and it seems to me that accesses to mStreams on the main thread (the IsEmpty() call at least) is not synchronized in any way with the graph thread, so at least in theory it should be possible for the graph thread to call UpdateStreamOrder for example which temporarily empties out mStreams, and then for an IsEmpty() check on the main thread to succeed.

roc, do you think I'm barking up the wrong tree?

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

Ehsan thinks this bug affects everywhere we have WebRTC.

Comment 3

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

From AppendMessage:
  if (mDetectedNotRunning &&
      mLifecycleState > LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP) {
    // The graph control loop is not running and main thread cleanup has
    // happened.

So there is no need to synchronize with the graph thread, since it has already exited. At least that's the plan :-)

Comment 4

[(no longer active)]

Hmm, then I don't have any ideas, I'm afraid.

Comment 5

[Christoph Diehl [:posidron]]

Attachment: testcase

Testcase which will crash at the given location.

Comment 6

[Christoph Diehl [:posidron]]

I believe this is connected to bug 875402. It crashed for me with the stack attached here but sometimes also with the stack attached in bug 875402.

Comment 7

[(no longer active)]

Attachment: reduced testcase

(from attachment 753840 [details])

Comment 8

[(no longer active)]

Christoph, I can't reproduce this bug using the reduced testcase, and having the smallest possible testcase is going to help tremendously here.  Can you please give reducing this testcase another shot?  Thanks!

Comment 9

[Christoph Diehl [:posidron]]

The reduced testcase works but you need to hit refresh a bit more often.

Comment 10

[Christoph Diehl [:posidron]]

Attachment: testcase-highly-reduced

Do a soft reload followed by a hard reload - this does trigger it every time for me.

Tested with m-i changeset: 133065:1d6a04be8c0f

Comment 11

[(no longer active)]

OK, I finally have some idea what's happening here.

So we get a call to MediaStreamGraphImpl::AppendMessage, and we delete the graph object, but its thread is still running, which can result in any number of bad things to happen.

Now, the question is why.  I don't know that yet.

Comment 12

[(no longer active)]

I don't actually see a way to fix this problem.  When we currently delete this in AppendMessage, we need to make sure that the thread is correctly shut down.  We can't do that there though because nsThread::Shutdown spins the event loop.  We can't do it before since, well, that ship has sailed.  I'm not sure how MSG is supposed to be properly shut down usually, and why this is not a bug for realtime MSGs.  roc, do you know how this shutdown path is supposed to work?

Comment 13

[(no longer active)]

Attachment: Patch (v1)

I think I finally figured this out!  This is caused by an imbalance in mStreams, since we currently suppress the CreateMessage if run during shutdown.  The obvious solution is to not do that for non-realtime graphs at least.

Requesting feedback from cdiehl to give him a chance to test this with all of these test cases.  It does fix the crash with the non-reduced testcase, but I have not been able to reproduce with the other two.

Comment 14

[Christoph Diehl [:posidron]]

I can not reproduce it anymore :-)

Tested with m-i changeset: 133183:1c67a51e0fe5 and https://bugzilla.mozilla.org/attachment.cgi?id=755131

Comment 16

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> roc, do you know how this shutdown path is supposed to work?

It's documented in LifecycleState in MediaStreamGraphImpl.h. I know it's complicated, and this complexity keeps being a problem, but I did my best to explain how it works and right now I don't know of a simpler way to do it.

When mLifecycleState reaches LIFECYCLE_WAITING_FOR_THREAD_SHUTDOWN, the following must have already happened:
-- on the MSG thread, RunThread() set LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP as the last thing it did before releasing the lock on the MediaStreamGraph and exiting to the event loop.
-- then, in a stable state, the main thread took the MSG lock and transitioned mLifecycleState to LIFECYCLE_WAITING_FOR_THREAD_SHUTDOWN. Since the main thread got the lock, we know the RunThread() must have released the lock and either it has actually returned, or at least it's in a state where it won't access the MediaStreamGraph object again.

Let me know if that's still not clear.

Comment 17

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

Comment on attachment 755131 [details] [diff] [review]
Patch (v1)

Review of attachment 755131 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/MediaStreamGraph.cpp
@@ +1257,5 @@
> +      // is destroyed, and if that races with stream creation, we need
> +      // to be able to depend on knowing exactly when the last stream
> +      // is destroyed, hence we cannot bypass processing the create
> +      // message during "shutdown".
> +      Run();

I think this should be unconditional. There's no particular reason why the shutdown path for realtime and offline MSGs should differ, AFAIK.

Comment 18

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

Maybe what's unclear is that LIFECYCLE_WAITING_FOR_THREAD_SHUTDOWN means that RunThread won't touch the MSG again, so effectively the MSG thread is done. But we're still waiting for XPCOM thread shutdown of that thread, hence the name.

Comment 19

[(no longer active)]

Thanks, this makes more sense now.

Comment 20

[(no longer active)]

Attachment: Patch (v2)

Comment 21

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

Add a comment in RunDuringShutdown that we need to add the stream to balance the pending Destroy call.

Comment 22

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/318254a9a66b

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/318254a9a66b

Comment 24

[(no longer active)]

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