Closed Bug 1259810 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

Unspecified
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- fixed
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

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.
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.
Attachment #8734870 - Flags: review?(bas)
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?
Attachment #8734870 - Flags: feedback+
Attachment #8734870 - Flags: review?(bas) → review+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/c39faad857e5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
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.)
Flags: needinfo?(lsalzman)
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.
Flags: needinfo?(lsalzman)
Attachment #8736129 - Flags: review+
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.
Attachment #8734870 - Flags: approval-mozilla-aurora?
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.
Attachment #8736129 - Flags: approval-mozilla-beta?
Comment on attachment 8734870 [details] [diff] [review]
check that D2D1 device creation succeeds and otherwise fall to software backend

Crash fix, Aurora47+
Attachment #8734870 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Attachment #8736129 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.