1380922 - Make D2D singleton accessors thread-safe

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

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

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

Comment 1

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

Attachment: part 1, use RefPtr

This patch uses StaticRefPtr to store singletons, and RefPtr to return them.

Comment 2

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

Attachment: part 2, use mutexes

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.

Comment 3

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

Mason, should we also consider flushing the paint thread before handling a device reset?

Comment 4

[Mason Chang [Inactive] [:mchang]]

(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?

Comment 5

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

(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.

Comment 6

[Mason Chang [Inactive] [:mchang]]

(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?

Comment 7

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

Since it's all single-threaded, we revoke the D2D singletons and recreate them in one go. There's no races.

Comment 8

[Mason Chang [Inactive] [:mchang]]

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.

Comment 9

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

Attachment: part 3, flush async paints

Comment 10

[Mason Chang [Inactive] [:mchang]]

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?

Comment 11

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

Attachment: part 2, use mutexes

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.

Comment 12

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

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

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bc529efe4e79
https://hg.mozilla.org/mozilla-central/rev/50011cd29784
https://hg.mozilla.org/mozilla-central/rev/9cd50771d27a