Closed Bug 1214967 Opened 6 years ago Closed 6 years ago

Fatal assert creating GMP decoder with e10s enabled

Categories

(Core :: Audio/Video: Playback, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
* 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)
* 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+
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)
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
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
https://hg.mozilla.org/mozilla-central/rev/81edda656162
https://hg.mozilla.org/mozilla-central/rev/285de7bf33e3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
You need to log in before you can comment on or make changes to this bug.