1214967 - Fatal assert creating GMP decoder with e10s enabled

Status: RESOLVED FIXED

Description

[Chris Pearce [:cpearce (GMT+13)]]

On pearce.org.nz/h, with e10s enabled.

 	xul.dll!mozilla::gmp::GeckoMediaPluginServiceChild::GetPluginVersionForAPI(const nsACString_internal & aAPI, nsTArray<nsCString> * aTags, bool * aHasPlugin, nsACString_internal & aOutVersion) Line 127	C++
 	xul.dll!mozilla::gmp::GeckoMediaPluginService::HasPluginForAPI(const nsACString_internal & aAPI, nsTArray<nsCString> * aTags, bool * aOutHavePlugin) Line 564	C++
 	xul.dll!mozilla::GMPDecoderModule::SupportsMimeType(const nsACString_internal & aMimeType, const mozilla::Maybe<nsCString> & aGMP) Line 154	C++
>	xul.dll!mozilla::GMPDecoderModule::SupportsMimeType(const nsACString_internal & aMimeType) Line 163	C++
 	xul.dll!mozilla::PDMFactory::GetDecoder(const nsACString_internal & aMimeType) Line 275	C++
 	xul.dll!mozilla::PDMFactory::CreateDecoder(const mozilla::TrackInfo & aConfig, mozilla::FlushableTaskQueue * aTaskQueue, mozilla::MediaDataDecoderCallback * aCallback, mozilla::layers::LayersBackend aLayersBackend, mozilla::layers::ImageContainer * aImageContainer) Line 138	C++
 	xul.dll!mozilla::MediaFormatReader::EnsureDecodersCreated() Line 391	C++
 	xul.dll!mozilla::MediaFormatReader::OnDemuxerInitDone(nsresult __formal) Line 349	C++
 	xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::InvokeCallbackMethod<mozilla::MediaFormatReader,void (__thiscall mozilla::MediaFormatReader::*)(enum nsresult),enum nsresult const &>(mozilla::MediaFormatReader * aThisVal, void (nsresult) * aMethod, const nsresult & aValue) Line 437	C++
 	xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::MethodThenValue<mozilla::MediaFormatReader,void (__thiscall mozilla::MediaFormatReader::*)(enum nsresult),void (__thiscall mozilla::MediaFormatReader::*)(enum mozilla::DemuxerFailureReason)>::DoResolveOrRejectInternal(const mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ResolveOrRejectValue & aValue) Line 486	C++
 	xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ThenValueBase::DoResolveOrReject(const mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ResolveOrRejectValue & aValue) Line 386	C++
 	xul.dll!mozilla::MozPromise<enum nsresult,enum mozilla::DemuxerFailureReason,1>::ThenValueBase::ResolveOrRejectRunnable::Run() Line 320	C++
 	xul.dll!mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run() Line 183	C++
 	xul.dll!mozilla::TaskQueue::Runner::Run() Line 171	C++
 	xul.dll!nsThreadPool::Run() Line 230	C++

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Maire, can you have someone look into this?

Comment 2

[Chris Pearce [:cpearce (GMT+13)]]

This is a <video> playback issue, not a WebRTC related one. I'll take it.

Comment 3

[Chris Pearce [:cpearce (GMT+13)]]

The problem here is that we're calling the main thread only GeckoMediaPluginServiceChild::GetPluginVersionForAPI() to ask the chrome process if a GMP is supported from the decode thread. Presumably that's main thread only because it uses PContent to talk to the chrome process.

A simple fix is to build a lookup table of the GMPs that we'll use for decoding in GMPDecoderModule::Init(), which runs on the main thread, and whenever that changes, update the table.

Comment 4

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8674653 [details] [diff] [review]
Patch 1: Add lookup table of GMP codecs

* Add a table of the GMP codecs we expect to use. This is on the main thread, so GMPService::HasPluginForAPI() works.
* Implement GMPDecoderModule::SupportsMimeType() by consulting the the table.

Updating the table to come in the next patch.

Comment 5

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8674655 [details] [diff] [review]
Patch 2: Update GMP PDM's codec table on GMP enable/disable

* Dispatch "gmp-changed" observer service notification whenever GMPs added or removed. This corresponds to the GMPProvider at startup, or if the user enables/disables a GMP via the about:addons>plugins page.
* Proxy the "gmp-changed" observer service notification over from chrome to content process via PContent.

r? jwwang for GMPServiceParent.cpp change.
r? billm for PContent change.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8674653 [details] [diff] [review]
Patch 1: Add lookup table of GMP codecs

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

::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
@@ +207,5 @@
> +  const bool isAAC = aMimeType.EqualsLiteral("audio/mp4a-latm");
> +  const bool isH264 = aMimeType.EqualsLiteral("video/avc") ||
> +                      aMimeType.EqualsLiteral("video/mp4");
> +
> +  for (GMPCodecs& gmp : sGMPCodecs) {

Is there any synchronization to ensure changes of sGMPCodecs is visible to other threads and no race in reading sGMPCodecs here while it is being updated on the main thread?

@@ +220,2 @@
>    }
> + 

space.

Comment 7

[Chris Pearce [:cpearce (GMT+13)]]

(In reply to JW Wang [:jwwang] from comment #6)
> Comment on attachment 8674653 [details] [diff] [review]
> Patch 1: Add lookup table of GMP codecs
> 
> Review of attachment 8674653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp
> @@ +207,5 @@
> > +  const bool isAAC = aMimeType.EqualsLiteral("audio/mp4a-latm");
> > +  const bool isH264 = aMimeType.EqualsLiteral("video/avc") ||
> > +                      aMimeType.EqualsLiteral("video/mp4");
> > +
> > +  for (GMPCodecs& gmp : sGMPCodecs) {
> 
> Is there any synchronization to ensure changes of sGMPCodecs is visible to
> other threads and no race in reading sGMPCodecs here while it is being
> updated on the main thread?

No. I don't think it's necessary since GMPs won't be aggressively switching off and on again... I can change GMPCodec::mHas* into atomics if you like, but it seems like overkill.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

For correctness, the whole table must be updated in an indivisible transaction so GMPDecoderModule::SupportsMimeType() will never see the table in an inconsistent state. Failing to do so might lead to subtle bugs which rarely happen.

So atomics doesn't fix that problem. You will need a mutex for that.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Come to think about it, if a user asks supported codec while GMP add/remove is in action, this is inherently a race and the result is indeterministic. So this should be probably fine.

Comment 10

[Chris Pearce [:cpearce (GMT+13)]]

Created attachment 8674670 [details] [diff] [review]
Patch 1v2: Add lookup table of GMP codecs

With StaticMutex.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8674655 [details] [diff] [review]
Patch 2: Update GMP PDM's codec table on GMP enable/disable

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

::: dom/ipc/ContentParent.cpp
@@ +3295,5 @@
>              }
>          }
>      }
> +    else if (!strcmp(aTopic, "gmp-changed")) {
> +      unused << SendNotifyGMPsChanged();

We shouldn't do this if IsDestroyed().

Comment 12

[Chris Pearce [:cpearce (GMT+13)]]

(In reply to Bill McCloskey (:billm) from comment #11)
> Comment on attachment 8674655 [details] [diff] [review]
> Patch 2: Update GMP PDM's codec table on GMP enable/disable
> 
> Review of attachment 8674655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentParent.cpp
> @@ +3295,5 @@
> >              }
> >          }
> >      }
> > +    else if (!strcmp(aTopic, "gmp-changed")) {
> > +      unused << SendNotifyGMPsChanged();
> 
> We shouldn't do this if IsDestroyed().

Thanks. I see other messages being sent without checking IsDestroyed(), should we file a follow-up to add IsDestroyed() checks to them too?

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I looked at this a bit more and I guess the check isn't needed in this particular case. For non-toplevel actors, it's possible that the MessageChannel will have been destroyed and we'll segfault if we Send after ActorDestroy. But a toplevel actor (like ContentParent) owns its channel, so that won't happen.

In addition, Send will call ProcessingError(MessageDropped) if the connection is closed. But ContentParent ignores that error:
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1940

It's still good practice to do the check, but I guess there's no harm in missing it on ContentParent.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/af6b752acbd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/120b53ea0f41

Comment 15

[Phil Ringnalda (:philor)]

The only thing more unwelcome than being backed out for an incomprehensible and seemingly-unrelated b2g failure like https://treeherder.mozilla.org/logviewer.html#?job_id=15796569&repo=mozilla-inbound is being backed out for etc. in a suite which takes two bloody hours to run.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4eaf357c0a

Comment 16

[Chris Pearce [:cpearce (GMT+13)]]

I've wasted too much time trying to debug this on try and I can't get B2G emulator to build locally. So I'll just disable the "gmp-changed" observer service notification on B2G, and get this landed and move on. We're not interested in using GMPs for non-EME decoding on B2G anyway, and not expecting GMPs to change at runtime on B2G at this stage.

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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81edda656162
https://hg.mozilla.org/integration/mozilla-inbound/rev/285de7bf33e3

Comment 18

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/81edda656162
https://hg.mozilla.org/mozilla-central/rev/285de7bf33e3

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/81edda656162
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/285de7bf33e3

Comment 20

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion