1225645 - Null-check D3D10 devices in a few places

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

bug 1183910 added a Factory::SetDirect3D10Device(nullptr) call on TDRs. There are a few places that weren't updated to null check for this.

Comment 1

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

Attachment: null checks

Comment 2

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

Comment on attachment 8688674 [details] [diff] [review]
null checks

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

These are null pointer checks, some with immediate crashes to follow, so if it passes review, I'd like to see it uplifted to 44 and 43 to match the change that introduced the resetting of the devices.

::: gfx/2d/SourceSurfaceD2DTarget.cpp
@@ +98,5 @@
>  
> +  if (!Factory::GetDirect3D10Device()) {
> +    gfxCriticalError() << "Invalid D3D10 device in D2D target surface";
> +    return nullptr;
> +  }

It appears we are ready for this to return nullptr in the call sites?

Comment 3

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

Comment on attachment 8688674 [details] [diff] [review]
null checks

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

These are null pointer checks, some with immediate crashes to follow, so if it passes review, I'd like to see it uplifted to 44 and 43 to match the change that introduced the resetting of the devices.

::: gfx/2d/SourceSurfaceD2DTarget.cpp
@@ +98,5 @@
>  
> +  if (!Factory::GetDirect3D10Device()) {
> +    gfxCriticalError() << "Invalid D3D10 device in D2D target surface";
> +    return nullptr;
> +  }

It appears we are ready for this to return nullptr in the call sites?

Comment 4

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

Attachment: v2

Hrm good catch. New patch.

Comment 5

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

I see quite a few crashes under SyncObjectD3D11::FinalizeFrame, and some of these are due to a null ID3D10Device, such as:

https://crash-stats.allizom.org/report/index/1f6f09d3-3dae-47cd-9e05-5d98e2151111

But some of these are versions older than Firefox 43, so this must have been a problem regardless. Sounds like a good candidate for uplift.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/98bd7a8e4705

Comment 7

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/740e5cca5434 for https://treeherder.mozilla.org/logviewer.html#?job_id=17486863&repo=mozilla-inbound

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7683624ba826

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7683624ba826

Comment 10

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

Comment on attachment 8688835 [details] [diff] [review]
v2

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Potential intermittent crashes on Windows Vista/7 when graphics drivers reset.
[Describe test coverage new/current, TreeHerder]: Nightly
[Risks and why]: No risk, just null checks.
[String/UUID change made/needed]:

Comment 11

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

Marking affected for 43+ from comment 5.

Comment 12

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

Comment on attachment 8688835 [details] [diff] [review]
v2

Graphics driver crash fix, ok to uplift to aurora and beta. 
If this causes any issues, please back it out.

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/1477b8cf6b93

Comment 14

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

I'm hitting a conflict uplifting this to beta.

Comment 15

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

Attachment: bug1225645-beta.patch

rebased, carrying r+ forward

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2eb3cf5ae2bc

Comment 17

[Carsten Book [:Tomcat]]

and backed out in  https://hg.mozilla.org/releases/mozilla-beta/rev/2efcef291e80 since the beta patch caused a bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=665252&repo=mozilla-beta

Comment 18

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

Hey David, we're going to build for beta 8 tomorrow morning, so you could still try again to land this.

Comment 19

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

http://hg.mozilla.org/releases/mozilla-beta/rev/7b0f3cd65f62

Comment 20

[Carsten Book [:Tomcat]]

(In reply to David Anderson [:dvander] from comment #19)
> http://hg.mozilla.org/releases/mozilla-beta/rev/7b0f3cd65f62

setting flag