693754 - Remove JSObject::getFunctionPrivate, add call scope to JSFunction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Function objects created in some call scope use their parent field to indicate the scope chain to use in frames when the function is called.  Since parent is due to be removed, this doesn't work, and the scope should be stored explicitly in the script union of the function (doing so does not increase sizeof(JSFunction)).  This means, however, that every function must actually be a JSFunction, and cannot indirectly point to an owning JSFunction via the private pointer.

With the recent privateData changes, this is also does not change the net size of even a cloned function on 32 bit platforms --- the private data consumes a fixed slot, which bumped cloned functions into a new size class.  With this change, once the rest of object shrinking is done JSFunction will be at 12 words on 32 bit (from 18 words pre-shrinking).  Cutting the reserved slots from JSFunction (they're almost always unused) will drop sizeof(JSFunction) to 8 words.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: patch

https://hg.mozilla.org/projects/jaegermonkey/rev/b9137bf550bd

Comment 2

[Brian Hackett [Laid off!]]

Note: the patch above 

The patch above disables two of the jsdbg2 tests.  There seems to be an assumption in Debugger::wrapFunctionScript that the JSFunction for a script is canonical, and I'm not sure how this should be fixed.  cc'ing jsdbg2 folks.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: followup

Followup to fix several places where the browser depended on funobj->getPrivate() == fun.  The main concern here is a change in the logic in nsScriptSecurityManager::GetFunctionObjectPrincipal, which depended on knowing whether a function object has been cloned from another (information which can no longer be determined).

https://hg.mozilla.org/projects/jaegermonkey/rev/ab14001bab84

Comment 4

[Shu-yu Guo [:shu]]

I'm getting a segfault in fun_trace when visiting gmail -- it looks like fun->script() is NULL sometimes, I'm not sure why.

Comment 5

[Brian Hackett [Laid off!]]

Oh, for measuring you probably want to rebase on m-c tip rather than JM.  I don't think the JM branch is in a usable state right now.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett from comment #5)
> Oh, for measuring you probably want to rebase on m-c tip rather than JM.  I
> don't think the JM branch is in a usable state right now.

I figured that after getting more errors, working off the last MC->JM merge revision now.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: part 2

The first patch removes JSObject::getFunctionPrivate and adds the scope field to JSFunction, but does not actually use the latter.  This adds functionality to set and use the call scope for setting the initial scope chain of function frames.

https://hg.mozilla.org/projects/jaegermonkey/rev/04d4b9920e44

Comment 8

[Luke Wagner [:luke]]

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

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

Hot

::: js/src/jit-test/tests/debug/Script-01.js
@@ +1,5 @@
>  // |jit-test| debug
>  // We get the same Debugger.Script object instance each time we ask.
>  
> +// XXX bug 693754 disabled
> +/*

Can you leave these tests in and failing so that they are not accidentally committed?

::: js/src/methodjit/InvokeHelpers.cpp
@@ +352,5 @@
>       */
>      FrameRegs regs = f.regs;
>  
>      /* Get pointer to new frame/slots, prepare arguments. */
> +    if (!cx->stack.pushInlineFrame(cx, regs, args, *newfun, newfun, newscript, initial, &f.stackLimit))

Can you change the signature of pushInlineFrame to remove the redundant parameter?

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 566545 [details] [diff] [review]
followup

>diff --git a/caps/src/nsScriptSecurityManager.cpp b/caps/src/nsScriptSecurityManager.cpp
>@@ -2235,17 +2235,17 @@ nsScriptSecurityManager::GetFunctionObje
>         // principal we want is in the frame's script, not in the
>         // function's script. The function's script is where the
>         // eval-calling code came from, not where the eval or new
>         // Script object came from, and we want the principal of
>         // the eval function object or new Script object.
> 
>         script = frameScript;
>     }
>-    else if (JS_GetFunctionObject(fun) != obj)
>+    else if (!JS_GetScriptPrincipals(cx, script))
>     {
>         // Here, obj is a cloned function object.  In this case, the
>         // clone's prototype may have been precompiled from brutally

I don't thinks this is valid: I believe the case this is trying to handle is precisely when 'script' has a different principal than 'obj'.  Switching the review over to the caps hitman.

Blake: I would guess that the fix would be to *always* call doGetObjectPrincipal(obj) (after the frameScript != script check) and only if that returns NULL start looking at the script; is there ever a case (other than the eval case caught by the frameScript check) where the script principal is right and obj principal is wrong?

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 566905 [details] [diff] [review]
part 2

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

Cool.  You might consider sending a message to dev-tech-js-engine-internals when this lands to say "beware, callee.parent is now only points to the callee's global, not the closure's scope; use callee.toFunction()->callScope() instead".  On the subject, callScope isn't quite right (it ambiguates the "call object", which is totally different).  Can you rename it to closureScope instead?

::: js/src/jsfun.cpp
@@ +2318,5 @@
>      JSFunction *fun;
>  
>      if (funobj) {
>          JS_ASSERT(funobj->isFunction());
> +        JS_ASSERT(funobj->getParent() == parent);

Preexisting for sure but: this 'funobj' parameter is bizarro and it looks like its only used GlobalObject::initFunctionAndObjectClasses.  Would it make sense to split out a JSFunction::init() that can be called by both initFunctionAndObjectClasses and js_NewFunction (perhaps renamed to JSFunction::create)?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 566545 [details] [diff] [review]
followup

Yeah, that change isn't right. Luke's suggestion of always calling doGetObjectPrincipal would work, but is probably a lot slower than need be. Luke mentioned that there might still be an API to tell us whether or not "this function object is a cloned function object". If so, can we use that instead?

Comment 12

[Jason Orendorff [:jorendorff]]

(In reply to Luke Wagner [:luke] from comment #10)
> On the subject, callScope isn't
> quite right (it ambiguates the "call object", which is totally different). 
> Can you rename it to closureScope instead?

Can we just call it env()?

There's 60 years of history thinking of closures as a code-environment pair. Also "environment" happens to be the term the ES5 spec uses.

Comment 13

[Brian Hackett [Laid off!]]

(In reply to Blake Kaplan (:mrbkap) from comment #11)
> Comment on attachment 566545 [details] [diff] [review] [diff] [details] [review]
> followup
> 
> Yeah, that change isn't right. Luke's suggestion of always calling
> doGetObjectPrincipal would work, but is probably a lot slower than need be.
> Luke mentioned that there might still be an API to tell us whether or not
> "this function object is a cloned function object". If so, can we use that
> instead?

There isn't a way to tell that a function object is cloned anymore; we copy the entire JSFunction when cloning.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: add JSScript::function

The function changes here were causing leaks involving TypeScript --- the TypeScript's function was set to the first JSFunction the script was called with, not the original uncloned function.  That function could hold references on windows / etc. and prevent them from being destroyed.  This patch adds a JSScript::function which holds the original uncloned function for function scripts.  This also allows getting the original uncloned function for an interpreted JSFunction, via fun->script()->function(), and adds an API to restore the original behavior of GetFunctionObjectPrincipal.

Comment 15

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/projects/jaegermonkey/rev/d849045ad3f2

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 568782 [details] [diff] [review]
add JSScript::function

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

Sweet, much nicer.

::: js/src/jsscript.h
@@ +534,5 @@
>      /* array of execution counters for every JSOp in the script, by runmode */
>      JSPCCounters    pcCounters;
>  
> +    /* Function for the script, if it has one. */
> +    JSFunction      *function_;

Can you make this private?

@@ +536,5 @@
>  
> +    /* Function for the script, if it has one. */
> +    JSFunction      *function_;
> +
> +    JSFunction *function() const { return function_; }

Perhaps add a comment saying this is "null, if this is a global or eval script [[right?]]; otherwise the original function for which the script was compiled" or something.

Comment 17

[Andrew McCreight [:mccr8]]

I think this landed as part of ObjShrink, quite some time ago.