902095 - Add ExclusiveContext::compartment/zone methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 1

[Luke Wagner [:luke]]

I happened to notice this patch introduces a few "used by never defined" warnings for JSScript::global() and ObjectImpl::compartment() in gcc

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fde265c467b2

Comment 4

[Daniel Holbert [:dholbert]]

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

Comment 5

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64ab5bb8af51

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64ab5bb8af51