Closed Bug 791850 Opened 7 years ago Closed 7 years ago

Lazily clone self-hosted methods installed via js_DefineFunction

Categories

(Core :: JavaScript Engine, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: till, Assigned: till)

References

(Blocks 1 open bug)

Details

(Whiteboard: [js:t])

Attachments

(3 files, 22 obsolete files)

17.04 KB, patch
Details | Diff | Splinter Review
v12
44.09 KB, patch
Waldo
: review+
bhackett
: review+
Details | Diff | Splinter Review
89.73 KB, patch
bhackett
: review+
Details | Diff | Splinter Review
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.
Attached patch v1 (obsolete) — Splinter Review
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.
Assignee: general → tschneidereit
Status: NEW → ASSIGNED
Attachment #662335 - Flags: review?(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?
Attachment #662335 - Flags: review?(luke) → review+
Attached patch v2, carrying r+ (obsolete) — Splinter Review
(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.
Attachment #662335 - Attachment is obsolete: true
Attachment #663034 - Flags: review+
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.
Attached patch v3 (obsolete) — Splinter Review
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
Attachment #663034 - Attachment is obsolete: true
Attachment #674142 - Flags: review?(luke)
Attachment #674142 - Attachment is patch: true
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/tschneidereit@gmail.com-0f9730d7a4d8
Comment on attachment 674142 [details] [diff] [review]
v3

Yeah, so that didn't quite work. Running tests locally might help ...
Attachment #674142 - Flags: review?(luke)
Attached patch v4 (obsolete) — Splinter Review
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.
Attachment #674142 - Attachment is obsolete: true
Attachment #674886 - Flags: review?(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?
Attachment #674886 - Flags: review?(luke)
Attached patch v5 (obsolete) — Splinter Review
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
Attachment #674886 - Attachment is obsolete: true
Attachment #675763 - Flags: review?(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())
Attachment #675763 - Flags: review?(luke)
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/tschneidereit@gmail.com-2c0865ac331c
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/tschneidereit@gmail.com-2c0865ac331c
Attached patch v6 (obsolete) — Splinter Review
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
Attachment #675763 - Attachment is obsolete: true
Attachment #680445 - Flags: review?(luke)
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.
Attachment #680445 - Flags: review?(luke)
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/tschneidereit@gmail.com-c47f46026da6
Attached patch v7 (obsolete) — Splinter Review
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.
Attachment #680445 - Attachment is obsolete: true
Attachment #680478 - Flags: review?(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).
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/tschneidereit@gmail.com-42fe9480eab2
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.
(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.
(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!
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.
(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!
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/tschneidereit@gmail.com-c47f46026da6
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/tschneidereit@gmail.com-42fe9480eab2
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.
(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 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.)
Attachment #680478 - Flags: review?(luke)
Attached patch V8 (obsolete) — Splinter Review
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.
Attachment #680478 - Attachment is obsolete: true
Attachment #680852 - Flags: review?(luke)
Oh, and pushed to try again because of the assert in JSFunction::script() here:
http://tbpl.mozilla.org/?tree=Try&rev=f7e7be126994
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.
Attachment #680852 - Flags: review?(luke) → review+
Great work, by the way!  Does this mean we can turn on some self-hosted functions w/o hurting memory now?
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/tschneidereit@gmail.com-f7e7be126994
https://hg.mozilla.org/mozilla-central/rev/862f9cd7eb0b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Depends on: 811584
Depends on: 811625
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 808949
Attached patch supplemental patch (obsolete) — Splinter Review
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.
Attached patch supplemental patch (obsolete) — Splinter Review
Oops, used the wrong comment style for the part of the engine this patch is in.
Attachment #681432 - Attachment is obsolete: true
Attached patch v9, wip (obsolete) — Splinter Review
Fixes most issues revealed by fuzzing and incorporates Shu's supplemental patch. Still choking on proxies, though.
Attachment #680852 - Attachment is obsolete: true
Attachment #681433 - Attachment is obsolete: true
Attached patch Decompile patch (obsolete) — Splinter Review
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.
Blocks: 811853
(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.
Attached patch Decompile patch v2 (obsolete) — Splinter Review
Per discussion with till, changing to %_DecompileArg.

Luke, what do you think about this approach?
Attachment #681841 - Attachment is obsolete: true
Attachment #682120 - Flags: feedback?(luke)
Depends on: 811694
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)
Attachment #682120 - Flags: feedback?(luke)
Attached patch Decompile patch v3 (obsolete) — Splinter Review
Refactored according to luke's righteous suggestions.
Attachment #682120 - Attachment is obsolete: true
(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"
Status: REOPENED → NEW
Attached patch v10, wip (obsolete) — Splinter Review
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)
Attachment #681570 - Attachment is obsolete: true
Attached patch Decompile patch v4 (obsolete) — Splinter Review
(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.
Attachment #682673 - Attachment is obsolete: true
(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.
Attached patch Decompile patch v5 (obsolete) — Splinter Review
5th try, hack around frameless natives for |"".replace(RegExp(), Array.reduce)|
Attachment #682949 - Attachment is obsolete: true
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.
Attachment #683062 - Flags: review?(luke)
Attached patch v11 (obsolete) — Splinter Review
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
Attachment #682937 - Attachment is obsolete: true
Attachment #683170 - Flags: review?(luke)
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 on attachment 683170 [details] [diff] [review]
v11

As just discussed on IRC.
Attachment #683170 - Flags: review?(luke) → review?(jwalden+bmo)
Attachment #683062 - Flags: review?(luke) → review?(jwalden+bmo)
Attached patch Combined patch for fuzzing (obsolete) — Splinter Review
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!
Attachment #683344 - Flags: feedback?(gary)
Attachment #683344 - Flags: feedback?(choller)
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.
Attachment #683062 - Flags: review?(jwalden+bmo) → review-
Should I wait with the fuzzing for a new patch? :)
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.
Attached patch Decompile patch v6 (obsolete) — Splinter Review
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|.
Attachment #683062 - Attachment is obsolete: true
Attachment #683386 - Flags: review?(jwalden+bmo)
Attached patch Decompile patch v6 (obsolete) — Splinter Review
Oops, had a JSAtom * instead of RootedAtom.
Attachment #683386 - Attachment is obsolete: true
Attachment #683386 - Flags: review?(jwalden+bmo)
Attachment #683387 - Flags: review?(jwalden+bmo)
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.
Attachment #683170 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #683387 - Flags: review?(jwalden+bmo) → review+
The decompile patch with Waldo's comments applied.

Till, please fold this into the main patch.
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).
Attachment #683344 - Flags: feedback?(choller) → feedback-
(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.
s/can might/might
Whiteboard: [js:t]
Attachment #683387 - Attachment is obsolete: true
Attachment #683344 - Attachment is obsolete: true
Attachment #683344 - Flags: feedback?(gary)
Attached patch Combined patch for fuzzing, v2 (obsolete) — Splinter Review
Thanks for the last round of fuzzing! Can I have another one, please?
Attachment #683829 - Flags: feedback?(gary)
Attachment #683829 - Flags: feedback?(choller)
Attached patch v12Splinter Review
@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!
Attachment #683170 - Attachment is obsolete: true
Attachment #683832 - Flags: review?(jwalden+bmo)
Attachment #683832 - Flags: review?(bhackett1024)
Attachment #683832 - Attachment is patch: true
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(...).
Attachment #683832 - Flags: review?(jwalden+bmo) → review+
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.
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 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.
Attachment #683832 - Flags: review?(bhackett1024) → review+
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.
Whiteboard: [js:t] → [leave open][js:t]
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.
Attachment #684050 - Flags: review?(bhackett1024)
Attachment #684050 - Flags: review?(bhackett1024) → review+
Comment on attachment 683829 [details] [diff] [review]
Combined patch for fuzzing, v2

Thanks for the fuzzing!
Attachment #683829 - Attachment is obsolete: true
Attachment #683829 - Flags: feedback?(gary)
Attachment #683829 - Flags: feedback?(choller)
Looks like no Jetpacks were harmed in the pushing of this bug this time.

Thanks!
https://hg.mozilla.org/mozilla-central/rev/35d619dc1707
Status: NEW → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 815010
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/tschneidereit@gmail.com-da0f578c7819
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/tschneidereit@gmail.com-cb137a08ee3d
You need to log in before you can comment on or make changes to this bug.