Closed
Bug 1258174
Opened 8 years ago
Closed 8 years ago
Access to gfxWindowsPlatform devices is not thread-safe
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: dvander, Assigned: dvander)
Details
Attachments
(2 files)
28.00 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
9.97 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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 2•8 years ago
|
||
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)?
Assignee | ||
Comment 5•8 years ago
|
||
sure, sounds reasonable (second patch since this already landed)
Attachment #8732942 -
Flags: review?(vladimir)
Comment 6•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb1dc3553bb2
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Updated•6 years ago
|
Attachment #8732942 -
Flags: review?(vladimir)
You need to log in
before you can comment on or make changes to this bug.
Description
•