Closed Bug 584789 Opened 11 years ago Closed 10 years ago

Make functions per compartment, and deep copy instead of clone them if needed

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: jorendorff, Assigned: gal)

References

Details

(Whiteboard: [compartments][ready in patch queue])

Attachments

(2 files, 11 obsolete files)

JM needs them to be mutable and not shared across threads. jimb's debugger work needs this too.
Feel free to steal this, but if nobody else wants it I will do it.
Assignee: general → gal
Blocks: compartments
No longer blocks: JaegerBrowser
Depends on: 567198
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.
Could the API do the cloning automatically when needed?
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.
I'm hoping to have an easy to use API.
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.
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.
blocking2.0: --- → beta5+
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.
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?
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.
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
No longer depends on: 567198
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #465827 - Flags: feedback?(mrbkap)
Andreas, are we keeping the invariant that cloned scripts cannot be compile-n-go? (Please say yes, for now :)
Coming upstairs to discuss.
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.
Attachment #465827 - Flags: feedback?(mrbkap) → review+
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.
Actually I think we should not automatically clone at the API boundary and instead just expose CloneScript. Blake?
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.
Attachment #465827 - Flags: review+
    (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?
contexts and compartments are orthogonal, many contexts can run on a compartment and contexts can share functions and scripts
Summary: Make JSScripts per-compartment, copy them at API boundaries as needed → Make functions per compartment, and deep copy instead of clone them if needed
Attached patch patch (obsolete) — Splinter Review
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).
Attachment #465827 - Attachment is obsolete: true
Attachment #465853 - Flags: review?(mrbkap)
Attachment #465853 - Flags: review?(mrbkap) → review+
These needs some testing. David promised his intern can reproduce the issue and would test this (adrake?).
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
Attachment #465853 - Flags: review+ → review-
Oops. Sorry!
I can hack up the proper cloning. Not a big deal.
Attachment #465853 - Attachment is obsolete: true
New patch should be ready in a couple hours. I still have snippets of the deep cloning approach lying around.
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #466047 - Flags: review?(brendan)
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
Attachment #466047 - Flags: review?(brendan) → review+
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
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.
Assignee: gal → rfrostig
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
Yes, compartments are single principal and yes, we can move the principal into the compartment (its already there, so just delete from script).
Attached patch patch, post-review (obsolete) — Splinter Review
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.
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?
For compile-time Function objects, you can return
GET_FUNCTION_PRIVATE(cx, obj)->u.i.script->compartment.
(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
Was b5+ for JM, which is landing after b5: b6+.
blocking2.0: beta5+ → beta6+
Depends on: 558861
This is done but blocked by the landing of the new compartmental GC. We need the true compartment, not the fake getCompartment(cx) functionality.
Assignee: rfrostig → gal
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.
Attachment #466047 - Attachment is obsolete: true
Attachment #466479 - Attachment is obsolete: true
Whiteboard: [compartments]
Blocks: 599503
Attached patch rebased patch (obsolete) — Splinter Review
Just rebased this on top of Bug 599503.
Attachment #473712 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
refreshed
Attachment #478433 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #479548 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #479663 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #479665 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #479670 - Attachment is obsolete: true
Attached patch patchSplinter Review
Attachment #479673 - Attachment is obsolete: true
Attachment #479676 - Flags: review?(mrbkap)
Attachment #479676 - Flags: review?(mrbkap) → review+
http://hg.mozilla.org/tracemonkey/rev/a74e7c9e1880
Whiteboard: [compartments] → [compartments], fixed-in-tracemonkey
Backed out and re-landed.

http://hg.mozilla.org/tracemonkey/rev/861b91b0a9eb
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
Attachment #479713 - Attachment is patch: true
Attachment #479713 - Attachment mime type: application/octet-stream → text/plain
Whiteboard: [compartments], fixed-in-tracemonkey → [compartments]
Whiteboard: [compartments] → [compartments][ready in patch queue]
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 )
http://hg.mozilla.org/mozilla-central/rev/a2dd5130bb3a
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
No longer blocks: 584860
Duplicate of this bug: 567198
Duplicate of this bug: 567199
Depends on: 643967
You need to log in before you can comment on or make changes to this bug.