Closed Bug 1225645 Opened 4 years ago Closed 4 years ago

Null-check D3D10 devices in a few places

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed

People

(Reporter: dvander, Assigned: dvander)

Details

Attachments

(2 files, 1 obsolete file)

bug 1183910 added a Factory::SetDirect3D10Device(nullptr) call on TDRs. There are a few places that weren't updated to null check for this.
Attached patch null checks (obsolete) — Splinter Review
Attachment #8688674 - Flags: review?(bas)
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 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?
Attachment #8688674 - Flags: feedback+
Attached patch v2Splinter Review
Hrm good catch. New patch.
Attachment #8688674 - Attachment is obsolete: true
Attachment #8688674 - Flags: review?(bas)
Attachment #8688835 - Flags: review?(bas)
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.
Status: NEW → ASSIGNED
Attachment #8688835 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/7683624ba826
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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]:
Attachment #8688835 - Flags: approval-mozilla-beta?
Attachment #8688835 - Flags: approval-mozilla-aurora?
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.
Attachment #8688835 - Flags: approval-mozilla-beta?
Attachment #8688835 - Flags: approval-mozilla-beta+
Attachment #8688835 - Flags: approval-mozilla-aurora?
Attachment #8688835 - Flags: approval-mozilla-aurora+
I'm hitting a conflict uplifting this to beta.
Flags: needinfo?(dvander)
rebased, carrying r+ forward
Flags: needinfo?(dvander)
Attachment #8692228 - Flags: review+
Attachment #8692228 - Flags: approval-mozilla-beta?
Hey David, we're going to build for beta 8 tomorrow morning, so you could still try again to land this.
Flags: needinfo?(dvander)
Attachment #8692228 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.