899832 - hoist JSScript::originPrincipals and LazyScript::originPrincipals into ScriptSource

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

The "origin principals" are the same for the entire tree of scripts (lazy and eager) created by a single compilation so the hoisting is pretty straightforward.

Comment 1

[Luke Wagner [:luke]]

Attachment: hoist-origin-principals

Double bonus points: the patch gets to remove padding32.

The current originPrincipals "normalization" we do is kinda ad hoc and I would've had to inject it in a random third place so I hoisted the normalization so that it happens at the JSAPI entry points and is asserted in the frontend entry points.

Comment 2

[Luke Wagner [:luke]]

Attachment: hoist-origin-principals

Oops, need to add padding to LazyScript on 32-bit (and a static assert).

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 783495 [details] [diff] [review]
hoist-origin-principals

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

Sorry for the delay.  It'd be good to see another version with the JS_DropPrincipals and AssertCompileOptionsNormalized stuff fixed.

::: js/src/jsapi.cpp
@@ +4333,5 @@
>          rt->destroyPrincipals(principals);
>  }
>  
>  JS_PUBLIC_API(void)
> +JS_DropPrincipals(JSPrincipals *principals)

Can you remove this method?  This is not threadsafe and the existing signature for this is better, since threads generally cannot get their hands on a JSRuntime* without going through a threadsafe assert, at least after bug 898886.

::: js/src/jsscript.cpp
@@ +1351,5 @@
>      adjustDataSize(0);
>      js_free(filename_);
>      js_free(sourceMap_);
> +    if (originPrincipals_)
> +        JS_DropPrincipals(originPrincipals_);

To get the JSRuntime* here I think it would be best to supply it as a parameter to ScriptSource::{destroy,decref}, similarly to what is done with IonCode::decref.

::: js/src/jsscript.h
@@ +317,5 @@
>          ready_(true)
>      {
>          data.source = NULL;
> +        if (originPrincipals_)
> +            JS_HoldPrincipals(originPrincipals_);

A main thread assertion on the runtime might be nice here.

@@ +1490,5 @@
> + * JSAPI clients are to leave CompileOptions.originPrincipals in which case the
> + * JS engine makes options.originPrincipals = origin.principals. This
> + * normalization step must before the originPrincipals get stored in the
> + * JSScript/ScriptSource.
> + */

Mangled comment.

@@ +1506,5 @@
> +                                                          options->originPrincipals);
> +}
> +
> +static inline void
> +AssertCompileOptionsNormalized(const CompileOptions &options)

I think this method and NormalizeCompileOptions are really only necessary because principals and originPrincipals are public members of CompileOptions.  And yet CompileOptions has setters for these members!  That struct is pretty incoherent.

Can you make CompileOptions::{principals,originPrincipals} into private members and access them through getters?  Since there is already a setter API clients shouldn't be bothered much by this change.  Then the getter for originPrincipals can just call NormalizeOriginPrincipals.

::: js/src/vm/Debugger.cpp
@@ +4120,5 @@
>       * static level will suffice.
>       */
>      CompileOptions options(cx);
>      options.setPrincipals(env->compartment()->principals)
> +           .setOriginPrincipals(env->compartment()->principals)

I don't think this is necessary with the CompileOptions changes above.

::: js/src/vm/Runtime.cpp
@@ +99,5 @@
> +void
> +PerThreadData::destroyPrincipals(JSPrincipals *principals)
> +{
> +    runtime_->destroyPrincipals(principals);
> +}

This should be removed.

Comment 4

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #3)

> Sorry for the delay.  It'd be good to see another version with the
> JS_DropPrincipals and AssertCompileOptionsNormalized stuff fixed.

Great points, will fix!

> > +        if (originPrincipals_)
> > +            JS_HoldPrincipals(originPrincipals_);
> 
> A main thread assertion on the runtime might be nice here.

JS_HoldPrincipals is threadsafe (it's just a JS_ATOMIC_INCREMENT) so while the assertion would hold, I don't think it'd really be ensuring anything necessary.

Comment 5

[Luke Wagner [:luke]]

Attachment: hoist-origin-principals

Comment 6

[Luke Wagner [:luke]]

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

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e827cc07b006