Beef up threadsafe assertions when accessing runtime from compartment/zone/cells

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla26
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 782278 [details] [diff] [review]
patch (fb48c7d58b8b)

Right now we have pretty good assurances of thread safety when accessing the runtime from a context, due to the constraints on when this method is available and associated assertions.  The runtime can also be accessed from individual cells, from compartments and from zones, and there are no thread safety assertions here.  The attached patch adds assertions, changing runtime() methods on these constructs into runtimeFromMainThread() accessors which check thread safety.  There is also a runtimeFromAnyThread() in a couple places, which is used by GC methods that are protected by the runtime's GC lock, and by post barriers, which are not threadsafe (will fix this in a future patch).
Attachment #782278 - Flags: review?(wmccloskey)
On first glance, this looks fine. I'm worried about removing the runtime->needsBarrier() checks though. Are you sure that's okay? The details are in bug 852802.
(Assignee)

Comment 2

5 years ago
Created attachment 784586 [details] [diff] [review]
patch (f0ce0463bd65)

Update with runtime->needsBarrier() checks restored.  Per IRC discussion right now these will race in GGC builds though it should be harmless (zone->needsBarrier() will never be true) and this should get fixed down the line by bug 887030.
Assignee: general → bhackett1024
Attachment #782278 - Attachment is obsolete: true
Attachment #782278 - Flags: review?(wmccloskey)
Attachment #784586 - Flags: review?(wmccloskey)
Comment on attachment 784586 [details] [diff] [review]
patch (f0ce0463bd65)

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

::: js/src/ion/shared/CodeGenerator-shared.cpp
@@ +45,5 @@
>  #ifdef DEBUG
>      pushedArgs_(0),
>  #endif
>      lastOsiPointOffset_(0),
> +    sps_(&GetIonContext()->runtime->spsProfiler, &lastPC_),

This sorta suggests that PJS code probably doesn't interact well with the profiler. Oh well I guess.
Attachment #784586 - Flags: review?(wmccloskey) → review+
Depends on: 901799
https://hg.mozilla.org/mozilla-central/rev/f836042326f9
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
(Assignee)

Updated

5 years ago
Blocks: 902095
You need to log in before you can comment on or make changes to this bug.