Jason Orendorff [:jorendorff] Reporter
	Description • 14 years ago

JM needs them to be mutable and not shared across threads. jimb's debugger work needs this too.

	Andreas Gal :gal Assignee
	Comment 1 • 14 years ago

Feel free to steal this, but if nobody else wants it I will do it.

	Jason Orendorff [:jorendorff] Reporter
	Comment 2 • 14 years ago

Earlier we decided to make scripts appear to be runtime-wide at the API level rather than per-compartment. That means cloning at API boundaries.

But that will be kind of ugly. Maybe we should instead make scripts per-compartment (like everything else), provide an API for cloning scripts, and require the application to clone scripts if it moves them from compartment to compartment.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 3 • 14 years ago

Could the API do the cloning automatically when needed?

	Jason Orendorff [:jorendorff] Reporter
	Comment 4 • 14 years ago

It could. The question is whether it should. It's a lot of extra goofing around and data structures that we wouldn't otherwise need.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 5 • 14 years ago

I'm hoping to have an easy to use API.

	Jason Orendorff [:jorendorff] Reporter
	Comment 6 • 14 years ago

Look -- we're all on the same team here. We should place the complexity where it makes the most sense.

If it should be outside the JS engine, Gal or I will put up the code for it.

	Jason Orendorff [:jorendorff] Reporter
	Comment 7 • 14 years ago

dvander confirmed on IRC that JM does not need separate JSScripts per-global-object. Per-compartment is fine.

We do not need cloning for scripts that are compiled with COMPILE_N_GO because those get compiled and executed in a single compartment and then thrown away.

	Jason Orendorff [:jorendorff] Reporter
	Comment 8 • 14 years ago

How about this:

1) For embeddings that use compartments (Gecko) there is a new restriction that you must execute (and destroy) a script in the same compartment where you compiled it.

2) But we add a new API to cover the case smaug is interested in.

   JSReusableScript* JS_CompileReusableScript(JSContext *cx, ...);
   ...and other JS_Compile variants...

   JSBool JS_ExecuteReusableScript(JSContext *cx, JSObject *global,
                                   JSReusableScript *script, jsval *rval);

   void JS_DestroyReusableScript(JSContext *cx, JSReusableScript *script);

Instead of returning a JSScript, JS_CompileReusableScript produces a JSReusableScript, which is basically a js::HashMap<JSCompartment *, JSScript *> with initially a single entry. It clones the script to other compartments automatically as needed.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 14 years ago

js::HashMap<JSCompartment *, JSScript *> would keep the JSScript alive, I assume?
Why is that needed, if I run the script only once in a JSContext/Compartment?

	Jason Orendorff [:jorendorff] Reporter
	Comment 10 • 14 years ago

Oh, it's just a cache. I thought it would help because a compartment may contain multiple (same-domain) web pages. But sure, if we don't need it, so much the better.

	Andreas Gal :gal Assignee
	Comment 11 • 14 years ago

Whats to do here:

- in every JSAPI function that takes a JSScript, copy the script to the right compartment and enter it into a hash map if its not already there
- purge the script cache at every GC
- deep copy the script with all call/block objects and properties

	Andreas Gal :gal Assignee
	Comment 12 • 14 years ago

This isn't the fastest way to clone a script, but with bhacket's changes to what kind of objects can live in the objectList of a script this seems like the best way to go forward. The patch doesn't cache either right now, we re-clone every time. If either the recompilation or the lack of caching turn into a performance problem, we can optimize harder.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 13 • 14 years ago

Andreas, are we keeping the invariant that cloned scripts cannot be compile-n-go? (Please say yes, for now :)

	Andreas Gal :gal Assignee
	Comment 14 • 14 years ago

Coming upstairs to discuss.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 15 • 14 years ago

Compiling scripts is pretty slow (which is why JSScripts are
cached for messageManager, Bug 584866), so I really hope we don't need to
re-compile for each compartment.

	Andreas Gal :gal Assignee
	Comment 16 • 14 years ago

We can try to clone more efficiently if this really turns out to be a performance problem. I would like to see that first in numbers though. In general it should be rare that you compile a script for principal A and then run it on principal B. XBL does it, but I hope nobody else does.

	Andreas Gal :gal Assignee
	Comment 17 • 14 years ago

Actually I think we should not automatically clone at the API boundary and instead just expose CloneScript. Blake?

	Blake Kaplan (:mrbkap) (inactive)
	Comment 18 • 14 years ago

Comment on attachment 465827 [details] [diff] [review]
patch

Actually, gal and I think that this is only needed from under the JS_CloneFunction API. He'll come up with a new patch.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 19 • 14 years ago

    (In reply to comment #16)
    > We can try to clone more efficiently if this really turns out to be a
    > performance problem. I would like to see that first in numbers though.
    Bug 584866 was done because of few ms TXul regressions.
    Azakai would know more.

    > In
    > general it should be rare that you compile a script for principal A and then
    > run it on principal B. XBL does it, but I hope nobody else does.
    Ah, do we have different compartments only for different principals, not for
    different JSContexts?

	Andreas Gal :gal Assignee
	Comment 20 • 14 years ago

contexts and compartments are orthogonal, many contexts can run on a compartment and contexts can share functions and scripts

	Andreas Gal :gal Assignee
	Comment 21 • 14 years ago

We don't actually use scripts across compartment boundaries, so just an API to clone them and leave that to the embedding.

We do clone functions across compartments, that we have to fix. The private of a function must be same compartment. If we are about to violate that, deep copy the function (by cloning the script).

	Andreas Gal :gal Assignee
	Comment 22 • 14 years ago

These needs some testing. David promised his intern can reproduce the issue and would test this (adrake?).

	Brendan Eich [:brendan]
	Comment 23 • 14 years ago

Comment on attachment 465853 [details] [diff] [review]
patch

>+JS_PUBLIC_API(JSScript *)
>+JS_CloneScript(JSContext *cx, JSScript *script, JSObject *obj)
>+{
>+    JS_ASSERT(obj->getCompartment(cx) != script->compartment);
>+    JS_ASSERT(script->compartment);
>+
>+    JSString *code = JS_DecompileScript(cx, script, js_anonymous_str, 0);
>+    if (!code)
>+        return NULL;
>+    return JS_CompileUCScript(cx, obj, code->chars(), code->length(),
>+                              script->filename, script->lineno);
>+}

 . . .

>+        /*
>+         * Across compartments we have to deep copy JSFunction and clone the
>+         * script (for interpreted functions).
>+         */
>+        clone = NewFunction(cx, proto, parent);
>+        if (!clone)
>+            return NULL;
>+        JSFunction *cfun = (JSFunction *) clone;
>+        cfun->nargs = fun->nargs;
>+        cfun->flags = fun->flags;
>+        cfun->u = fun->u;
>+        cfun->atom = fun->atom;
>+        clone->setPrivate(cfun);
>+        if (cfun->isInterpreted()) {
>+            JSScript *script = cfun->u.i.script;
>+            JS_ASSERT(script);
>+            JS_ASSERT(script->compartment != cx->compartment);
>+            JS_ASSERT(script->compartment == fun->getCompartment(cx));
>+            cfun->u.i.script = JS_CloneScript(cx, script, parent);

This can't possibly work! For one, 'return' is not legal outside of a function (which is how you are compiling the function's script's decompilation).

Let's have a problem clone meta-methodology for scripts, functions, blocks, regexps, and (bhackett will do this) objects and arrays. Throw on E4X objects. There's no point doing something quick and dirty that cannot possibly work.

/be

	Blake Kaplan (:mrbkap) (inactive)
	Comment 24 • 14 years ago

Oops. Sorry!

	Andreas Gal :gal Assignee
	Comment 25 • 14 years ago

I can hack up the proper cloning. Not a big deal.

	Andreas Gal :gal Assignee
	Comment 26 • 14 years ago

New patch should be ready in a couple hours. I still have snippets of the deep cloning approach lying around.

	Andreas Gal :gal Assignee
	Comment 27 • 14 years ago

Clone scripts using XDR. I think this is the right way to go. XDR is pretty fast, and it takes care of all complexities we can encounter here. Block objects have parents, we have to clone them and then restore the parent chain properly, and we have to clone the properties. XDR does all this. There is no point duplicating this. XDR into memory and back out should be fast. Brendan, what do you think?

	Brendan Eich [:brendan]
	Comment 28 • 14 years ago

Comment on attachment 466047 [details] [diff] [review]
patch

> js_CloneFunctionObject(JSContext *cx, JSFunction *fun, JSObject *parent,
>                        JSObject *proto)
> {
>     JS_ASSERT(parent);
>     JS_ASSERT(proto);
> 

Pre-existing: could you add:

    JS_ASSERT_IF(fun->isInterpreted(), !FUN_FLAT_CLOSURE(fun));

here?

>+    // serialize script
>+    JSXDRState *w = JS_XDRNewMem(cx, JSXDR_ENCODE);
>+    if (!w)
>+        return NULL;
>+    if (!JS_XDRScript(w, &script))
>+        return NULL;

Leaks w on failure.

>+    uint32 nbytes;
>+    void *p = JS_XDRMemGetData(w, &nbytes);
>+    if (!p)
>+        return NULL;

Leaks w on failure.

>+
>+    // de-serialize script
>+    JSXDRState *r = JS_XDRNewMem(cx, JSXDR_DECODE);

Missing OOM check.

>+    JS_XDRMemSetData(r, p, nbytes);
>+    if (!JS_XDRScript(r, &script))
>+        return NULL;

Leaks w and (with OOM check) r on failure. Just set script to null here instead of returning null.

>+
>+    JS_XDRDestroy(r);
>+    JS_XDRDestroy(w);
>+
>+    return script;
>+}
>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>--- a/js/src/jsscript.h
>+++ b/js/src/jsscript.h
>@@ -173,25 +173,27 @@ struct JSScript {
>     bool            hasSharps:1;      /* script uses sharp variables */
>     bool            strictModeCode:1; /* code is in strict mode */
>     bool            warnedAboutTwoArgumentEval:1; /* have warned about use of
>                                                      obsolete eval(s, o) in
>                                                      this script */
> 
>     jsbytecode      *main;      /* main entry point, after predef'ing prolog */
>     JSAtomMap       atomMap;    /* maps immediate index to literal struct */
>+    JSCompartment   *compartment; /* compartment the script was compiled for */
>     const char      *filename;  /* source filename or null */
>     uint32          lineno;     /* base line number of script */
>     uint16          nslots;     /* vars plus maximum stack depth */
>     uint16          staticLevel;/* static level for display maintenance */
>     JSPrincipals    *principals;/* principals for this script */

Is a compartment single-principals? If so, move principals into it?

r=me with fixes for all leak and OOM-handling bugs.

/be

	Brendan Eich [:brendan]
	Comment 29 • 14 years ago

I like using XDR, it supports DRY. Otherwise there's no way to avoid some amount of repitition between direct cloning and serialization-based cloning, however you hard you factor.

/be

	Andreas Gal :gal Assignee
	Comment 30 • 14 years ago

Roy, can you fix the nits and drive this in? I will be without internet until friday. If you run into problems, I will pick this up Friday.

	Brendan Eich [:brendan]
	Comment 31 • 14 years ago

Jason and Blake should weigh in on my q:

> Is a compartment single-principals? If so, move principals into it?

and followup bug as needed.

/be

	Andreas Gal :gal Assignee
	Comment 32 • 14 years ago

Yes, compartments are single principal and yes, we can move the principal into the compartment (its already there, so just delete from script).

	Roy Frostig :froystig
	Comment 33 • 14 years ago

Applied to TM tip and addressed review comments.  Seeing 33 assertion failures on a Linux debug build.  Of those, 26 are from the assertion Brendan asked for in top of Comment #28.  All seven others are from

>+            JS_ASSERT(script->compartment != cx->compartment);

in the original patch (and also assert just the same with only the original patch applied).

Posting the modified patch with the review comments addressed just for reference.

	Roy Frostig :froystig
	Comment 34 • 14 years ago

First assertion turns out to be misplaced, so false alarm there, but second assertion is legitimate:  The incoming JSFunction we wish to clone is already in the destination compartment, lies to us and says it's in the default compartment instead, and then we end up deep copying and failing when asserting that the JSFunction actually moved across compartments.


The lies that throw us off seem to come from JSObject::getCompartment.  We're hitting the branch therein that suspiciously says

>        // Compile-time Function, Block, and RegExp objects are not parented.
>        if (clasp == &js_FunctionClass || clasp == &js_BlockClass || clasp == &js_RegExpClass) {
>            // This is a bogus answer, but it'll do for now.
>            return cx->runtime->defaultCompartment;
>        }

Indeed, placing the following assert

>    JS_ASSERT_IF(fun->isInterpreted(), fun->u.i.script->compartment == fun->getCompartment(cx));

at the top of js_CloneObjectFunction causes failures in a superset of the already-failing testcases.  Should an interpreted function ever be reporting a different compartment than the script it holds?

	Jason Orendorff [:jorendorff] Reporter
	Comment 35 • 14 years ago

For compile-time Function objects, you can return
GET_FUNCTION_PRIVATE(cx, obj)->u.i.script->compartment.

	Brendan Eich [:brendan]
	Comment 36 • 14 years ago

(In reply to comment #35)
> For compile-time Function objects, you can return
> GET_FUNCTION_PRIVATE(cx, obj)->u.i.script->compartment.

One finally can write (when did this go in? Yay, to whoever did it):

obj->getFunctionPrivate()->u.i.script->compartment.

/be

	Mike Shaver (:shaver -- probably not reading bugmail closely)
	Comment 37 • 14 years ago

Was b5+ for JM, which is landing after b5: b6+.

	Andreas Gal :gal Assignee
	Comment 38 • 14 years ago

This is done but blocked by the landing of the new compartmental GC. We need the true compartment, not the fake getCompartment(cx) functionality.

	Roy Frostig :froystig
	Comment 39 • 14 years ago

Here's the latest version of the WIP patch after I fixed all leaks and crashes I was seeing on tryserver.  This still occasionally fails at certain assert points.  As Andreas mentioned, this is due to the interim version of JSObject::getCompartment violating the patch's assumption that an interpreted JSFunction and its corresponding JSScript live in the same compartment.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 40 • 14 years ago

Just rebased this on top of Bug 599503.

	Andreas Gal :gal Assignee
	Comment 41 • 14 years ago

refreshed

	Andreas Gal :gal Assignee
	Comment 47 • 14 years ago

http://hg.mozilla.org/tracemonkey/rev/a74e7c9e1880

	Andreas Gal :gal Assignee
	Comment 48 • 14 years ago

Backed out and re-landed.

http://hg.mozilla.org/tracemonkey/rev/861b91b0a9eb

	Andreas Gal :gal Assignee
	Comment 49 • 14 years ago

Backed out again. This can't land on TM without blake's patch queue since we don't set compartments right for event handlers and the cloning mechanism ends up trying to clone a compile-and-go function.

http://hg.mozilla.org/tracemonkey/rev/daa55e52e942

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 51 • 14 years ago

Please note that we have now created a branch for beta 7 work. In addition to landing your fix on mozilla-central default, please also land it on mozilla-central GECKO20b7pre_20101006_RELBRANCH

(note: when landing on mozilla-central default, you will be given priority in the landing queue at https://wiki.mozilla.org/LandingQueue )

	Robert Sayre
	Comment 52 • 14 years ago

http://hg.mozilla.org/mozilla-central/rev/a2dd5130bb3a