Make JSRuntime's exclusiveAccessOwner a js::Thread::Id instead of a PRThread*

RESOLVED FIXED in Firefox 51

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: fitzgen, Assigned: fitzgen)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee: nobody → nfitzgerald
Blocks: 956899
Status: NEW → ASSIGNED
Attachment #8776148 - Flags: review?(terrence) → review+
Splitting this patch into two parts: the operator== consting and the exclusiveAccessOwner changes. The latter is causing some issues, which I'm still debugging. Landing the former in the meantime.
(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #2)
> Splitting this patch into two parts: the operator== consting and the
> exclusiveAccessOwner changes. The latter is causing some issues, which I'm
> still debugging. Landing the former in the meantime.

Pushed under the wrong bug number: https://bugzilla.mozilla.org/show_bug.cgi?id=1290317#c4

Ah well...

Comment 4

3 years ago
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b61757c08a5f
Make JSRuntime's exclusiveAccessOwner a js::Thread::Id instead of a PRThread*; r=terrence
Backed out for asserting mIsSome in testParallelCompile.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/85272f8ab75c

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b61757c08a5fbe9df2a1af7e462650b8c5b556ba
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=33515679&repo=mozilla-inbound

TEST-PASS | js/src/jit-test/tests/asm.js/testParallelCompile.js | Success (code 0, args "--ion-eager --ion-offthread-compile=off --non-writable-jitcode --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads")
TEST-PASS | js/src/jit-test/tests/asm.js/testParallelCompile.js | Success (code 0, args "--no-asmjs")
Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261
Exit code: -11
FAIL - asm.js/testParallelCompile.js
TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/asm.js/testParallelCompile.js | Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261 (code -11, args "")
INFO exit-status     : -11
INFO timed-out       : False
INFO stderr         2> Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261
Assertion failure: mIsSome, at /home/worker/workspace/build/src/obj-spider/dist/include/mozilla/Maybe.h:261
Exit code: -11
FAIL - asm.js/testParallelCompile.js
Flags: needinfo?(nfitzgerald)
Ok, so these assertions that keep failing are totally bogus. We're checking some state before locking that is not synchronized by any lock. In fact, the whole *CanLock() functions are pretty useless right now. We have checks against reentrancy and wrong thread unlocking in DEBUG builds via pthreads' ERRORCHECK mode and via the AutoLockWhatever& parameters that are thread through all these functions. It would be nice to have some dynamic ordering checks against inversion, but as it stands now, these assertions we do have are hurting way more than they are helping. I'm going to remove them.
Flags: needinfo?(nfitzgerald)
See also comment 6.

PTHREAD_MUTEX_ERRORCHECK gives us this error checking against reentrancy and
unlocking an unlocked lock or another thread's lock already, so it isn't
needed. This also makes the *CanLock assertions no-ops.

In the future, it would be nice to introduce ordering checks against
inversions. I plan to do this in a follow up.
Attachment #8779091 - Flags: review?(terrence)
Attachment #8776148 - Attachment is obsolete: true
Comment on attachment 8779091 [details] [diff] [review]
Remove JSRuntime's exclusiveAccessOwner and *CanLock assertions

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

This looks correct. That said, I'd really like to land the patch that passes the |const AutoLockForExclusiveAccess&| to the functions you're stripping of assertions either first or concurrent with this patch.
Attachment #8779091 - Flags: review?(terrence) → review+
Moved a comment about why the exclusive access lock is taken in AutoTraceSession
to right above AutoTraceSession's AutoLockForExclusiveAccess member declaration.
Attachment #8779538 - Flags: review+
Attachment #8779091 - Attachment is obsolete: true
The only place that lost a proof of lock holding assertion (as opposed to
ordering assertion) was js::ExclusiveContext::setCompartment.
Attachment #8779539 - Flags: review?(terrence)
Comment on attachment 8779539 [details] [diff] [review]
Part 1: Thread AutoLockForExclusiveAccess params through compartment setting functions as proof of lock holding

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

Awesome!
Attachment #8779539 - Flags: review?(terrence) → review+

Comment 14

3 years ago
Pushed by nfitzgerald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f8ff90165cc9
Part 0: Remove JSRuntime's exclusiveAccessOwner and *CanLock assertions; r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/eefc85446e21
Part 1: Thread AutoLockForExclusiveAccess params through compartment setting functions as proof of lock holding; r=terrence

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f8ff90165cc9
https://hg.mozilla.org/mozilla-central/rev/eefc85446e21
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.