Matt Woodrow (:mattwoodrow) Reporter
	Description • 8 years ago

This is the first step of bug 1300675, just avoiding crashing the content process if the GPU process crashes.

In the future we want to try figure out a way to actually resume decoding video (from the next keyframe?) rather than returning an error and stopping playback.

	Matt Woodrow (:mattwoodrow) Reporter
	Comment 1 • 8 years ago

This stops the content process waiting for decoded frames and puts the video into an error state instead. Not ideal, but better than crashing!

	Matt Woodrow (:mattwoodrow) Reporter
	Comment 3 • 8 years ago

We need to be able to handle crashes that happen during painting, but if it's already broken then we might as well bail early.

	Nicolas Silva [:nical]
	Comment 4 • 8 years ago

Comment on attachment 8794732 [details] [diff] [review]
Rebuild the ImageContainers IPDL actors after a crash

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

::: gfx/layers/ImageContainer.cpp
@@ +106,5 @@
>  
> +void
> +ImageContainer::EnsureImageClient(bool aCreate)
> +{
> +  if (!aCreate && (!mImageClient || mImageClient->GetForwarder()->IPCOpen())) {

Why do we need aCreate? In the constructor mImageClient is null so it should never take this branch, and in the other call-site, aCreate is false so it doesn't seem to affect this branch.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 5 • 8 years ago

Comment on attachment 8794732 [details] [diff] [review]
Rebuild the ImageContainers IPDL actors after a crash

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

r=me pending comments

::: gfx/layers/ImageContainer.cpp
@@ +106,5 @@
>  
> +void
> +ImageContainer::EnsureImageClient(bool aCreate)
> +{
> +  if (!aCreate && (!mImageClient || mImageClient->GetForwarder()->IPCOpen())) {

Should this be |mImageClient && mImageClient->GetForwarder()->IPCOpen()|?

@@ +131,5 @@
>  {
> +  if (flag == ASYNCHRONOUS) {
> +    EnsureImageClient(true);
> +  } else {
> +    mAsyncContainerID = sInvalidAsyncContainerId;

Looks better to just assign this in the constructor list.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 6 • 8 years ago

Comment on attachment 8794733 [details] [diff] [review]
Don't paint LayerManager that are disconnected

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

this looks okay, but I think we should do it in a separate bug *after* we've fixed the sites that crash as a result of not doing this. The channel can go away during a sync message, so unless we're sure we don't have any of those during painting, we should handle when they fail.

::: layout/base/nsPresShell.cpp
@@ +6267,5 @@
>    bool shouldInvalidate = layerManager->NeedsWidgetInvalidation();
>  
> +
> +  if (layerManager->AsShadowForwarder() &&
> +    !layerManager->AsShadowForwarder()->IPCOpen()) {

This probably fits on one line, but if not, align the ! to after ( above.

	Matt Woodrow (:mattwoodrow) Reporter
	Comment 7 • 8 years ago

We're currently using the default impl (return true), which isn't much help.

	Pulsebot
	Comment 8 • 8 years ago

Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f53bc40fa5f9
Implement IPCOpen for ImageBridgeChild. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b046a7b934
Notify media if the GPU process crashes during video playback. r=dvander
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0755dff5ddc
Rebuild IPDL actors on ImageContainer once we've recovered from a GPU process crash. r=dvander

	Nicolas Silva [:nical]
	Comment 9 • 8 years ago

(In reply to Nicolas Silva [:nical] from comment #4)
> Comment on attachment 8794732 [details] [diff] [review]
> Rebuild the ImageContainers IPDL actors after a crash
> 
> Review of attachment 8794732 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageContainer.cpp
> @@ +106,5 @@
> >  
> > +void
> > +ImageContainer::EnsureImageClient(bool aCreate)
> > +{
> > +  if (!aCreate && (!mImageClient || mImageClient->GetForwarder()->IPCOpen())) {
> 
> Why do we need aCreate? In the constructor mImageClient is null so it should
> never take this branch, and in the other call-site, aCreate is false so it
> doesn't seem to affect this branch.

Matt, did you see my question on the remaining patch? The patch looks good, I just want to understand the part about aCreate before giving the r+.

	Matt Woodrow (:mattwoodrow) Reporter
	Comment 10 • 8 years ago

(In reply to Nicolas Silva [:nical] from comment #9) 
> > > +void
> > > +ImageContainer::EnsureImageClient(bool aCreate)
> > > +{
> > > +  if (!aCreate && (!mImageClient || mImageClient->GetForwarder()->IPCOpen())) {
> > 
> > Why do we need aCreate? In the constructor mImageClient is null so it should
> > never take this branch, and in the other call-site, aCreate is false so it
> > doesn't seem to affect this branch.
> 
> Matt, did you see my question on the remaining patch? The patch looks good,
> I just want to understand the part about aCreate before giving the r+.

Oops, no, I totally missed this!

In the constructor, aCreate gets passed as true, so we don't take the early return and we always create an image client.

In the other call-site, aCreate is false, so we only create a new image client if there was an existing one and it's no longer valid.

	Phil Ringnalda (:philor)
	Comment 11 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f53bc40fa5f9
https://hg.mozilla.org/mozilla-central/rev/a5b046a7b934
https://hg.mozilla.org/mozilla-central/rev/d0755dff5ddc

	Nicolas Silva [:nical]
	Comment 12 • 8 years ago

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> In the constructor, aCreate gets passed as true, so we don't take the early
> return and we always create an image client.
> 
> In the other call-site, aCreate is false, so we only create a new image
> client if there was an existing one and it's no longer valid.

Can this be simplified into

> if (mImageClient && mImageClient->GetForwarder()->IPCOpen())

as David suggested? Or do we specifically want to not create an ImageClient after the constructor if mImageClient is null? If so, please document it because the next person who will touch this code will be itching to simplify the condition.

	Matt Woodrow (:mattwoodrow) Reporter
	Comment 13 • 8 years ago

(In reply to Nicolas Silva [:nical] from comment #12)
> 
> Can this be simplified into
> 
> > if (mImageClient && mImageClient->GetForwarder()->IPCOpen())
> 
> as David suggested? Or do we specifically want to not create an ImageClient
> after the constructor if mImageClient is null? If so, please document it
> because the next person who will touch this code will be itching to simplify
> the condition.

No, because if mImageClient is null, then we're not an async ImageContainer and we shouldn't be creating a new one. We only want to create one if we're forcibly doing sure during the ctor, or if we have an existing one, but it's been disconnected.

I landed the patch with a comment explaining this a bit better.