Failed to create ContentDevice in content process after device reset

RESOLVED FIXED in Firefox 55

Status

()

P3
normal
RESOLVED FIXED
2 years ago
4 months ago

People

(Reporter: kechen, Assigned: kechen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8853367 [details] [diff] [review]
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
Attachment #8853367 - Flags: feedback?(dvander)
(Assignee)

Updated

2 years ago
Assignee: nobody → kechen
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.
Attachment #8853367 - Flags: feedback?(dvander)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8853367 - Attachment is obsolete: true
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.
Attachment #8854731 - Flags: review?(dvander) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

2 years ago
(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 hidden (mozreview-request)

Comment 8

2 years ago
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b147ce9b3241
Update device data while processing device reset; r=dvander
(Assignee)

Updated

2 years ago
Blocks: 1160157

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b147ce9b3241
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Updated

4 months ago
Blocks: 1188006
You need to log in before you can comment on or make changes to this bug.