Fatal assert creating GMP decoder with e10s enabled

RESOLVED FIXED in Firefox 45

Status

()

P1
normal
RESOLVED FIXED
3 years ago
3 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)
(Assignee)

Comment 2

3 years ago
This is a <video> playback issue, not a WebRTC related one. I'll take it.
Flags: needinfo?(mreavy)
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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)
(Assignee)

Comment 5

3 years ago
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+
(Assignee)

Comment 7

3 years ago
(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+
(Assignee)

Comment 10

3 years ago
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+
(Assignee)

Comment 12

3 years ago
(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
tracking-e10s: --- → +
(Assignee)

Comment 16

3 years ago
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 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81edda656162
https://hg.mozilla.org/mozilla-central/rev/285de7bf33e3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
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
status-b2g-v2.5: fixed → ---
You need to log in before you can comment on or make changes to this bug.