791850 - Lazily clone self-hosted methods installed via js_DefineFunction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Till Schneidereit [:till]]

Currently, all functions installed using js_DefineFunction are cloned eagerly. That means that as soon as a standard class is initialized, all its self-hosted methods are cloned.

We should change that by installing a stub function instead that lazily clones the real function and replaces itself with that.

Comment 1

[Till Schneidereit [:till]]

Attachment: v1

This implements just what the bug says. Seems to work on and passes tests on my computer, at least.

A try run (https://tbpl.mozilla.org/?tree=Try&rev=0d2e58bf2930) showed a weird error that I can't yet reproduce. Looking into that right now.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 662335 [details] [diff] [review]
v1

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

Nicely done!  How does this affect the talos regression?

::: js/src/jsapi.cpp
@@ +5002,5 @@
>      return fs->call.op(cx, argc, vp);
>  }
>  
> +JSBool
> +js_self_hosted_method_stub_dispatcher(JSContext *cx, unsigned argc, Value *vp)

Since this symbol isn't used externally, could this be a static function and perhaps named lazy_self_hosted_function (or something with "lazy" in the name)?

@@ +5009,5 @@
> +
> +    RootedFunction stubFun(cx, vp->toObject().toFunction());
> +    JSFunctionSpec *fs = (JSFunctionSpec *)stubFun->getExtendedSlot(0).toPrivate();
> +
> +    RootedObject obj(cx, stubFun->getParent());

This seems to depend on the fact that js_DefineFunction sets the parent of the new function to 'obj' (which it does).  One day we'd like to remove 'parent' (since its almost always cx->global).  Perhaps you could avoid adding a dependency by instead storing/retrieving 'obj' through set/getExtendedSlot(1)?

@@ +5020,5 @@
> +        fun->setJitInfo(fs->call.info);
> +    args.setCallee(ObjectValue(*fun));
> +    if (!InvokeKernel(cx, args))
> +        return JS_FALSE;
> +    return JS_TRUE;

Can you use 'true'/'false' through this function?

@@ +5068,5 @@
>               */
>              fun->setExtendedSlot(0, PrivateValue(fs));
>          }
>  
> +        if (fs->selfHostedName) {

Could you put a comment here saying to the effect of "delaying cloning self-hosted functions until they are called by initially installing functions with a stub that does the cloning on demand".

@@ +5075,5 @@
> +            if (!fun)
> +                return JS_FALSE;
> +            fun->setExtendedSlot(0, PrivateValue(fs));
> +        } else {
> +            fun = js_DefineFunction(cx, obj, id, fs->call.op, fs->nargs, flags, fs->selfHostedName);

Can you s/fs->selfHostedName/NULL/ in this line?

Comment 3

[Till Schneidereit [:till]]

Attachment: v2, carrying r+

(In reply to Luke Wagner [:luke] from comment #2)
> Comment on attachment 662335 [details] [diff] [review]
> v1
> 
> Review of attachment 662335 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nicely done!  How does this affect the talos regression?

It's ... hard to tell. Looking at http://graphs.mozilla.org/graph.html#tests=[[29,63,18]]&sel=none&displayrange=7&datatype=running, I have the feeling that we were edging closer to needing a new chunk per compartment and that that point could be crossed with pretty much any patch that increases a compartments minimum size.

I will test this some more and will only push it once we actually have self-hosted code as we don't need it right now and it's not likely to rot too much.

Comment 4

[Luke Wagner [:luke]]

I just thought of one catch: if the client code writes:

  var f = Array.prototype.forEach;
  [].forEach();
  assertEq(f, [].forEach);

won't that fail with this patch?  (Another symptom would be if the client attached properties to the forEach function object before calling it, they would effectively disappear.)  Fixing this suggests that we'd need to mutate the JSFunction in-place from a native to interpreted function.

Comment 5

[Till Schneidereit [:till]]

Attachment: v3

This version switches out the guts of the JSFunction holding the lazy cloning-native for the cloned script.

It seems to work well, but this try run will tell more: http://tbpl.mozilla.org/?tree=Try&rev=0f9730d7a4d8

Applies on top of https://bugzilla.mozilla.org/attachment.cgi?id=673762

Comment 6

[Mozilla RelEng Bot]

Try run for 0f9730d7a4d8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=0f9730d7a4d8
Results (out of 97 total builds):
    exception: 36
    success: 2
    warnings: 59
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-0f9730d7a4d8

Comment 7

[Till Schneidereit [:till]]

Comment on attachment 674142 [details] [diff] [review]
v3

Yeah, so that didn't quite work. Running tests locally might help ...

Comment 8

[Till Schneidereit [:till]]

Attachment: v4

So, this looks much better: it passes all js- and jit-tests. Pushed to try here:
https://tbpl.mozilla.org/?tree=Try&rev=f35733c1fe09

This uses waldo's idea of doing the gut swapping in js::InvokeKernel (although I also had to add it to JM's UncachedInlineCall).

Additionally, I patched up all places uncovered by the local tests where fun->isInterpreted() was used to guard accesses to fun->script()->someThing. The try run might turn up more of those, though.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 674886 [details] [diff] [review]
v4

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

::: js/src/jsapi.cpp
@@ +5027,3 @@
>          if (fs->selfHostedName) {
> +            fun = js_DefineFunction(cx, obj, id, NULL, fs->nargs, flags | JSFUN_LAZY,
> +                                    NullPtr(), JSFunction::ExtendedFinalizeKind);

Could you replace NullPtr() (which is for handles, not normal pointers) with:
  /* selfHostedName = */NULL

::: js/src/jsapi.h
@@ +2531,5 @@
>  #define JSFUN_LAMBDA            0x08    /* expressed, not declared, function */
>  
> +#define JSFUN_LAZY              0x10    /* interpreted function that gets its
> +                                           real guts lazily on first
> +                                           execution. */

As discussed irl, could JSFUN_LAZY be a separate state from both native and interpreted?

Comment 10

[Till Schneidereit [:till]]

Attachment: v5

This makes lazy functions neither native nor interpreted. Also, it uses one private slot less, as the parent object can be retrieved from fun->environment().

Pushed to try here:
http://tbpl.mozilla.org/?tree=Try&rev=755ad7378a5d

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 675763 [details] [diff] [review]
v5

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

So I was grepping through the codebase for "isInterpreted()" and there are actually a lot more sites that would need to be fixed to include "|| isLazy()".  Rather than doing that (which seems unfortunate), how about a third (and hopefully final) iteration:
 bool isInterpreted();  // could potentially have a JSScript
 bool hasScript();  // you can call script() right now to get the JSScript
 bool isLazy();  // isInterpreted(), but !hasScript()

Thus, the state space breaks down: a script is either interpreted or native; an interpreted script is either lazy or not.  How does that sound to you?  We'll still want to audit all calls to "script()".  Note: anywhere that refers to the script of a function that is running is safe since, well, we're running it :)

::: js/src/jsfun.cpp
@@ +483,5 @@
>  }
>  
> +bool
> +js::InitializeLazyFunctionScript(JSContext *cx, HandleFunction fun)
> +{

Can you JS_ASSERT(fun->isLazy()); ?

@@ +484,5 @@
>  
> +bool
> +js::InitializeLazyFunctionScript(JSContext *cx, HandleFunction fun)
> +{
> +    RootedObject parent(cx, fun->environment());

This shouldn't get mutated, so you can just pass fun->environment() directly to defineGeneric.

@@ +490,5 @@
> +    RootedAtom funName(cx, Atomize(cx, fs->selfHostedName, strlen(fs->selfHostedName)));
> +    if (!funName)
> +        return JS_FALSE;
> +
> +    RootedFunction newFun(cx, cx->runtime->getSelfHostedFunction(cx, funName));

It's a bit of a shame to clone the function+script only to throw away the function.  Could you call js::CloneScript instead?  As is, CloneInterpretedFunction also links the cloned script to 'newFun', which we don't want.

@@ +494,5 @@
> +    RootedFunction newFun(cx, cx->runtime->getSelfHostedFunction(cx, funName));
> +    if (!newFun)
> +        return false;
> +
> +    fun->flags = newFun->flags | JSFUN_EXTENDED;

IIUC, you should be able to write:
  fun->flags &= ~JSFUN_LAZY;

@@ +496,5 @@
> +        return false;
> +
> +    fun->flags = newFun->flags | JSFUN_EXTENDED;
> +    RootedScript newScript(cx, newFun->script());
> +    fun->initScript(newScript);

You'll also need newScript->setFunction(fun).

@@ +501,5 @@
> +
> +    RootedValue funVal(cx, ObjectValue(*fun));
> +    RootedId funId(cx, AtomToId(funName));
> +    if (!JSObject::defineGeneric(cx, parent, funId, funVal, NULL, NULL,
> +                                 fun->flags & ~JSFUN_FLAGS_MASK))

This whole 'flags' business makes me a bit uncomfortable.  I'd much prefer that we explicitly enumerate what flags we're trying to pass on here to avoid unintentional pass-through (e.g., I think there is a bug atm that JSFUN_INTERPRETED will get passed through which has a completely different meaning as a property flag (weak type system...)).  In fact, I'd expect that there was a single flags value we need to pass to defineGeneric for all self-hosted functions.  If so, we can assert this is the only content in jsapi.cpp in JS_DefineFunctions.

@@ +1481,5 @@
>  
>      /* Initialize all function members. */
>      fun->nargs = uint16_t(nargs);
>      fun->flags = flags & (JSFUN_FLAGS_MASK | JSFUN_INTERPRETED);
> +    if (flags & (JSFUN_INTERPRETED | JSFUN_LAZY)) {

Since fun->flags has been set, we can just use
  if (fun->isInterpreted())

Comment 12

[Mozilla RelEng Bot]

Try run for 2c0865ac331c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2c0865ac331c
Results (out of 83 total builds):
    success: 11
    warnings: 54
    failure: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2c0865ac331c

Comment 13

[Mozilla RelEng Bot]

Try run for 2c0865ac331c is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=2c0865ac331c
Results (out of 84 total builds):
    success: 11
    warnings: 54
    failure: 19
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-2c0865ac331c

Comment 14

[Till Schneidereit [:till]]

Attachment: v6

This is a pretty fundamental rewrite of the entire lazy cloning approach.

I tried factoring things in such a way that the parts related to dealing with the selfHostingGlobal and the cloning are in jscntxt.cpp, whereas the JSFunction-related parts reside in jsfun.cpp.

Directly using CloneScript turned out to be much better indeed, if not all that easy to implement.

Also, the whole flags situation is a bit cleaner than in the last patch: laziness isn't expressed via a flag at all. Dealing with the fact that the flags in JSFunctionSpecs are meant to be interpreted as both function flags and property flags is a bit ugly: In js_DefineFunction, I had to add some ugly masking business to make that work. (As can be seen by the try run from Saturday.)


try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=c47f46026da6

Comment 15

[Till Schneidereit [:till]]

Comment on attachment 680445 [details] [diff] [review]
v6

*Sigh*. Looks like I didn't fully work out the flags intricacies after all. Looking into it.

Comment 16

[Mozilla RelEng Bot]

Try run for c47f46026da6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c47f46026da6
Results (out of 307 total builds):
    exception: 4
    success: 203
    warnings: 100
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c47f46026da6

Comment 17

[Till Schneidereit [:till]]

Attachment: v7

Try-servering again here: http://tbpl.mozilla.org/?tree=Try&rev=42fe9480eab2

See comment

I wonder whether we should just move all JSFunction flags up by one (i.e., INTERPRETED to 0x0002, etc.) and not use 0x0001 at all. That flag is only ever used to mean ITERABLE from the JSAPI and removing the double meaning would make things somewhat cleaner.

Comment 18

[Luke Wagner [:luke]]

Comment on attachment 680478 [details] [diff] [review]
v7

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

Looking good.  Not done yet, but quick nits/questions:

::: js/src/jsapi.cpp
@@ +4975,5 @@
>          if (!atom)
>              return JS_FALSE;
>  
>          Rooted<jsid> id(cx, AtomToId(atom));
> +        JSFunction *fun;

I think it is best to push declarations in as much as they can go (which means pushing this down into the 3 below).

::: js/src/jsfun.cpp
@@ +90,5 @@
>       * below walk to only check each explicit frame rather than needing to
>       * check any calls that were inlined.
>       */
>      if (fun->isInterpreted()) {
> +        JS_ASSERT(fun->hasScript());

Instead of asserting this here, could you assert in JSFunction::script() ?

@@ +399,5 @@
>                                       name);
>              }
>              return false;
>          }
> +        JS_ASSERT(fun->hasScript());

This wouldn't be needed with the abovementioned script() assert.

@@ +1511,5 @@
>  
>      clone->nargs = fun->nargs;
>      clone->flags = fun->flags & ~JSFunction::EXTENDED;
>      if (fun->isInterpreted()) {
> +        JS_ASSERT(fun->hasScript());

ditto

@@ +1598,5 @@
>      }
>  
> +    JSFunction::Flags funFlags;
> +    if (!native) {
> +        flags &= ~JSFunction::INTERPRETED;

Could you just not pass JSFunction::INTERPRETED in the first place?  Ideally, after my patch, we'd never mix JSFunction::Flags with the JSFunctionSpec::flags (which is what 'flags' is here).

Comment 19

[Mozilla RelEng Bot]

Try run for 42fe9480eab2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=42fe9480eab2
Results (out of 305 total builds):
    exception: 4
    success: 280
    warnings: 20
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-42fe9480eab2

Comment 20

[Till Schneidereit [:till]]

Comment on attachment 680478 [details] [diff] [review]
v7

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

::: js/src/jsapi.cpp
@@ +4975,5 @@
>          if (!atom)
>              return JS_FALSE;
>  
>          Rooted<jsid> id(cx, AtomToId(atom));
> +        JSFunction *fun;

Will do.

::: js/src/jsfun.cpp
@@ +90,5 @@
>       * below walk to only check each explicit frame rather than needing to
>       * check any calls that were inlined.
>       */
>      if (fun->isInterpreted()) {
> +        JS_ASSERT(fun->hasScript());

Oh, yes, that makes sense.

@@ +1598,5 @@
>      }
>  
> +    JSFunction::Flags funFlags;
> +    if (!native) {
> +        flags &= ~JSFunction::INTERPRETED;

I tried doing that, but unfortunately JSFunction::INTERPRETED has the same value as JSPROP_ENUMERATE, which is given to the JSObject::defineGeneric call below. Hence my musings about moving the JSFunction::Flags in comment 17.

Comment 21

[Luke Wagner [:luke]]

(In reply to Till Schneidereit [:till] from comment #20)
> I tried doing that, but unfortunately JSFunction::INTERPRETED has the same
> value as JSPROP_ENUMERATE, which is given to the JSObject::defineGeneric
> call below. Hence my musings about moving the JSFunction::Flags in comment
> 17.

I'm afraid I don't understand: the flags passed to js_DefineFunction are the jsapi.h flags, not the JSFunction::Flags.  Noone should be passing JSFunction::INTERPRETED to js_DefineFunction in the first place.

Comment 22

[Till Schneidereit [:till]]

(In reply to Luke Wagner [:luke] from comment #21)
> (In reply to Till Schneidereit [:till] from comment #20)
> > I tried doing that, but unfortunately JSFunction::INTERPRETED has the same
> > value as JSPROP_ENUMERATE, which is given to the JSObject::defineGeneric
> > call below. Hence my musings about moving the JSFunction::Flags in comment
> > 17.
> 
> I'm afraid I don't understand: the flags passed to js_DefineFunction are the
> jsapi.h flags, not the JSFunction::Flags.  Noone should be passing
> JSFunction::INTERPRETED to js_DefineFunction in the first place.

I agree: Noone should. It happens a lot, though, as many places in Gecko define JSFunctionSpecs with the 0x1 bit set to make the installed method enumerable. That's then used in the call to JSObject::defineGeneric in js_DefineFunction.

And yes: this is very confusing and annoying!

Comment 23

[Luke Wagner [:luke]]

I still don't understand; it's fine if 0x1 is set in the JSFunctionSpec::flags, they filter through JSAPIToJSFunctionFlags before getting passed to js_NewFunction.

Comment 24

[Till Schneidereit [:till]]

(In reply to Luke Wagner [:luke] from comment #23)
> I still don't understand; it's fine if 0x1 is set in the
> JSFunctionSpec::flags, they filter through JSAPIToJSFunctionFlags before
> getting passed to js_NewFunction.

Yeah, but JSAPIToJSFunctionFlags can only return JSFunction::NATIVE_CTOR or JSFunction::NATIVE_FUN. I had a version that checked for JSFunction::INTERPRETED, but that seemed uglier than doing the !native test in js_DefineFunction itself. And that test has to be done to properly set the flag:

If (native && JSFunction::INTERPRETED), then the JSFunction::INTERPRETED *really* means !JSFunction::INTERPRETED, and JSOP_ENUMERATE

whereas

if (!native && JSFunction::INTERPRETED), then it means what it says, and !JSOP_ENUMERATE ...

Also, without changing something about the flags situation, we can't install enumerable self-hosted methods.

Fun!

Comment 25

[Mozilla RelEng Bot]

Try run for c47f46026da6 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=c47f46026da6
Results (out of 308 total builds):
    exception: 4
    success: 203
    warnings: 100
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-c47f46026da6

Comment 26

[Mozilla RelEng Bot]

Try run for 42fe9480eab2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=42fe9480eab2
Results (out of 306 total builds):
    exception: 4
    success: 280
    warnings: 20
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-42fe9480eab2

Comment 27

[Luke Wagner [:luke]]

I still don't see why JS_DefineFunctions needs to pass JSFunction::INTERPRETED to js_DefineFunction, which immediately masks it off.  If someone passes JSPROP_ENUMERATE, that's fine: it won't confuse the funFlags, and it will properly propagate to defineProperty.  It seems like 'native' or '!native' tells us everything we need to know.

Comment 28

[Till Schneidereit [:till]]

(In reply to Luke Wagner [:luke] from comment #27)
> I still don't see why JS_DefineFunctions needs to pass
> JSFunction::INTERPRETED to js_DefineFunction, which immediately masks it
> off.  If someone passes JSPROP_ENUMERATE, that's fine: it won't confuse the
> funFlags, and it will properly propagate to defineProperty.  It seems like
> 'native' or '!native' tells us everything we need to know.

I'm obviously failing hard at explaining the problem. Let me try it once again:

js_DefineFunction uses the flags for two different things:

1: to determine the defined function's type. You're right: JSFunction::INTERPRETED isn't needed for that at all and properly masked off.

2: to pass them on to JSObject::defineGeneric:
http://mxr.mozilla.org/mozilla-central/source/js/src/jsfun.cpp#1604
This usage needs the JSFunction::INTERPRETED flag, because it interprets it as JSPROP_ENUMERATE.


I hope that explains it better than my last attempts. If not, we should talk in #jsapi.

In case it is clear: It seems like js_DefineFunction is doing more than it should. Maybe what we should really do is remove the JSObject::defineGeneric call from js_DefineFunction and move it to the call-sites instead. AFAICT, there's three of those:
- JS_DefineFunctions (in jsapi)
- JS_DefineFunctionsWithHelp (in jsfriendapi)
- DefineFunctionWithReserved (also in jsfriendapi)

Comment 29

[Luke Wagner [:luke]]

Comment on attachment 680478 [details] [diff] [review]
v7

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

Looks great.  I think we'll be ready to land with these addressed and the change we discussed on IRC regarding JSFunction::INTERPRETED.

::: js/src/jscntxt.cpp
@@ +396,5 @@
>  {
> +    RootedValue funVal(cx);
> +    if (!getUnclonedSelfHostedValue(cx, name, &funVal))
> +        return false;
> +    JS_ASSERT(funVal.isObject() && funVal.toObject().isFunction());

The next statement will assert this condition (in the to* helpers) so I think it could be removed here.

@@ +399,5 @@
> +        return false;
> +    JS_ASSERT(funVal.isObject() && funVal.toObject().isFunction());
> +
> +    Rooted<JSScript*> sourceScript(cx, funVal.toObject().toFunction()->script());
> +    Rooted<JSObject*> scope(cx, sourceScript->enclosingStaticScope());

In the case of cross-compartment cloning, the 'enclosingScope' needs to be in the *destination* compartment.  Fortunately, for top-level scripts, this is just NullPtr().  I know js_CloneFunctionObject looks like it is passing the source's enclosingScope, but see the immediately-preceding assert that, if this is cross-compartment, the enclosingScope is null.  Can you JS_ASSERT(!sourceScript->enclosingStaticScope()) and then pass NullPtr here?  (This would only fail if we try to clone a nested function (or a top-level function that is inside a global let statement.)

Comment 30

[Till Schneidereit [:till]]

Attachment: V8

Addresses all feedback and adds some more asserts/ guards against invalid uses of JSFunction::script().

I'd be grateful for a close look at the latter of these changes: While I'm fairly confident that they're all correct, I'm far from completely sure about it.

Comment 31

[Till Schneidereit [:till]]

Oh, and pushed to try again because of the assert in JSFunction::script() here:
http://tbpl.mozilla.org/?tree=Try&rev=f7e7be126994

Comment 32

[Luke Wagner [:luke]]

Comment on attachment 680852 [details] [diff] [review]
V8

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

::: js/src/jsapi.cpp
@@ +5015,5 @@
>  
> +        /*
> +         * Delay cloning self-hosted functions until they are called by
> +         * setting a flag that instructs js::InvokeKernel to do the cloning
> +         * on demand.

This comment is now a bit out of date.  Perhaps "Delay cloning self-hosted functions until they are called. This is achieved by passing js_DefineFunction a NULL JSNative which produces an interpreted JSFunction where !hasScript. Interpreted call paths then call InitializeLazyFunctionScript if !hasScript."

@@ +5021,2 @@
>          if (fs->selfHostedName) {
> +            JSFunction *fun = js_DefineFunction(cx, obj, id, NULL, fs->nargs, 0,

Could you prefix NULL with /* native = */ ?

::: js/src/jsinterpinlines.h
@@ +1019,5 @@
>  
>      void initFunction(const Value &fval) {
>          if (fval.isObject() && fval.toObject().isFunction()) {
>              JSFunction *fun = fval.toObject().toFunction();
> +            if (fun->hasScript()) {

Hm, for this one I think we should keep it "if fun->isInterpreted" and, inside the branch, do "if (!fun->hasScript()) InitializeLazyFunctionScript(...)" since we are effectively about to call 'fun'.

::: js/src/jsscript.cpp
@@ +1853,5 @@
>      RawObject enclosing = enclosingScope_;
>      while (enclosing) {
>          if (enclosing->isFunction()) {
>              RawFunction fun = enclosing->toFunction();
> +            if (!fun->hasScript())

Nice.  You can also remove the 'nogc' up top.

Comment 33

[Luke Wagner [:luke]]

Great work, by the way!  Does this mean we can turn on some self-hosted functions w/o hurting memory now?

Comment 34

[Mozilla RelEng Bot]

Try run for f7e7be126994 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f7e7be126994
Results (out of 111 total builds):
    exception: 4
    success: 97
    warnings: 9
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f7e7be126994

Comment 35

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/862f9cd7eb0b

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/862f9cd7eb0b

Comment 37

[Phil Ringnalda (:philor)]

Sorry, backed out in https://hg.mozilla.org/mozilla-central/rev/dd68409d7810 - I wanted to give you a few days to deal with the bug 811625 way that these broke the Jetpack tests, but it turns out that they broke the Jetpack SDK rather than just its tests, turning it into the Crash All The Time SDK.

Comment 38

[Shu-yu Guo [:shu]]

Attachment: supplemental patch

This patch is on top of the main patch.

It's not enough to just lazily clone the script. Flags also need to be lazily synched, else we push the wrong frames, e.g. when we need a CallObject.

Removed the *Script suffixes since we need to do more than just clone the script and renamed Rooted<T *> to already-provided typedefs.

Comment 39

[Shu-yu Guo [:shu]]

Attachment: supplemental patch

Oops, used the wrong comment style for the part of the engine this patch is in.

Comment 40

[Till Schneidereit [:till]]

Attachment: v9, wip

Fixes most issues revealed by fuzzing and incorporates Shu's supplemental patch. Still choking on proxies, though.

Comment 41

[Shu-yu Guo [:shu]]

Attachment: Decompile patch

Patch DecompileValueGenerator and expose a new (very unsafe!) intrinsic to decompile values in the nearest non-builtin frame. For printing useful errors when throwing instead of just "callbackfn".

Also fixes some crashes.

Comment 42

[Till Schneidereit [:till]]

(In reply to Shu-yu Guo [:shu] from comment #41)
> Created attachment 681841 [details] [diff] [review]
> Decompile patch
> 
> Patch DecompileValueGenerator and expose a new (very unsafe!) intrinsic to
> decompile values in the nearest non-builtin frame. For printing useful
> errors when throwing instead of just "callbackfn".
> 
> Also fixes some crashes.

An earlier version of intrinsic_ThrowError took index positions of the self-hosted function's arguments and decompiled those directly. It seems to me like that would still be preferable to having to call %_DecompileNonBuiltinFrameValue. Note that the value itself isn't needed when calling DecompileValueGenerator with an actual spIndex instead of JSDVG_*_STACK.

Comment 43

[Shu-yu Guo [:shu]]

Attachment: Decompile patch v2

Per discussion with till, changing to %_DecompileArg.

Luke, what do you think about this approach?

Comment 44

[Luke Wagner [:luke]]

Comment on attachment 682120 [details] [diff] [review]
Decompile patch v2

Nice solution.  It *almost* seems right except for the fact that the argument index passed to %_DecompileArg is interpreted as the distance from 'sp'.  sp will point past the end of the last actual argument which means that %_DecompileArg(0), which *means* to refer to the first formal parameter, will point to either the last actual argument or, if 0 arguments were passed, some random Value (possibly not even a Value if the stack is empty, the last (uninitialized!) uint64_t of the StackFrame.

So that's bad, but not unfixable.  Rather than trying to hack this by solving for the right spindex, I think it would be cleaner to add a new function DecompileSelfHostedArgument that mostly cloned DecompileExpressionFromStack, but:
 - instead of spindex took a formalIndex.
 - didn't call FindStartPC but instead just inlined the small relevant portion (creating the PCStack, deriving the pc (if any) that produced the formal argument)

Comment 45

[Shu-yu Guo [:shu]]

Attachment: Decompile patch v3

Refactored according to luke's righteous suggestions.

Comment 46

[Till Schneidereit [:till]]

(In reply to Shu-yu Guo [:shu] from comment #45)
> Created attachment 682673 [details] [diff] [review]
> Decompile patch v3
> 

Unfortunately, this causes the wrong value to be decompiled in some cases.

STR:
1. apply the patch from bug 784294
2. apply this patch
3. execute "./js -e "[0,0].sort(Array.some)"

Expected result:
the error "TypeError: 0 is not a function"

Actual result:
the error "TypeError: Array.some is not a function"

Comment 47

[Till Schneidereit [:till]]

Attachment: v10, wip

Addresses all feedback and integrates the patch from bug 811584.

WIP, because it causes a crash somewhere in TI with --always-mjit.

STR:
1. apply the patch from bug 784294
2. apply this patch
3. execute ./js --always-mjit -e "[0,0].sort(Array.some)"

Expected result:
The error "-e:1:0 TypeError: 0 is not a function"

Actual result:
NPE in js::gc::Cell::compartment() (Heap.h:989)

Comment 48

[Shu-yu Guo [:shu]]

Attachment: Decompile patch v4

(In reply to Till Schneidereit [:till] from comment #46)
> (In reply to Shu-yu Guo [:shu] from comment #45)
> > Created attachment 682673 [details] [diff] [review]
> > Decompile patch v3
> > 
> 
> Unfortunately, this causes the wrong value to be decompiled in some cases.
> 
> STR:
> 1. apply the patch from bug 784294
> 2. apply this patch
> 3. execute "./js -e "[0,0].sort(Array.some)"
> 
> Expected result:
> the error "TypeError: 0 is not a function"
> 
> Actual result:
> the error "TypeError: Array.some is not a function"

This is really unfortunate, because the call stack is: Array.sort -> Array.some -> Array.prototype.some

Because both Array.some and Array.prototype.some are builtin, DecompileArg(0) obviously gives you the wrong error. It'll be harder to teach the provenance of the argument to be decompiled. That is, in this case you'd have to know that Array.prototype.some's 0th formal is actually Array.some's 1st formal. We can't query frame slot values other than for the topmost frame, so we're screwed trying to do something smarter.

This basically means we need to report errors as early as possible in the builtin code, with a fallback on converting the value to source if the argument cannot be decompiled.

A new patch is attached that does this. We need to check if callbackfn is not callable in the frame that we want to report the error.

Comment 49

[Shu-yu Guo [:shu]]

(In reply to Till Schneidereit [:till] from comment #47)
> Created attachment 682937 [details] [diff] [review]
> v10, wip
> 
> Addresses all feedback and integrates the patch from bug 811584.
> 
> WIP, because it causes a crash somewhere in TI with --always-mjit.
> 
> STR:
> 1. apply the patch from bug 784294
> 2. apply this patch
> 3. execute ./js --always-mjit -e "[0,0].sort(Array.some)"
> 
> Expected result:
> The error "-e:1:0 TypeError: 0 is not a function"
> 
> Actual result:
> NPE in js::gc::Cell::compartment() (Heap.h:989)

This patch fixes:

-        RootedObject clone(cx);
-        if (fun->isNative())
-            clone = CloneFunctionObject(cx, fun, cx->global(), fun->getAllocKind());
-        else
-            clone = CloneInterpretedFunction(cx, NullPtr(), fun);
+        RootedObject clone(cx, CloneFunctionObject(cx, fun, cx->global(), fun->getAllocKind()));

Though I imagine you're calling |CloneInterpretedFunction| for a reason. Are you explicitly trying to ensure that the script also gets cloned? This happens anyways because cloning intrinsic values is cross-compartmental.

This function is infuriatingly named -- it should only be used for cloning parentless lambdas that gets a parent when JSOP_LAMBDA is executed. So by using |CloneInterpretedFunction|, the clone's |environment()| is NULL. This caused the scopeChain assert you saw, as scopeChain is assigned from environment() and is now NULL.

Comment 50

[Shu-yu Guo [:shu]]

Attachment: Decompile patch v5

5th try, hack around frameless natives for |"".replace(RegExp(), Array.reduce)|

Comment 51

[Till Schneidereit [:till]]

Comment on attachment 683062 [details] [diff] [review]
Decompile patch v5

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

This correctly decompiles all cases I can come up with.

::: js/src/vm/Stack.cpp
@@ +1301,5 @@
>  StackIter::settleOnNewState()
>  {
>      AutoAssertNoGC nogc;
>  
> +    /* Reset whether or we popped a call last time we settled. */

Nit: slightly broken comment.

Comment 52

[Till Schneidereit [:till]]

Attachment: v11

Addresses review comments and includes some tests for function invocation, arg handling and value decompilation.

Try-servering here: https://tbpl.mozilla.org/?tree=Try&rev=da0f578c7819

Comment 53

[Till Schneidereit [:till]]

Oh, and the try-run has quite a few other patches applied, too:
- shu's latest decompilation patch from this bug
- both patches from bug 812906

And it integrates the patch from bug 811584, as proposed there.

Comment 54

[Till Schneidereit [:till]]

Comment on attachment 683170 [details] [diff] [review]
v11

As just discussed on IRC.

Comment 55

[Till Schneidereit [:till]]

Attachment: Combined patch for fuzzing

Can you please do some fuzzing on this combined patch? It's the two patches in this bug, plus the one from 784294 and the two from 812906 combined.

Thanks!

Comment 56

[Jeff Walden [:Waldo]]

Comment on attachment 683062 [details] [diff] [review]
Decompile patch v5

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

The ownership model around what DecompileArgumentFromStack returns is a bit confused, I think, which is worrisome enough I'd like to review a new version of this patch (interdiff will do fine) before saying it's good.

::: js/src/jscntxt.cpp
@@ +297,5 @@
>  
>      char *errorArgs[3] = {NULL, NULL, NULL};
>      for (unsigned i = 1; i < 4 && i < args.length(); i++) {
>          RootedValue val(cx, args[i]);
> +        if (val.isInt32() || val.isString())

ToString(Int32Value(...)) can fail, so you should test for isInt32 and if so assign ToString(val) to val, then go on to the rest of this.

@@ +325,5 @@
> +    JS_ASSERT(args.length() == 2);
> +
> +    RootedValue value(cx, args[1]);
> +    char *str = DecompileArgument(cx, args[0].toInt32(), value);
> +    if (str) {

Er...

Shouldn't this return false if !str?  That's an error case.

@@ +326,5 @@
> +
> +    RootedValue value(cx, args[1]);
> +    char *str = DecompileArgument(cx, args[0].toInt32(), value);
> +    if (str) {
> +        args.rval().setString(Atomize(cx, str, strlen(str)));

Separate out the Atomize call here; it can fail, and setString(NULL) is going to complain.

You really don't need atomization overhead here at all, as far as I can see, but this probably is the most convenient API, so long as decompilation is phrased in terms of const char* rather than const jschar*.  (We really should switch this at some point.)  So I guess atomizing needlessly is okay for now.

::: js/src/jsopcode.cpp
@@ +6282,5 @@
> +    JS_ASSERT(formalIndex >= 0);
> +
> +#ifdef JS_MORE_DETERMINISTIC
> +    /* See note in DecompileExpressionFromStack. */
> +    return NULL;

That reads like a failure case to me.  Except it's not, it's just trying to always get identical output.  Can you make this return |JS_strdup(cx, "")| and have the function return const char*?

@@ +6293,5 @@
> +    StackIter frameIter(cx);
> +    while (!frameIter.done() && !frameIter.isScript())
> +        ++frameIter;
> +    if (frameIter.done())
> +        return NULL;

Is this case even possible?  I would think we could just assert that this can't happen.  Please explain this to me!

@@ +6307,5 @@
> +     * script but we popped a call frame during the last bump, assume that we
> +     * just came from a frameless native and bail conservatively.
> +     */
> +    if (frameIter.done() || frameIter.poppedCallDuringSettle() || !frameIter.isScript())
> +        return NULL;

JS_strdup(cx, "")

@@ +6318,5 @@
> +
> +    JS_ASSERT(script->code <= current && current < script->code + script->length);
> +
> +    if (current < script->main())
> +        return NULL;

JS_strdup(cx, "")

@@ +6325,5 @@
> +    if (!pcStack.init(cx, script, current))
> +        return NULL;
> +
> +    if (formalIndex + 2 >= pcStack.depth())
> +        return NULL;

JS_strdup(cx, "")

@@ +6359,5 @@
> +    if (!fallback)
> +        return NULL;
> +
> +    size_t length = fallback->length();
> +    const jschar *chars = fallback->getChars(cx);

You need to have a StableString to do this stuff safely.  Please ensureStable one into a Rooted<JSStableString*> and only then call these here.

Comment 57

[Christian Holler (:decoder)]

Should I wait with the fuzzing for a new patch? :)

Comment 58

[Jeff Walden [:Waldo]]

For that patch at least, I think all my comments should only manifest as issues in edge cases, so you can fuzz that one, at least.

Comment 59

[Shu-yu Guo [:shu]]

Attachment: Decompile patch v6

Applied Waldo's comments, all of which were good.

The |JS_strdup(cx, "")| bits were not applied, as after separating out the error case from the not-going-to-decompile state, we will generate a fallback string using |js_ValueToSource| if |DecompileArgumentFromStack| returns true while |result = NULL|.

Comment 60

[Shu-yu Guo [:shu]]

Attachment: Decompile patch v6

Oops, had a JSAtom * instead of RootedAtom.

Comment 61

[Jeff Walden [:Waldo]]

Comment on attachment 683170 [details] [diff] [review]
v11

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

::: js/src/ion/IonMacroAssembler.h
@@ +274,5 @@
>      void PopRegsInMaskIgnore(RegisterSet set, RegisterSet ignore);
>  
>      void branchTestValueTruthy(const ValueOperand &value, Label *ifTrue, FloatRegister fr);
>  
> +    void branchIfFunctionDoesNotHaveScript(Register fun, Label *label) {

I might name this branchIfFunctionHasNoScript for slightly more brevity, myself.

::: js/src/jscntxt.cpp
@@ +433,5 @@
> +    targetFun->flags = sourceFun->flags | JSFunction::EXTENDED;
> +    if (!JSFunction::setTypeForScriptedFunction(cx, targetFun))
> +        return false;
> +
> +    RootedValue targetFunVal(cx, OBJECT_TO_JSVAL(targetFun));

ObjectValue(*targetFun)

::: js/src/jsfun.cpp
@@ +481,5 @@
> +    Rooted<PropertyName *> funName(cx, funAtom->asPropertyName());
> +    if (!cx->runtime->cloneSelfHostedFunctionScript(cx, funName, fun))
> +    {
> +        return false;
> +    }

No braces needed here.

@@ -1567,5 @@
> -    /*
> -     * To support specifying both native and self-hosted functions using
> -     * JSFunctionSpec, js_DefineFunction can be invoked with either native
> -     * or selfHostedName set. It is assumed that selfHostedName is set if
> -     * native isn't.

A nice touch, making the function name lazy as well here.

::: js/src/jsfun.h
@@ +148,5 @@
>          JS_ASSERT(!isExprClosure());
>          flags |= EXPR_CLOSURE;
>      }
>  
> +    void setIsLazy(bool value) {

This is only ever called with value = false, so this method should be changed to markNotLazy().  If you're thinking into the future when we might want to discard scripts from stuff, I'd kind of rather see that be inlined into a hypothetical JSFunction::discardScript() method that might exist then -- or at least have two methods for this rather than one overloaded on a boolean parameter.

Comment 62

[Jeff Walden [:Waldo]]

Comment on attachment 683387 [details] [diff] [review]
Decompile patch v6

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

::: js/src/jscntxt.cpp
@@ +310,2 @@
>          }
> +        JS_ASSERT(errorArgs[i - 1]);

Erm, I must have missed this before.  We just assume decompilation won't hit errors here?  Ugh.  I think we can return false here if !errorArgs[i - 1] to solve this.  Right?

@@ +330,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    JS_ASSERT(args.length() == 2);
> +
> +    RootedValue value(cx, args[1]);
> +    char *str = DecompileArgument(cx, args[0].toInt32(), value);

I think you can use js::ScopedFreePtr<char> from js/Utility.h and eliminate all the js_frees in this function.

::: js/src/jsopcode.cpp
@@ +6269,5 @@
>          if (!fallback)
>              return NULL;
>      }
> +
> +    Rooted<JSStableString *> stable(cx, fallback->ensureStable(cx));

if (!stable)
    return NULL;

@@ +6273,5 @@
> +    Rooted<JSStableString *> stable(cx, fallback->ensureStable(cx));
> +    size_t length = stable->length();
> +    const jschar *chars = stable->chars().get();
> +    if (!chars)
> +        return NULL;

You don't need to null-check here; once you have a stable string, its chars are good -- as you can tell by it being chars() you're calling, not getChars().  (A "get" prefix implies fallibility in SpiderMonkey, usually.)

@@ +6274,5 @@
> +    size_t length = stable->length();
> +    const jschar *chars = stable->chars().get();
> +    if (!chars)
> +        return NULL;
> +    return DeflateString(cx, chars, length);

At that point you could even pass stable->length()/chars.get() directly in here, and avoid the temporaries.

@@ +6346,5 @@
> +    AssertCanGC();
> +    {
> +        char *result;
> +        if (!DecompileArgumentFromStack(cx, formalIndex, &result))
> +            return NULL;

Changing DecompileArgumentFromStack to return a bool success code and put the string in an outparam isn't quite what I was asking for, but it works just as well.  :-)

@@ +6359,5 @@
> +    RootedString fallback(cx, js_ValueToSource(cx, v));
> +    if (!fallback)
> +        return NULL;
> +
> +    Rooted<JSStableString *> stable(cx, fallback->ensureStable(cx));

if (!stable)
    return NULL;

@@ +6366,3 @@
>      if (!chars)
>          return NULL;
>      return DeflateString(cx, chars, length);

Don't need if (!chars), could pass these directly without having temporaries.

Comment 63

[Shu-yu Guo [:shu]]

Attachment: Decompile patch final (fold this)

The decompile patch with Waldo's comments applied.

Till, please fold this into the main patch.

Comment 64

[Christian Holler (:decoder)]

Comment on attachment 683344 [details] [diff] [review]
Combined patch for fuzzing

Using the combined patch with this code


actual = 0;
actual += Array.lastIndexOf([2, 3,, 4, 5, 6]);
actual += [2, 3,, 4, 5, 6].lastIndexOf();
actual += Array.lastIndexOf([2, 3,, 4, 5, 6], undefined);


yields Assertion failure: types->hasType(types::GetValueType(cx, vp)), at js/src/ion/IonBuilder.cpp:4781

(Run with --ion-eager).

Comment 65

[Shu-yu Guo [:shu]]

(In reply to Christian Holler (:decoder) from comment #64)
> Comment on attachment 683344 [details] [diff] [review]
> Combined patch for fuzzing
> 
> Using the combined patch with this code
> 
> 
> actual = 0;
> actual += Array.lastIndexOf([2, 3,, 4, 5, 6]);
> actual += [2, 3,, 4, 5, 6].lastIndexOf();
> actual += Array.lastIndexOf([2, 3,, 4, 5, 6], undefined);
> 
> 
> yields Assertion failure: types->hasType(types::GetValueType(cx, vp)), at
> js/src/ion/IonBuilder.cpp:4781
> 
> (Run with --ion-eager).

This is caused by |JSRuntime::cloneSelfHostedFunctionScript| overwriting an already-saved entry for 'ArrayLastIndexOf' in the intrinsics holder.

What happens is that on line 2, |ArrayStaticLastIndexOf| calls |ArrayLastIndexOf|, causing it to be cloned into the intrinsics holder. On line 3, calling |.lastIndexOf()| lazily clones the |ArrayLastIndexOf| script into itself *and* overwrites the already existing clone of |ArrayLastIndexOf| in the intrinsic holder with itself.

Till, I suggest to not overwrite if an entry already exists in the holder in |JSRuntime::cloneSelfHostedFunctionScript|, so as to maintain the TI invariant that once cloned, the JSOP_INTRINSICNAME shouldn't ever get a new type (it's type-stable).

The downside of this is that we can might get 2 clones of every intrinsic if it's always called first from within builtin code, then from the outside via a lazy function. But we can't share scripts when this happens anyways, as we can't have a 1-to-many relationship from scripts to functions.

Comment 66

[Shu-yu Guo [:shu]]

s/can might/might

Comment 67

[Till Schneidereit [:till]]

Attachment: Combined patch for fuzzing, v2

Thanks for the last round of fuzzing! Can I have another one, please?

Comment 68

[Till Schneidereit [:till]]

Attachment: v12

@Waldo: This is pretty much what I posted as a pastebin earlier today, plus the changes you requested. A quick interdiff look should be enough, or I can carry the r+. (Ignore the jsinfer.cpp changes)

@Bhackett: As Shu wrote earlier, we had to add some de-lazification of interpreted functions to TI. I found some more sites where that is required during test. Can you take a look at jsinfer.cpp and see if that looks good to you?


thanks!

Comment 69

[Jeff Walden [:Waldo]]

Comment on attachment 683832 [details] [diff] [review]
v12

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

::: js/src/jsfun.cpp
@@ +479,5 @@
> +    if (!funAtom)
> +        return false;
> +    Rooted<PropertyName *> funName(cx, funAtom->asPropertyName());
> +    if (!cx->runtime->cloneSelfHostedFunctionScript(cx, funName, fun))
> +        return false;

Did you remove the markNotLazy call here on purpose?  Looking like no, I think.  (If it was intentional, just return cx->runtime->cloneSelfBlahblahblah(...).

Comment 70

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 683829 [details] [diff] [review]
Combined patch for fuzzing, v2

Just a quick headsup - I'm on PTO for the next week, may not be able to get to this till then.

Comment 71

[Shu-yu Guo [:shu]]

Last minute fix to decompile (shouldn't affect fuzzing). I was computing the formal index wrong since I misunderstood how PCStack works.

> --- a/js/src/jsopcode.cpp
> +++ b/js/src/jsopcode.cpp
> @@ -6321,23 +6321,24 @@ DecompileArgumentFromStack(JSContext *cx
>  
>      if (current < script->main())
>          return true;
>  
>      PCStack pcStack;
>      if (!pcStack.init(cx, script, current))
>          return false;
>  
> -    if (formalIndex + 2 >= pcStack.depth())
> +    uint32_t formalStackIndex = pcStack.depth() - GET_ARGC(current) + formalIndex;
> +    if (formalStackIndex >= pcStack.depth())
>          return true;
>  
>      ExpressionDecompiler ed(cx, script, fun);
>      if (!ed.init())
>          return false;
> -    if (!ed.decompilePC(pcStack[formalIndex + 2]))
> +    if (!ed.decompilePC(pcStack[formalStackIndex]))
>          return false;
>  
>      return ed.getOutput(res);
>  }
>  
>  char *
>  js::DecompileArgument(JSContext *cx, int formalIndex, HandleValue v)
>  {

Comment 72

[Brian Hackett [Laid off!]]

Comment on attachment 683832 [details] [diff] [review]
v12

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

::: js/src/jsinfer.cpp
@@ +1323,5 @@
> +    if (callee->isInterpretedLazy()) {
> +        RootedFunction fun(cx, callee);
> +        if (!InitializeLazyFunctionScript(cx, fun))
> +            return;
> +    }

Can you roll this in to a new fun->getOrCreateScript(cx) that does these checks for you?  Having to sprinkle this chunk of code in so many places is pretty unfortunate.  A second reason is that you can add a MaybeCheckStackRoots call to getOrCreateScript --- this will help the rooting analysis, as this logic is stuff that will very rarely trip and can trigger a GC only when it does.

I don't know if you have added this logic in all the right places.  Broadly, I think that this patch makes getting-a-script-from-a-function much more error prone, in ways both subtle and not.  Calling isInterpreted() and then script() can segv, and calling maybeScript() and using that to distinguish interpreted vs. native will not work in more subtle ways.

The current interface is:

script()
maybeScript()

With these changes, I think a better one is:

JSScript* getOrCreateScript(cx) --- see above
bool maybeGetOrCreateScript(cx, JSScript**) --- as for getOrCreateScript, but succeeds with a NULL script on native functions.
nonLazyScript() --- as existing script()
maybeNonLazyScript() --- as existing maybeScript()

The latter two should almost never be used, with names and comments to indicate clearly their footgun nature and the circumstances when they can be safely used (i.e. as luke points out, when referring to an actively running function).

This is cleanup, and I don't want to hold up this bug, so r+.  But I'd really appreciate it if you made these changes as a followup.

Comment 73

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ef490fef5ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5cbd2b5c5e9

I'll attach a follow-up with bhackett's suggested interface changes soon.

Also, I didn't yet push the self-hosted Array extras, as I want to wait for fuzzing results with that. All the preparatory patches should be safe in that regard.

Comment 74

[Till Schneidereit [:till]]

Attachment: Cleanup of lazy script handling

This patch contains the interface cleanup proposed in comment 72 and applies on top of the other patches.

Although pretty large, the patch should be safe, as almost all of it is just renaming of function calls.

Additionally, I have turned IntializeLazyFunctionScript into the method JSFunction::initializeLazyScript and made it private.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ef490fef5ab
https://hg.mozilla.org/mozilla-central/rev/b5cbd2b5c5e9
https://hg.mozilla.org/mozilla-central/rev/26eeb8c3a0ae

Comment 76

[Till Schneidereit [:till]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35d619dc1707

Comment 77

[Till Schneidereit [:till]]

Comment on attachment 683829 [details] [diff] [review]
Combined patch for fuzzing, v2

Thanks for the fuzzing!

Comment 78

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Looks like no Jetpacks were harmed in the pushing of this bug this time.

Thanks!

Comment 79

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35d619dc1707

Comment 80

[Mozilla RelEng Bot]

Try run for da0f578c7819 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=da0f578c7819
Results (out of 309 total builds):
    exception: 5
    success: 283
    warnings: 17
    failure: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-da0f578c7819

Comment 81

[Mozilla RelEng Bot]

Try run for cb137a08ee3d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cb137a08ee3d
Results (out of 266 total builds):
    success: 256
    warnings: 9
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cb137a08ee3d