1573774 - Weird mutex use in Thread::idMutex_

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Paul Bone [:pbone] (PTO until 2026-01-05)]

js::Thread uses a mutex field idMutex_ to protect the id_ field. But since it uses a seperate mutex for each thread things can get weird when one thread uses the id_ field of another thread. The move and constructor & assignment operators are even inconsistent in which they lock. Furthermore they never lock both.

Maybe we don't need this lock at all, because the only time we modify the id_ field is in create(), join() and detach(). Maybe it's more protection than we need. At the very least we can have a single static mutex for all id_ fields.

Comment 1

[Jan de Mooij [:jandem]]

(In reply to Paul Bone [:pbone] from comment #0)

At the very least we can have a single static mutex for all id_ fields.

The code in Thread::join is a bit annoying:

  LockGuard<Mutex> lock(idMutex_);
  MOZ_RELEASE_ASSERT(joinable(lock));
  int r = pthread_join(id_.platformData()->ptThread, nullptr);
  MOZ_RELEASE_ASSERT(!r);
  id_ = Id();

pthread_join can be slow. If there's a single mutex we block all other threads in the system that are trying to do something with threads.

Comment 2

[Paul Bone [:pbone] (PTO until 2026-01-05)]

(In reply to Jan de Mooij [:jandem] from comment #1)

(In reply to Paul Bone [:pbone] from comment #0)

At the very least we can have a single static mutex for all id_ fields.

The code in Thread::join is a bit annoying:

pthread_join can be slow. If there's a single mutex we block all other threads in the system that are trying to do something with threads.

That's a good point. I'll take care around that.

We could unlock while pthread_join() executes. This could allow multiple threads to call join() on a single thread.

What do you think about removing this mutex altogether? is it really that important to protect id_?

The only races I can see happening are between multiple threads calling join on a single thread. Or a race between detach() and join() or detach() and a thread accessing its own Id_ field. I suppose you could make other races but I feel they'd be contrived.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Paul Bone [:pbone] from comment #2)

What do you think about removing this mutex altogether? is it really that important to protect id_?

Luke added this mutex a few years ago, maybe he remembers more.

Comment 4

[Paul Bone [:pbone] (PTO until 2026-01-05)]

Attachment: Bug 1573774 - Use a single mutex for all Thread::id_ r=jandem

Comment 5

[Paul Bone [:pbone] (PTO until 2026-01-05)]

Attachment: Bug 1573774 - Don't include Mutex.h from Thread.h

Depends on D42110

Comment 6

[Paul Bone [:pbone] (PTO until 2026-01-05)]

See Bug 1406421 Comment 5 and Bug 1347644 Comment 27,

It looks like, especially from that last link, that something about finishing a thread but maybe the thread's creator hasn't set it's ID yet goes "badly".

Comment 7

[Luke Wagner [:luke]]

IIRC, there was a race between thread creation and the id_ field being initialized because you only get the thread id when pthread_create returns but, of course, by that time the thread has already started.

Comment 8

[Luke Wagner [:luke]]

Oops, the problem I described was why idMutex_ is needed (more critically) in the windows Thread.cpp, where _beginthreadex() returns the HANDLE. I couldn't find any specific guarantee that pthread_create() writes into the pthread_t outparam before starting the thread, but one would hope this was true. IIRC, the race in question for POSIX's Thread.cpp was with hasThread.

Probably a better solution for both Windows/Posix would be to repurpose the mutex to only guard thread initialization: it was held during pthread_create/_beginthreadex() and dropped after the Thread fields were initialized and the first thing the thread's start function does is grab and immediately drop the mutex. But after that, no locking.

Comment 9

[Paul Bone [:pbone] (PTO until 2026-01-05)]

(In reply to Luke Wagner [:luke] from comment #8)

Oops, the problem I described was why idMutex_ is needed (more critically) in the windows Thread.cpp, where _beginthreadex() returns the HANDLE. I couldn't find any specific guarantee that pthread_create() writes into the pthread_t outparam before starting the thread, but one would hope this was true. IIRC, the race in question for POSIX's Thread.cpp was with hasThread.

Probably a better solution for both Windows/Posix would be to repurpose the mutex to only guard thread initialization: it was held during pthread_create/_beginthreadex() and dropped after the Thread fields were initialized and the first thing the thread's start function does is grab and immediately drop the mutex. But after that, no locking.

That makes sense to me. Then any reads by either the creator or createe will find the right data. And no-one writes unless they detach or join, and we already have assertions to say that can't happen.

Comment 10

[Paul Bone [:pbone] (PTO until 2026-01-05)]

There's another reason why I wanted to remove the Mutex field from Thread, I want to break that dependency so I can add the a Thread field in Mutex (to add some assertions that helped me with some debugging for 1568410). Then when I looked at idMutex_ I wanted to understand why it was there at all.

Comment 11

[Paul Bone [:pbone] (PTO until 2026-01-05)]

Attachment: Bug 1573774 - Protect a thread's id only during creation r=luke

Comment 12

[Paul Bone [:pbone] (PTO until 2026-01-05)]

Attachment: Bug 1573774 - Assert that a thread isn't already running in Thread::create() r=luke

Depends on D42801

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16589104ca01
Protect a thread's id only during creation r=luke
https://hg.mozilla.org/integration/autoland/rev/0b3796588781
Assert that a thread isn't already running in Thread::create() r=luke

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/16589104ca01
https://hg.mozilla.org/mozilla-central/rev/0b3796588781