Closed Bug 1258174 Opened 4 years ago Closed 4 years ago

Access to gfxWindowsPlatform devices is not thread-safe

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: dvander, Assigned: dvander)

Details

Attachments

(2 files)

ID3D11Device is threadsafe, but RefPtr is not. Specifically it's not atomic with respect to mutations on other threads. When we mutate mD3D11Device on the main thread, and read it in the compositor, we are potentially capturing a pointer that will be Released on the main thread before we can AddRef it on the compositor.

The chances of this causing problems is very low since the device should have enough references elsewhere. But it's better to have the chance be zero instead of non-zero.
Attached patch fixSplinter Review
Note: I removed the gfxCriticalErrors for mismatched devices in the compositor. These are inherently racy because TDRs are handled on the main thread, so they make testing device resets very difficult.

It seems fine to change them into returns or some non-critical error though, so I can add them back if that's desired.
Attachment #8732585 - Flags: review?(bas)
Comment on attachment 8732585 [details] [diff] [review]
fix

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

::: gfx/thebes/gfxWindowsPlatform.h
@@ +348,5 @@
>      RefPtr<IDWriteFactory> mDWriteFactory;
>      RefPtr<IDWriteRenderingParams> mRenderingParams[TEXT_RENDERING_COUNT];
>      DWRITE_MEASURING_MODE mMeasuringMode;
>  
> +    mozilla::Monitor mDeviceMonitor;

As far as I can tell there's no re-entry and a mutex could suffice, correct?
Attachment #8732585 - Flags: review?(bas) → review+
drive-by.. GetDevice(&device) is kind of gross, can we have it return an already_AddRefed<> (or whatever the right pattern is currently)?
Attached patch follow-upSplinter Review
sure, sounds reasonable (second patch since this already landed)
Attachment #8732942 - Flags: review?(vladimir)
https://hg.mozilla.org/mozilla-central/rev/fb1dc3553bb2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.