1352376 - Failed to create ContentDevice in content process after device reset

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P3)

Description

[Kevin Chen[:kechen] (UTC + 8)]

With GPU process enabled, the program cannot create ContentDevice in content process after device reset.
This is not an expected behavior since we've already successfully created devices in compositor side.

Comment 1

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Update device status

The thing is that when creating content device and get the DXGI adapter, we will compare the adapter we get from system and the adapter information from mDeviceStatus[1].

The information in mDeviceStatus should be updated in [2] which will sync the information with GPU process through chrome process; however, the mGPUReady flag blocks the update.

We should reset the flag when device reset happened.

[1] https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/gfx/thebes/DeviceManagerDx.cpp#228
[2] https://dxr.mozilla.org/mozilla-central/rev/8df9fabf2587b7020889755acb9e75b664fe13cf/gfx/thebes/gfxWindowsPlatform.cpp#445

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8853367 [details] [diff] [review]
Update device status

Okay, so if I understand correctly: the content process synchronously requests new device initialization data from the chrome process. The chrome process thinks the GPU is ready, so it doesn't bother blocking on a new device being available.

The callstack for RemoteCompositorSession::Reset looks like:
 RemoteCompositorSession::Reset
 nsBaseWidget::OnRenderingDeviceReset
 RemoteCompositorSession::NotifyDeviceReset (or nsWindow::OnPaint if in-process)
 GPUProcessManager::OnProcessDeviceReset(GPUProcessHost* aHost)
 GPUChild::RecvNotifyDeviceReset()
 [-- IPC --]
 GPUParent::NotifyDeviceReset()

Could we, instead, send the new device information in that NotifyDeviceReset IPC message? Then we don't need to change the GPUReady state or block on the GPU process again.

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1352376 - Update device data while processing device reset;

Review commit: https://reviewboard.mozilla.org/r/126702/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126702/

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8854731 [details]
Bug 1352376 - Update device data while processing device reset;

https://reviewboard.mozilla.org/r/126702/#review130026

Thanks! r=me with the aforementioned change removed or split out.

::: gfx/thebes/DeviceManagerDx.cpp:175
(Diff revision 1)
>  
>    // We should have been assigned a DeviceStatus from the parent process,
>    // GPU process, or the same process if using in-process compositing.
>    MOZ_ASSERT(mDeviceStatus);
>  
> -  if (CreateContentDevice() == FeatureStatus::CrashedInHandler) {
> +  if (CreateContentDevice() != FeatureStatus::Available) {

Is this change intended? If so, it should get a separate bug/patch with an explanation, since DisableD3D11AfterCrash() is intended to be called after SEH triggers.

Comment 5

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8854731 [details]
Bug 1352376 - Update device data while processing device reset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126702/diff/1-2/

Comment 6

[Kevin Chen[:kechen] (UTC + 8)]

(In reply to David Anderson [:dvander] from comment #4)
> Comment on attachment 8854731 [details]
> Bug 1352376 - Update device data while processing device reset.
> 
> https://reviewboard.mozilla.org/r/126702/#review130026
> 
> Thanks! r=me with the aforementioned change removed or split out.
> 
> ::: gfx/thebes/DeviceManagerDx.cpp:175
> (Diff revision 1)
> >  
> >    // We should have been assigned a DeviceStatus from the parent process,
> >    // GPU process, or the same process if using in-process compositing.
> >    MOZ_ASSERT(mDeviceStatus);
> >  
> > -  if (CreateContentDevice() == FeatureStatus::CrashedInHandler) {
> > +  if (CreateContentDevice() != FeatureStatus::Available) {
> 
> Is this change intended? If so, it should get a separate bug/patch with an
> explanation, since DisableD3D11AfterCrash() is intended to be called after
> SEH triggers.

I was intended to handle the creation failure of the content device, we only handle FeatureStatus::CrashedInHandler here[1].
But it might be appropriate to discuss this in other bug, so I remove this change.

[1] https://dxr.mozilla.org/mozilla-central/rev/fdd53f7d454d15a0b0d82d6bdae2c4fed837d048/gfx/thebes/DeviceManagerDx.cpp#175

Comment 7

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8854731 [details]
Bug 1352376 - Update device data while processing device reset;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126702/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b147ce9b3241
Update device data while processing device reset; r=dvander

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b147ce9b3241