1309909 - Pick a mutex ordering and enforce it with assertions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

At the moment it's easy to add the possibility of deadlock by acquiring mutexes in a different order in different places in the engine.  We should pick an ordering and assert that it is obeyed.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: mutex-order-1

This patch renames the current js::Mutex to js::detail::MutexImpl so that I can make js::Mutex a wrapper that adds ordering checks in debug builds.

Comment 2

[Jon Coppeard (:jonco)]

Attachment: mutex-order-2

This patch makes the mutex constructor take a new MutexId struct containing a name (for debugging purposes) and the lock order.  

In debug builds the js::Mutex checks the order and forwards lock/unlock calls to the MutexImpl.  To do this it maintains a stack of currently held mutexes on each thread.

The MutexIds come from a bunch of constants defined in Mutex.h.  It may be worth moving this somewhere outside of the threading directory but I put it here to start with.

For the mutex order data I tried as much as possible to give mutexes that are never held at the same time the same order, to make it more obvious which possible combinations can actually happen in practice.  With a bit of fiddling to we might be able to reduce this further but I didn't want to change any engine behaviour with this patch.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8802599 [details] [diff] [review]
mutex-order-1

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

WFM

Another option would be to make MutexImpl templatized on a type providing whatever debug methods/hooks that has two implementations one for DEBUG and one that is a noop for non-DEBUG. Something like:

    #ifdef DEBUG
    using Mutex = js::detail::MutexImpl<DebugMutexMethods>;
    #else
    using Mutex = js::detail::MutexImpl<NopMutexMethods>;
    #endif

Or maybe this is essentially what you're planning?

Either way, I don't feel strongly about it.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8802601 [details] [diff] [review]
mutex-order-2

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

This is really great! The way you built this into the core of js::Mutex rather than tacking on spot asssertions here and there, I think this obsoletes my more complicated dynamic lock ordering violations discovery plan.

::: js/src/threading/Mutex.h
@@ +147,5 @@
> +
> +  using MutexVector = mozilla::Vector<const Mutex*>;
> +  static MOZ_THREAD_LOCAL(MutexVector*) HeldMutexStack;
> +
> +  MutexVector& heldMutexStack();

Nit: stylistically, I'd prefer if this was a static method.

Additionally, we should quickly document how we are using the HeldMutexStack to enforce our lock ordering either here or with the comment describing MutexId and the orderings.

::: js/src/threading/MutexCommon.cpp
@@ +11,5 @@
> +using namespace js;
> +
> +#ifdef DEBUG
> +
> +MOZ_THREAD_LOCAL(js::Mutex::MutexVector*) js::Mutex::HeldMutexStack;

Who is going to free this? Can we put js::UniquePtr in TLS?

Comment 5

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #4)
> ::: js/src/threading/MutexCommon.cpp
> @@ +11,5 @@
> > +using namespace js;
> > +
> > +#ifdef DEBUG
> > +
> > +MOZ_THREAD_LOCAL(js::Mutex::MutexVector*) js::Mutex::HeldMutexStack;
> 
> Who is going to free this? Can we put js::UniquePtr in TLS?

And also assert is is empty upon destruction?

Comment 6

[Jon Coppeard (:jonco)]

Attachment: bug1309909-mutex-order

Thanks for the comments.

> Nit: stylistically, I'd prefer if this was a static method.

ixed.

> Who is going to free this? Can we put js::UniquePtr in TLS?

It seems it allows integral types and pointers only.  I added code to free this in shutdown instead.

Other than that the patch is mainly the same with some code moved around:
 - renamed threading/*/Mutex.cpp to MutexImpl.cpp
 - renamed threading/MutexCommon.cpp to Mutex.cpp
 - moved mutex order definition to vm/MutexIDs.h

I fixed mips builds, tested and fixed an order problems with the trace logger.

One thing that came up though was the destruction of sigIdSet in asmjs/WasmInstance.cpp.  This is a global ExclusiveData<SigIdSet> and makes uses of locking in its destructor.  Unfortunately this can conflict with the destruction of the TLS for Mutex (the order is unspecified AIUI) and if they happen the wrong way round we get failures.  I changed sidIdSet so it gets explicitly initialised/destroyed via JS_Init/JS_Shutdown.

Comment 7

[Jon Coppeard (:jonco)]

Comment on attachment 8802929 [details] [diff] [review]
bug1309909-mutex-order

Requesting review for the wasm changes as described above.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8802929 [details] [diff] [review]
bug1309909-mutex-order

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

Looks wonderful!

::: js/src/threading/Mutex.h
@@ +95,5 @@
> +
> +// In debug builds, js::Mutex is a wrapper over MutexImpl that checks correct
> +// locking order is observed.
> +//
> +// The class maintians a per-thread stack of currently-held mutexes to enable it

maintians -> maintains

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8802929 [details] [diff] [review]
bug1309909-mutex-order

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

cool!

::: js/src/asmjs/WasmInstance.h
@@ +136,5 @@
>  
>  typedef UniquePtr<Instance> UniqueInstance;
>  
> +bool InitInstance();
> +void ShutDownInstance();

Could you rename these to (Init|ShutDown)InstanceStaticData() please?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0376c4d982a
Give each mutex an order and check the order of aquisition r=fitzgen r=luke

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e0376c4d982a

Comment 12

[Michael Smith [:mismith] (non-bmo mail: [e-mail])]

This seems to have broken building with |--enable-perf|. PerfSpewer.cpp [0] tries to construct js::Mutex with no arguments but this patch removed the zero-parameter constructor from js::Mutex [1].

[0] https://dxr.mozilla.org/mozilla-central/source/js/src/jit/PerfSpewer.cpp#97
[1] https://dxr.mozilla.org/mozilla-central/source/js/src/threading/Mutex.h#88

Comment 13

[Jon Coppeard (:jonco)]

(In reply to Michael Smith [:mismith] from comment #12)
Indeed, I filed bug 1314565 for this.