Remove `lockOwner`s since they are now redundant

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

We do all this bookkeeping about who owns what lock, but that is all redundant
now that we use `PTHREAD_MUTEX_ERRORCHECK` with `js::Mutex` in `DEBUG` builds.
The checks that the `lockOwner` member was used for are redundant now that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.
Attachment #8775619 - Flags: review?(terrence)
The checks that the `lockOwner` member was used for are now redundant given that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.

In methods where we asserted `isLocked()`, we now pass in an
`AutoLockHelperThreadState` reference for compile-time proof of lock
holding. This is mostly a mechanical change.
Attachment #8775620 - Flags: review?(terrence)
The checks that the `lockOwner` member was used for are redundant now that
`js::Mutex` uses `PTHREAD_MUTEX_ERRORCHECK` in `DEBUG` builds.
Attachment #8775621 - Flags: review?(terrence)
Assignee: nobody → nfitzgerald
Blocks: 956899
Status: NEW → ASSIGNED
Attachment #8775619 - Flags: review?(terrence) → review+
Comment on attachment 8775620 [details] [diff] [review]
Remove the DEBUG-only lockOwner member from GlobalHelperThreadState

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

::: js/src/vm/Runtime.cpp
@@ +908,3 @@
>          MOZ_FALLTHROUGH;
>        case GCLock:
>          break;

It would be nice to find a way to assert ordering of these locks statically. Maybe AutoLockGC should take the HelperThreadLock in its constructor?
Attachment #8775620 - Flags: review?(terrence) → review+
Attachment #8775621 - Flags: review?(terrence) → review+
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/573529c879ad
Remove the GCRuntime DEBUG-only lockOwner field; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bc0a14de00a
Remove the DEBUG-only lockOwner member from GlobalHelperThreadState; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/41096329fbed
Remove the DEBUG-only lockOwner member from vixl::Simulator; r=terrence
This or something else from the push appears to have turned 10.10 debug cpp tests permared: https://treeherder.mozilla.org/logviewer.html#?job_id=33028238&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/8786fe9a1993
Flags: needinfo?(nfitzgerald)
Flags: needinfo?(nfitzgerald)
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a261bbbe735
Remove the GCRuntime DEBUG-only lockOwner field; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9bc531fdc75
Remove the DEBUG-only lockOwner member from GlobalHelperThreadState; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/3335ffe05307
Remove the DEBUG-only lockOwner member from vixl::Simulator; r=terrence
https://hg.mozilla.org/mozilla-central/rev/4a261bbbe735
https://hg.mozilla.org/mozilla-central/rev/a9bc531fdc75
https://hg.mozilla.org/mozilla-central/rev/3335ffe05307
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1291349
Depends on: 1426067
You need to log in before you can comment on or make changes to this bug.