Closed Bug 1331761 Opened 6 years ago Closed 6 years ago

Crash when killing the GPU process during video decoding


(Core :: Graphics: Layers, defect, P1)




Tracking Status
firefox53 --- verified
firefox54 --- verified


(Reporter: dvander, Assigned: dvander)



(Whiteboard: [gfx-noted])


(1 file)

+++ This bug was initially created as a clone of Bug #1331548 +++

1. Open in a window.
2. Open about:support in a second window.
3. While the video is playing in 4k, click the "Terminate" button for the GPU process in about:support.

It looks like we are trying to create a GPUVideoTextureHost in the new in-process compositor, which attempts to access the non-existent VideoBridgeParent singleton.
Does this happen only when GPU process video decoding is enabled, or regardless of that setting?
(In reply to Milan Sreckovic [:milan] from comment #1)
> Does this happen only when GPU process video decoding is enabled, or
> regardless of that setting?

Never mind, answered in bug 1331548.
Assignee: nobody → dvander
Matt suggested a fix that seems very reasonable to me - protect ImageClient from sending stale TextureClients to the compositor. But I can't reproduce this anymore and have been trying on both debug/opt on multiple machines. I also don't see any crashes involving VideoBridgeParent on crash-stats.

I'm loathe to attempt a fix without any way to see that it works, so I'm holding off for now.
Perhaps we can have a MOZ_ASSERT added in the cases that this would fix, and see if anybody on nightly hits it?  Then replace the assert with the fix.
I spoke too soon... this morning I can reproduce it about 30% of the time.
Attached patch fixSplinter Review
Am able to verify now that this does fix the issue.
Attachment #8832618 - Flags: review?(matt.woodrow)
Comment on attachment 8832618 [details] [diff] [review]

Review of attachment 8832618 [details] [diff] [review]:

::: gfx/layers/client/ImageClient.cpp
@@ +229,5 @@
> +    // We check if the texture's allocator is still open, since in between media
> +    // decoding a frame and adding it to the compositable, we could have
> +    // restarted the GPU process.
> +    if (!texture || !texture->GetAllocator()->IPCOpen()) {
> +      return false;

Can we continue and see if any of the other textures in the list work?
Attachment #8832618 - Flags: review?(matt.woodrow) → review+
Pushed by
Don't send stale GPUVideoTextures to in-process compositors. (bug 1331761, r=mattwoodrow)
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8832618 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: GPU Process
[User impact if declined]: potential browser crashes if the GPU process dies
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This is a simple check that we are not using a stale IPC channel to send video. It's baked on aurora and nightly.
[String changes made/needed]:
Attachment #8832618 - Flags: approval-mozilla-beta?
Comment on attachment 8832618 [details] [diff] [review]

Fix for a graphics crash, let's take it on beta.
Attachment #8832618 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Duplicate of this bug: 1343818
I reproduced this issue using Fx 53.0b8 on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 54.0a2 (build ID: 20170403004002) and Fx 53.0b9 on Windows 10 x64.

You need to log in before you can comment on or make changes to this bug.