If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Fatal assert creating GMP decoder with e10s enabled

RESOLVED FIXED in Firefox 45

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(e10s+, firefox45 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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++
Maire, can you have someone look into this?
Flags: needinfo?(mreavy)
This is a <video> playback issue, not a WebRTC related one. I'll take it.
Flags: needinfo?(mreavy)
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.
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.
Attachment #8674653 - Flags: review?(jwwang)
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.
Attachment #8674655 - Flags: review?(wmccloskey)
Attachment #8674655 - Flags: review?(jwwang)
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.
Attachment #8674655 - Flags: review?(jwwang) → review+
(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.
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.
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.
Attachment #8674653 - Flags: review?(jwwang) → review+
Created attachment 8674670 [details] [diff] [review]
Patch 1v2: Add lookup table of GMP codecs

With StaticMutex.
Attachment #8674653 - Attachment is obsolete: true
Attachment #8674670 - Flags: review?(jwwang)
Attachment #8674670 - Flags: review?(jwwang) → review+
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().
Attachment #8674655 - Flags: review?(wmccloskey) → review+
(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?
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)

Comment 14

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/af6b752acbd0
https://hg.mozilla.org/integration/mozilla-inbound/rev/120b53ea0f41
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
Priority: -- → P1
tracking-e10s: --- → +
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

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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81edda656162
https://hg.mozilla.org/mozilla-central/rev/285de7bf33e3
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/81edda656162
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/285de7bf33e3
status-b2g-v2.5: --- → fixed
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.