1568410 - Remove the need to call Mutex::ShutDown()

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Paul Bone [:pbone]]

Two fuzzbugs Bug 1567292 and Bug 1562437 have a similar cause. After using a JS_Context a caller should call Mutex::ShutDown() to free some debugging information there. Maybe we should change JS_NewContext somehow to force the caller to free this. Or do it in JS_DestroyContext() if we're the last context for this thread.

Comment 1

[Jan de Mooij [:jandem]]

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

Or do it in JS_DestroyContext() if we're the last context for this thread.

There can be at most one context per thread, so in JS_DestroyContext it's always the thread's last context :)

Comment 2

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - Make JS_DestroyContext call Mutex::Shutdown r=tcampbell

If we call Mutex::Shutdown from here then most embedders' threads that
interact with JS will do the right thing.

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - Call Mutex::ShutDown() in ~JSContext() and ~JSRuntime() r=tcampbell

If we call Mutex::ShutDown() in these destructors then we can remove it from
several threads.

Comment 4

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 1) Refactor Mutex.h r=jandem

Factor the common parts of the DEBUG and non-DEBUG Mutex class together to
make it easier to see what the common and different parts of this class are.

Comment 5

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 3) Implement the mutex stack as a linked list r=jandem

Implement this as a linked list so that it cannot leak memory (unless it
also leaks locks) and the Mutex::ShutDown() function can be removed.

Depends on D40984

Comment 6

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 4) Remove Mutex::ShutDown() r=jandem

Depends on D40985

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 2) Call the pre/post lock code from ConditionVariable

Condition variables lock and unlock mutexes. The code in the next patch
would find assertion failures because this case wasn't handled.

Depends on D40984

Comment 8

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 5) Break a would-be cyclic #inclusion r=jandem

Part 6 will #include Thread.h from Mutex.h, we can change the mutex used by
Thread.h so that Thread.h doesn't need to include Mutex.h.

Depends on D40986

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 8) Add assertions to help catch problems Mutex behaviour r=jandem

While I worked on this patch series these assertions where helpful to detect
problems.

Depends on D43003

Comment 10

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 9) Verify ownedByCurrentThread() using owningThread_ r=jandem

We can add an extra assertion here.

Depends on D43004

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 10) Clarify an edge case in a comment r=jandem

Depends on D43005

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 5) Move Thread::Id to ThreadId and its own header r=jandem

Depends on D40986

Comment 13

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 6) Move ThisThread::GetId() to ThreadId::ThisThread() r=jandem

Depends on D44430

Comment 14

[Paul Bone [:pbone]]

Attachment: Bug 1568410 - (part 7) Remove references to Thread::Id r=jandem

Depends on D44431

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01952ffb255b
(part 1) Refactor Mutex.h r=jandem
https://hg.mozilla.org/integration/autoland/rev/371db20a6eec
(part 2) Call the pre/post lock code from ConditionVariable r=jandem
https://hg.mozilla.org/integration/autoland/rev/8e7f65fa0100
(part 3) Implement the mutex stack as a linked list r=jandem
https://hg.mozilla.org/integration/autoland/rev/27fd4f64ae73
(part 4) Remove Mutex::ShutDown() r=jandem
https://hg.mozilla.org/integration/autoland/rev/d7244290aa1f
(part 5) Move Thread::Id to ThreadId and its own header r=jandem
https://hg.mozilla.org/integration/autoland/rev/77edb2b4c0c3
(part 6) Move ThisThread::GetId() to ThreadId::ThisThread() r=jandem
https://hg.mozilla.org/integration/autoland/rev/5cd99fb56ad6
(part 7) Remove references to Thread::Id r=jandem
https://hg.mozilla.org/integration/autoland/rev/eca5bab9590e
(part 8) Add assertions to help catch problems Mutex behaviour r=jandem
https://hg.mozilla.org/integration/autoland/rev/4006bbe265cb
(part 9) Verify ownedByCurrentThread() using owningThread_ r=jandem
https://hg.mozilla.org/integration/autoland/rev/5a173762d443
(part 10) Clarify an edge case in a comment r=jandem

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/01952ffb255b
https://hg.mozilla.org/mozilla-central/rev/371db20a6eec
https://hg.mozilla.org/mozilla-central/rev/8e7f65fa0100
https://hg.mozilla.org/mozilla-central/rev/27fd4f64ae73
https://hg.mozilla.org/mozilla-central/rev/d7244290aa1f
https://hg.mozilla.org/mozilla-central/rev/77edb2b4c0c3
https://hg.mozilla.org/mozilla-central/rev/5cd99fb56ad6
https://hg.mozilla.org/mozilla-central/rev/eca5bab9590e
https://hg.mozilla.org/mozilla-central/rev/4006bbe265cb
https://hg.mozilla.org/mozilla-central/rev/5a173762d443