Make D2D singleton accessors thread-safe

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dvander, Assigned: dvander)

Tracking

(Blocks 1 bug)

40 Branch
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

According to MSDN, Direct2D is threadsafe [1], and will internally serialize draw commands on the same device. However we should make sure that the actual pointers are stored and accessed in a thread-safe way.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/jj569217(v=vs.85).aspx
This patch uses StaticRefPtr to store singletons, and RefPtr to return them.
Attachment #8886475 - Flags: review?(mchang)
Posted patch part 2, use mutexes (obsolete) — Splinter Review
We can use mutexes to ensure that if the main thread mutates a singleton, the paint thread will have already correctly AddRef'd the old object, thus avoiding any mismatching AddRef/Release issues. It works because the return RefPtrs are constructed within the lock. (Actually hitting an issue like this should be extremely rare, but better safe than sorry.)

This also forbids the paint thread from constructing singletons lazily via accessors. The main thread is responsible for doing so, and we don't want a race where the paint thread accidentally starts initializing moz2d stuff after a device reset.

Instead, if the paint thread races like this, it'll get back a nullptr.
Attachment #8886477 - Flags: review?(mchang)
Mason, should we also consider flushing the paint thread before handling a device reset?
(In reply to David Anderson [:dvander] from comment #3)
> Mason, should we also consider flushing the paint thread before handling a
> device reset?

What happens in a device reset now?
Attachment #8886475 - Flags: review?(mchang) → review+
(In reply to Mason Chang [:mchang] from comment #4)
> (In reply to David Anderson [:dvander] from comment #3)
> > Mason, should we also consider flushing the paint thread before handling a
> > device reset?
> 
> What happens in a device reset now?

Right now, in theory, the painting thread could be using D2D during a device reset. Part #2 makes pointers start returning null if the paint thread tries to acquire a new singleton in the middle, and in theory the paint thread could start mixing devices together.

As I say this, it sounds real bad so I think I'll do a third patch to flush the paint thread.
(In reply to David Anderson [:dvander] from comment #5)
> (In reply to Mason Chang [:mchang] from comment #4)
> > (In reply to David Anderson [:dvander] from comment #3)
> > > Mason, should we also consider flushing the paint thread before handling a
> > > device reset?
> > 
> > What happens in a device reset now?
> 
> Right now, in theory, the painting thread could be using D2D during a device
> reset. Part #2 makes pointers start returning null if the paint thread tries
> to acquire a new singleton in the middle, and in theory the paint thread
> could start mixing devices together.
> 
> As I say this, it sounds real bad so I think I'll do a third patch to flush
> the paint thread.

Sorry, what happens now w/o OMTP?
Since it's all single-threaded, we revoke the D2D singletons and recreate them in one go. There's no races.
Comment on attachment 8886477 [details] [diff] [review]
part 2, use mutexes

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

Hopefully the locks don't show up on Talos :(.

::: gfx/2d/2D.h
@@ +1692,5 @@
>    static StaticRefPtr<ID2D1Device> mD2D1Device;
>    static StaticRefPtr<ID3D11Device> mD3D11Device;
>    static StaticRefPtr<IDWriteFactory> mDWriteFactory;
> +protected:
> +  static StaticMutex mDeviceLock;

nit: Add comment that says this protects the 3 things above

::: gfx/2d/Factory.cpp
@@ +203,5 @@
>  static uint32_t mDeviceSeq = 0;
>  StaticRefPtr<ID3D11Device> Factory::mD3D11Device;
>  StaticRefPtr<ID2D1Device> Factory::mD2D1Device;
>  StaticRefPtr<IDWriteFactory> Factory::mDWriteFactory;
> +StaticMutex Factory::mDeviceLock;

nit: Add comment stating mDeviceLock locks three things above.

@@ +776,5 @@
>  
> +  RefPtr<ID2D1Factory1> factory;
> +  {
> +    StaticMutexAutoUnlock unlock(mDeviceLock);
> +    factory = D2DFactory();

This is scary :(. Can we just move this up to a scoped thing at the top of the method to fetch the factory before grabing the mDeviceLock? I don't dependent locks in called functions.
Attachment #8886477 - Flags: review?(mchang) → review+
Comment on attachment 8886708 [details] [diff] [review]
part 3, flush async paints

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

::: gfx/thebes/DeviceManagerDx.cpp
@@ +707,5 @@
>  {
> +  // Flush the paint thread before revoking all these singletons. This
> +  // should ensure that the paint thread doesn't start mixing and matching
> +  // old and new objects together.
> +  if (XRE_IsContentProcess() && PaintThread::Get()) {

nit: Don't we only  need PaintThread::Get() here?
Attachment #8886708 - Flags: review?(mchang) → review+
This version removes GetD2D1DeviceSeq, since a common pattern was to grab the lock, get the device, grab the lock again, get the sequence number. The new patch acquires both the device and sequence number with one lock.
Attachment #8886477 - Attachment is obsolete: true
Attachment #8886720 - Flags: review?(mchang)
Attachment #8886720 - Flags: review?(mchang) → review+
This conflicted badly with bug 1376026, which introduced some of the fixes from this patch at the same time. As a result I merged mDeviceLock with mDWriteFactoryLock into a single StaticMutex.

Comment 13

2 years ago
Pushed by danderson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc529efe4e79
Store and return D2D singletons in RefPtrs. (bug 1380922 part 1, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/50011cd29784
Protect D2D singletons with a mutex. (bug 1380922 part 2, r=mchang)
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cd50771d27a
Flush async paints before revoking device singletons. (bug 1380922 part 3, r=mchang)

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc529efe4e79
https://hg.mozilla.org/mozilla-central/rev/50011cd29784
https://hg.mozilla.org/mozilla-central/rev/9cd50771d27a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.