1259810 - D2D1 device creation may fail without properly falling over to software backend and causes downstream crashes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Lee Salzman [:lsalzman]]

As seen in bug 1229988 and bug 1189715, we're seeing the symptom, in many crash reports from 46 beta, that gfxWindowsPlatform::UpdateRenderMode fails to recreate the screen reference draw target after a device reset.

Looking at various bug reports, it seems that when Factory::SetDirect3D11Device fails to create the D2D1 device, the reason for failure is DXGI_ERROR_DEVICE_REMOVED in almost all cases.

In any event, if this situation occurs, we still end up reporting the D2D1 feature status as available, so we never fall over to the software backend, and so the originally mentioned problem with UpdateRenderMode occurs.

Comment 1

[Lee Salzman [:lsalzman]]

Attachment: check that D2D1 device creation succeeds and otherwise fall to software backend

This makes Factory::SetDirect3D11Device fallible, so that gfxWindowsPlatform::InitializeD2D can update the feature status appropriately. gfxWindowsPlatform::UpdateBackendPrefs downwind will take care of the rest for us now that it knows it can't use D2D.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734870 [details] [diff] [review]
check that D2D1 device creation succeeds and otherwise fall to software backend

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

::: gfx/2d/2D.h
@@ +1376,5 @@
> +  /*
> +   * Attempts to create and install a D2D1 device from the supplied Direct3D11 device.
> +   * Returns true on success, or false on failure and leaves the D2D1/Direct3D11 devices unset.
> +   */
> +  static bool SetDirect3D11Device(ID3D11Device *aDevice);

All this would be much clearer if the method was named SetDirectD3D11DeviceForD2D or something like that :)

::: gfx/thebes/gfxWindowsPlatform.cpp
@@ +2510,5 @@
> +  // Verify that Direct2D device creation succeeded.
> +  if (!Factory::SetDirect3D11Device(mD3D11ContentDevice)) {
> +    mD2D1Status = FeatureStatus::Failed;
> +    return;
> +  }

Alternatively, without the signature change:

Factory::SetDirect3D11Device(mD3D11ContentDevice);
if (!Factory::GetD2D1Device()) {
  mD2D1Status = FeatureStatus::Failed;
  return;
}

Do we also need to set mD3D11ContentDevice to null here?  Do we need a D3D11 content without D2D?

Comment 3

[Lee Salzman [:lsalzman]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Comment on attachment 8734870 [details] [diff] [review]
> check that D2D1 device creation succeeds and otherwise fall to software
> backend
> 
> Review of attachment 8734870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/2D.h
> @@ +1376,5 @@
> > +  /*
> > +   * Attempts to create and install a D2D1 device from the supplied Direct3D11 device.
> > +   * Returns true on success, or false on failure and leaves the D2D1/Direct3D11 devices unset.
> > +   */
> > +  static bool SetDirect3D11Device(ID3D11Device *aDevice);
> 
> All this would be much clearer if the method was named
> SetDirectD3D11DeviceForD2D or something like that :)
>

Might be worth undertaking this cleanup in the future, but for right now I'd rather go with the fix Bas and I agreed upon in discussion to make sure we get the issue resolved promptly.

> ::: gfx/thebes/gfxWindowsPlatform.cpp
> @@ +2510,5 @@
> > +  // Verify that Direct2D device creation succeeded.
> > +  if (!Factory::SetDirect3D11Device(mD3D11ContentDevice)) {
> > +    mD2D1Status = FeatureStatus::Failed;
> > +    return;
> > +  }
> 
> Alternatively, without the signature change:
> 
> Factory::SetDirect3D11Device(mD3D11ContentDevice);
> if (!Factory::GetD2D1Device()) {
>   mD2D1Status = FeatureStatus::Failed;
>   return;
> }
> 
> Do we also need to set mD3D11ContentDevice to null here?  Do we need a D3D11
> content without D2D?

Yep, needs to be null for now, since the availability of the D3D11 content device and D2D inside Factory as intertwined.

Comment 4

[Lee Salzman [:lsalzman]]

Oops, misunderstand, above. What I meant to say was that the D3D11 device within the Factory needs to be null provided D2D device creation failed, but outside of Factory it should not be an issue.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92ea1417e1d9

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c39faad857e5

Comment 6

[Carsten Book [:Tomcat]]

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

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Does this patch apply to aurora and beta as is?  If not, let's prepare those versions; I anticipate us wanting to uplift this later this week, assuming we see the stop in crashes (here and bug 1189715.)

Comment 8

[Lee Salzman [:lsalzman]]

Attachment: check that D2D1 device creation succeeds and otherwise fall to software backend (46 beta)

The existing patch should apply cleanly against 47 aurora. But beta required some slight merging, which is handled in this patch. Just getting this one ready so we can uplift it if/when necessary.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8734870 [details] [diff] [review]
check that D2D1 device creation succeeds and otherwise fall to software backend

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: #7 crash in 46, see bug 1189715
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: Low in that we're stopping a crash and falling back into a supported path (unaccelerated content.)
[String/UUID change made/needed]:

I will ask for the beta uplift on the beta specific patch.

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8736129 [details] [diff] [review]
check that D2D1 device creation succeeds and otherwise fall to software backend (46 beta)

Approval Request Comment:
Rebased beta patch for the above.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8734870 [details] [diff] [review]
check that D2D1 device creation succeeds and otherwise fall to software backend

Crash fix, Aurora47+

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8736129 [details] [diff] [review]
check that D2D1 device creation succeeds and otherwise fall to software backend (46 beta)

Fix for topcrash in beta, please uplift for beta 8 build today

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/6b34b6fb1b62

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ee1fb7522207