614160 - Assert and crash on shutdown with IPC when remote audio active

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

While trying to debug bug 614136, I've also discovered the following crash (which I can reproduce readily, unlike the other bug):

1. Open video file and wait for it to start playing.
2. Close Fennec.
3. Parent and child assert:
###!!! ABORT: not on worker thread!: 'mWorkerLoop == MessageLoop::current()', file ../../dist/include/mozilla/ipc/AsyncChannel.h, line 138
###!!! ABORT: corrupted actor state: file ../../ipc/ipdl/_ipdlheaders/mozilla/dom/PAudio.h, line 67

nsBuiltinDecoder::Shutdown is called from nsBuiltinDecoder::Observe when receiving an xpcom-shutdown event.  Shutdown() performs partial destruction, then queues an runnable to call nsBuiltinDecoder::Stop.

The runnable calling nsBuiltinDecoder::Stop eventually runs, dropping the only reference to nsBuiltinDecoderStateMachine, which holds the only reference to nsAudioStream.  nsAudioStream's dtor is called, which calls nsAudioStream::Shutdown.

In the IPC world, this calls PAudioChild::Send__delete on the child, which aborts with a fatal assert because mWorkerLoop is NULL.  We're already deep into shutdown here (mozilla::ShutdownXPCOM is on the stack).

The parent is dying for a similar reason: a sample offset timer callback is running when we're deep into shutdown (mozilla::ShutdownXPCOM is on the stack), and aborts for the same reason as the child.

Comment 1

[Josh Matthews [:jdm]]

You'll be wanting an ActorDestroy in the child, and to encapsulate the Send__delete__ call within a method that can check if the protocol is actually still alive.

Comment 2

[Josh Matthews [:jdm]]

Rather, you'll want an ActorDestroy method on both sides, and presumably guard every Send call with an if (mIPCOpen) check.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Make AudioChild refcounted so it's safe for nsAudioStreamRemote and content protocol manager to hold references to it.  Add ActorDestroy calls to update mIPCOpen flag, and use that to avoid sending IPCs when actor is being destroyed.

Thanks for the tip Josh!

Comment 4

[Doug Turner (:dougt)]

Comment on attachment 492886 [details] [diff] [review]
patch v0

Nice.

Maybe the events should hold the ref counted child instead of the nsAudioStreamRemote.  This would allow one less if stm when it runs.

Can shutdown happen on a non-main thread?  If so in a follow up we should forward that delete.  I thing, incorrectly, I just removed it.

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Address Doug's comments.  Changes the AudioInitEvent to be SYNC, otherwise it's possible to drop post-Init calls due to mAudioChild being NULL.  This also fixes bug 614136 because it was impossible to test this patch without fixing that bug.

Comment 6

[Doug Turner (:dougt)]

push to mc - http://hg.mozilla.org/mozilla-central/rev/24aed997dbcd

Comment 7

[Doug Turner (:dougt)]

windows hates you.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290921634.1290922466.32178.gz



AudioChild.cpp

e:/builds/moz2_slave/mozilla-central-win32/build/dom/ipc/AudioChild.cpp(45) : error C2373: 'mozilla::dom::AudioChild::AddRef' : redefinition; different type modifiers

        e:\builds\moz2_slave\mozilla-central-win32\build\obj-firefox\dist\include\mozilla/dom/AudioChild.h(51) : see declaration of 'mozilla::dom::AudioChild::AddRef'

e:/builds/moz2_slave/mozilla-central-win32/build/dom/ipc/AudioChild.cpp(46) : error C2373: 'mozilla::dom::AudioChild::Release' : redefinition; different type modifiers

        e:\builds\moz2_slave\mozilla-central-win32\build\obj-firefox\dist\include\mozilla/dom/AudioChild.h(52) : see declaration of 'mozilla::dom::AudioChild::Release'

Comment 8

[Matthew Gregan [:kinetik]]

Attachment: patch v1.1

Just adds NS_IMETHOD goop to the AddRef/Release decls.  Test built on Win32.

Comment 9

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/c33459065387