Add ExclusiveContext::compartment/zone methods

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 786426 [details] [diff] [review]
patch (3ec1dff8c460)

With bug 898886 fixed, you can't readily get from a compartment/zone to its runtime without going through a threadsafe assertion, so it's fine to make these directly accessible to threads with an ExclusiveContext (which have exclusive access to their compartment/zone).  The attached patch does this, gets rid of some crufty methods on ExclusiveContext for accessing data in the compartment, and adds bits to the runtime to ensure that data which can be accessed off thread is done so while holding a lock, whether that data is accessed via a context or the runtime itself.
Attachment #786426 - Flags: review?(wmccloskey)

Comment 1

5 years ago
I happened to notice this patch introduces a few "used by never defined" warnings for JSScript::global() and ObjectImpl::compartment() in gcc
Comment on attachment 786426 [details] [diff] [review]
patch (3ec1dff8c460)

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

Looks good!

::: js/src/jsapi.cpp
@@ -5370,5 @@
>  JS_CallFunctionValue(JSContext *cx, JSObject *objArg, jsval fval, unsigned argc, jsval *argv,
>                       jsval *rval)
>  {
>      RootedObject obj(cx, objArg);
> -    JS_THREADSAFE_ASSERT(cx->compartment() != cx->runtime()->atomsCompartment);

Can we get rid of the definition of JS_THREADSAFE_ASSERT?

::: js/src/jscntxt.h
@@ +340,5 @@
>  
> +    // Threads with an ExclusiveContext may freely access any data in their
> +    // compartment and zone.
> +    JSCompartment *compartment() const { return compartment_; }
> +    JS::Zone *zone() const {

It seems like this change has one problem--it can expose the atomsCompartment. Normally we won't access the atoms compartment except on special paths, so that's probably not unsafe. However, could we add an assertion like "if compartment_/zone_ is the atoms compartment/zone, then assert that we have the lock"? Even better would be to assert that compartment_/zone_ is not the atoms compartment/zone. I can't think of a case where this would occur, but maybe I'm wrong.

::: js/src/jscompartment.cpp
@@ +126,5 @@
>      if (!ionRuntime_->initialize(cx)) {
>          js_delete(ionRuntime_);
>          ionRuntime_ = NULL;
>  
> +        JSCompartment *c = cx->runtime()->atomsCompartment();

Please rename to comp. I'd like to standardize on that where possible.
Attachment #786426 - Flags: review?(wmccloskey) → review+

Updated

5 years ago
Blocks: 902506
This busted Fedora SM(r) builds (at least), with:
{
/builds/slave/m-in_l64-d_sm-ra-0000000000000/src/js/src/gc/Verifier.cpp: In function 'void CheckStackRoot(JSRuntime*, uintptr_t*, Rooter*, Rooter*)':
/builds/slave/m-in_l64-d_sm-ra-0000000000000/src/js/src/gc/Verifier.cpp:71:74: error: invalid use of member function (did you forget the '()' ?)
/builds/slave/m-in_l64-d_sm-ra-0000000000000/src/js/src/gc/Verifier.cpp:71:74: error: base operand of '->' is not a pointer
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=26446886&tree=Mozilla-Inbound

Backed out:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/118488b8f1d5
https://hg.mozilla.org/mozilla-central/rev/64ab5bb8af51
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.