1303419 - Crash in mozilla::MediaEngineWebRTCMicrophoneSource::FreeChannel

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-423577b1-78c1-4e8f-83d5-4f2ae2160916.
=============================================================
Crashing Thread (48)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::MediaEngineWebRTCMicrophoneSource::FreeChannel() 	dom/media/webrtc/MediaEngineWebRTCAudio.cpp:700
1 	xul.dll 	mozilla::MediaEngineWebRTCMicrophoneSource::Shutdown() 	dom/media/webrtc/MediaEngineWebRTCAudio.cpp:738
2 	xul.dll 	mozilla::MediaEngineWebRTC::Shutdown() 	dom/media/webrtc/MediaEngineWebRTC.cpp:439
3 	xul.dll 	`mozilla::MediaManager::Shutdown'::`2'::ShutdownTask::Run 	dom/media/MediaManager.cpp:2644
4 	xul.dll 	MessageLoop::RunTask(already_AddRefed<mozilla::Runnable>) 	ipc/chromium/src/base/message_loop.cc:349
5 	xul.dll 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask&&) 	ipc/chromium/src/base/message_loop.cc:357
6 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc:432
7 	xul.dll 	mozilla::ipc::MessagePumpForNonMainUIThreads::DoRunLoop() 	ipc/glue/MessagePump.cpp:423
8 	xul.dll 	base::MessagePumpWin::RunWithDispatcher(base::MessagePump::Delegate*, base::MessagePumpWin::Dispatcher*) 	ipc/chromium/src/base/message_pump_win.cc:56

this is a crash signature regressing in firefox 49 builds and happening in a codepath introduced in bug 1273206.
its crash volume is mid- to low-level making up ~0.1% of 49beta crashes during the last week, until now only on windows devices.

Comment 1

[Nils Ohlmeier [:drno]]

Looks like we are missing a safety if() around the DeleteChannel() here as well:
https://dxr.mozilla.org/mozilla-central/rev/edfff7a9e4c43f6d516dfbf69a64e205e5cdb699/dom/media/webrtc/MediaEngineWebRTCAudio.cpp#777

Although that leaves the question: do we need to do anything in case |mVoENetwork| and |mVoEBase| disappeared already?

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: Bug 1303419: check if mVoEBase is available.

Review commit: https://reviewboard.mozilla.org/r/79314/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79314/

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Nils Ohlmeier [:drno] from comment #1)
> Looks like we are missing a safety if() around the DeleteChannel() here as
> well:
> https://dxr.mozilla.org/mozilla-central/rev/
> edfff7a9e4c43f6d516dfbf69a64e205e5cdb699/dom/media/webrtc/
> MediaEngineWebRTCAudio.cpp#777
> 
> Although that leaves the question: do we need to do anything in case
> |mVoENetwork| and |mVoEBase| disappeared already?

Even the if (mVoENetwork) is overkill here; those are set to null by DeInitEngine(), which is always called after FreeChannel().  However, if there is a hole, it's that Shutdown() (unlike the other uses of FreeChannel()/DeInitEgine()) doesn't do "if (--sChannelsOpen == 0) { DeInitEngine(); }" -- so the first channel to shut down clears the engine pointers, and the second finds them NULL.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Audio gUM allocate/free improvements and nullptr crash fix

Comment 5

[Nils Ohlmeier [:drno]]

Comment on attachment 8792145 [details] [diff] [review]
Audio gUM allocate/free improvements and nullptr crash fix

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

LGTM. 
And yes this should fix the problem you described in the comment.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +707,5 @@
>    if (mChannel != -1) {
>      if (mVoENetwork) {
>        mVoENetwork->DeRegisterExternalTransport(mChannel);
>      }
>      mVoEBase->DeleteChannel(mChannel);

I would still prefer to make this consistent. Something like this:

  MOZ_ASSERT(mVoENetwork);
  if (mVoENetwork) {
      mVoENetwork->DeRegisterExternalTransport(mChannel);
  }
  MOZ_ASSERT(mVoEBase);
  if (mVoEBase) {
    mVoEBase->DeleteChannel(mChannel);
  }

That should be the super safe alternative. Or just remove the 'if (mVoENetwork)' check to make consistent, but less safe.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4624432ec08e
Audio gUM allocate/free improvements and nullptr crash fix r=drno

Comment 7

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/321c87566020
Backed out changeset 4624432ec08e

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbdc035f911f
Audio gUM allocate/free improvements and nullptr crash fix r=drno

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/fbdc035f911f

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jesup, should we consider uplifting this fix to Beta50?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8792145 [details] [diff] [review]
Audio gUM allocate/free improvements and nullptr crash fix

Approval Request Comment
[Feature/regressing bug #]: bug 1273206

[User impact if declined]: crashes in the field

[Describe test coverage new/current, TreeHerder]: not well covered as this requires "real" audio devices during testing.  We have some coverage with "real" devices on linux only, but it doesn't hit this particular corner case (which requires multiple real devices).

[Risks and why]: Low risk; and I can make a much simpler version of the patch that just adds the appropriate cleanup to the one path (::Shutdown) that didn't have it.  (The landed patch also cleans up the code considerably to avoid problems in the future.)

The patch will just be to replace DeInitEngine() in ::Shutdown with:
+    MOZ_ASSERT(sChannelsOpen > 0);
+    if (--sChannelsOpen == 0) {
+      DeInitEngine();
+    }

[String/UUID change made/needed]: none

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792145 [details] [diff] [review]
Audio gUM allocate/free improvements and nullptr crash fix

Crash fix, Beta50+.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Jesup, could you please land the smaller patch with only the change in shutdown as you suggested once moz-beta tree opens up? Thanks!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bc29a87b7aaf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for MediaEngineWebRTCAudio.cpp assertions.
https://treeherder.mozilla.org/logviewer.html#?job_id=1627789&repo=mozilla-beta
https://treeherder.mozilla.org/logviewer.html#?job_id=1627909&repo=mozilla-beta

https://hg.mozilla.org/releases/mozilla-beta/rev/91faf7ec36cd

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Fix nullptr crash in audio gUM (beta patch)

an extra if is needed in case the source is already released, since we're not doing this inside FreeChannel() (like we're doing in Nightly/Aurora).

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a734cc547c2d

Comment 18

[Nils Ohlmeier [:drno]]

Comment on attachment 8793158 [details] [diff] [review]
Fix nullptr crash in audio gUM (beta patch)

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

I'm not sure this is really what you want.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
@@ +819,5 @@
>      Deallocate(nullptr); // XXX Extend concurrent constraints code to mics.
>    }
>  
> +  if (mState != kReleased) {
> +    FreeChannel();

As FreeChannel() set mState to kReleased you are now only ever release the first of multiple channel. Is that really what you want here?

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Nils Ohlmeier [:drno] from comment #18)
> Comment on attachment 8793158 [details] [diff] [review]
> Fix nullptr crash in audio gUM (beta patch)
> 
> Review of attachment 8793158 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not sure this is really what you want.
> 
> ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp
> @@ +819,5 @@
> >      Deallocate(nullptr); // XXX Extend concurrent constraints code to mics.
> >    }
> >  
> > +  if (mState != kReleased) {
> > +    FreeChannel();
> 
> As FreeChannel() set mState to kReleased you are now only ever release the
> first of multiple channel. Is that really what you want here?

mState and mChannel are part of the same object; if mState is kReleased there is no channel to release (and the engine isn't inited).  This is the simple version of the larger patch, which moved the test of the static into FreeChannel(), within the same if() there.  Since it's now outside FreeChannel, I need to wrap them in an if().

Comment 20

[Nils Ohlmeier [:drno]]

Comment on attachment 8793158 [details] [diff] [review]
Fix nullptr crash in audio gUM (beta patch)

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

Looks I should not do code reviews in the middle of the night.

LGTM.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-beta/rev/eb11246a0e01