787813 - IonMonkey: Assertion failure: isFunctionFrame(), at vm/Stack.h:714 or Crash [@ js::ArgumentsObject::create]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on ionmonkey revision 68df0d67be37 (run with --ion -n -m --ion-eager):


function f(o) {
    var prop = "arguments";
    f[prop] = f[prop];
}
for(var i=0; i < 50; i++) {
    f();
}

Comment 1

[Christian Holler (:decoder)]

Opt-crash:

==5985== Invalid read of size 8
==5985==    at 0x590180: js::ArgumentsObject::create(JSContext*, js::StackFrame*) (Barrier.h:172)
==5985==    by 0x471963: fun_getProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>) (jsfun.cpp:122)
==5985==    by 0x4C794F: js::Shape::get(JSContext*, JS::Handle<JSObject*>, JSObject*, JSObject*, JS::MutableHandle<JS::Value>) (jscntxtinlines.h:439)
==5985==    by 0x4CE356: js::baseops::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>) (jsobj.cpp:4461)
==5985==    by 0x46881F: JSObject::getProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSObject*>, js::PropertyName*, JS::MutableHandle<JS::Value>) (jsobjinlines.h:173)
==5985==    by 0x70211C: js::ion::GetElementCache(JSContext*, unsigned long, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) (jsinterpinlines.h:729)
==5985==    by 0x4032527: ???
==5985==    by 0x633412: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1045)
==5985==    by 0x634C00: js::mjit::JaegerShotAtSafePoint(JSContext*, void*, bool) (MethodJIT.cpp:1103)
==5985==    by 0x4B03C6: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:1484)
==5985==    by 0x4B35CD: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:324)
==5985==    by 0x4B4370: js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) (jsinterp.cpp:509)
==5985==  Address 0x100000000 is not stack'd, malloc'd or (recently) free'd


Bisecting to find an owner more quickly now :)

Comment 2

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   105607:6cd206b37176
parent:      104959:b63bb39ed1c0
parent:      105606:a0240c1043ee
user:        David Anderson
date:        Wed Aug 29 17:51:24 2012 -0700
summary:     Merge from mozilla-central.

Oops! We didn't test rev b63bb39ed1c0, a parent of the blamed revision! Let's do that now.
Rev b63bb39ed1c0: Updating... Compiling... Testing... Exit status: NORMAL (0.022 seconds)
good (not interesting) 
As expected, the parent's label is the opposite of the blamed rev's label.

Oops! We didn't test rev a0240c1043ee, a parent of the blamed revision! Let's do that now.
We did not test rev a0240c1043ee because it is not a descendant of either 4ceb3e9961e4 or 68df0d67be37.
Rev a0240c1043ee: Updating... Compiling... Testing... Exit status: ABNORMAL return code 1 (0.012 seconds)
good (not interesting) 
As expected, the parent's label is the opposite of the blamed rev's label.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Quick look at this suggests we need to port ArgumentsObject::createUnexpected to use StackIter instead of js::StackFrame.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Argument object, Use StackIter instead of StackFrame.

Use StackIter instead of StackFrame for the creation of unexpected argument objects.

Fix latent bug related to the bailout of frame having a needsArgsObj after the bailout.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 661306 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

::: js/src/ion/Bailouts.cpp
@@ +584,5 @@
> +        do {
> +            JS_ASSERT(!iter.isIon());
> +            fp = iter.fp();
> +            if (iter.script()->needsArgsObj()) {
> +                ArgumentsObject *argsobj = ArgumentsObject::createExpected(cx, iter);

luke:

I had to add this piece of code to the bailout path to handle the conversion of frames which are now needing an argument object.  This is documented above the loop located at js/src/jsscript.cpp:2553 .  Can you explain why needsArgsObjs implies that all frames have an argument object?  We were hopping that the interpreter could build the argument object only when it needs one for the bailed frames instead of doing so inside the bailing code.  Can we remove the assertion and do so, or the assertion may prevent futher security/privacy issues?

In js/src/jsscript.cpp:2553, the loop use AllFramesIter, is it possible to run a script with a compartment which is not its own compartment, or we can just replace this loop iterator by ScriptFrameIter?

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 661306 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

::: js/src/ion/Bailouts.cpp
@@ +577,5 @@
> +    // As we cannot allocate during the bailout, we fallback here to allocate
> +    // argument objects which might now be needed if the function now need an
> +    // argument object.
> +    {
> +        br->entryfp()->clearRunningInIon();

I don't understand why this chunk is needed. The only way we could have an unexpected arguments object, is the case where we access f.arguments (since the .apply optimization is static, and if not, would be guarded anyway).

::: js/src/vm/ArgumentsObject.cpp
@@ +129,1 @@
>  ArgumentsObject::createUnexpected(JSContext *cx, StackFrame *fp)

Where does this still get called from, and could it be changed to use StackIter directly instead?

::: js/src/vm/Stack.h
@@ +1773,5 @@
>  
>      CallArgs nativeArgs() const { JS_ASSERT(isNativeCall()); return args_; }
>  
>      template <class Op>
> +    inline void forEachUnaliasedActual(Op op);

forEachUnaliasedActualArg?

Comment 7

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #6)
> Comment on attachment 661306 [details] [diff] [review]
> Argument object, Use StackIter instead of StackFrame.
> 
> Review of attachment 661306 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ion/Bailouts.cpp
> @@ +577,5 @@
> > +    // As we cannot allocate during the bailout, we fallback here to allocate
> > +    // argument objects which might now be needed if the function now need an
> > +    // argument object.
> > +    {
> > +        br->entryfp()->clearRunningInIon();
> 
> I don't understand why this chunk is needed. The only way we could have an
> unexpected arguments object, is the case where we access f.arguments (since
> the .apply optimization is static, and if not, would be guarded anyway).

If we don't do so, then the frame might still be flagged as runningInIon before we enter the ThunkToInterpreter function and cause StackIter to skip this frame after a bailout, because this frame was telling us to look at the IonActivation.  Which makes me think that I should also unwind the IonActivation such as we don't have an off-by-one while iterating stackframes/ionframes.  Another option would be to teach StackIter to iterate properly on bailed entry frames.

> ::: js/src/vm/ArgumentsObject.cpp
> @@ +129,1 @@
> >  ArgumentsObject::createUnexpected(JSContext *cx, StackFrame *fp)
> 
> Where does this still get called from, and could it be changed to use
> StackIter directly instead?

I tried to remove it, but StackIter only iterates on the frames of the same context. AllFramesIter iterates on all StackFrame* and might reach a stack frame which is not reachable from StackIter if the function is used in another context.  Which is why I ask luke for feedback.

> ::: js/src/vm/Stack.h
> @@ +1773,5 @@
> >  
> >      CallArgs nativeArgs() const { JS_ASSERT(isNativeCall()); return args_; }
> >  
> >      template <class Op>
> > +    inline void forEachUnaliasedActual(Op op);
> 
> forEachUnaliasedActualArg?

I just followed the naming used for StackFrame, but I would agree, …
luke, any thoughts ?

Comment 8

[Luke Wagner [:luke]]

Nicolas: assuming I understand the bug correctly, I would think the fix would be to add an "|| iter.isIon()" disjunct to the StackIter loop in fun_getProperty.  That way ion frames just don't appear and all the other issues don't arise.  Given that f.arguments forbids ion compilation of f, and we are in unspecified territory anyhow, this should be astronomically unlikely.

Even though I think most of this patch isn't necessary for this bug (I think the tiny fix above will work), I *do* think there is a latent issue when a script is active on the stack twice and we call JSScript::argumentsSpeculationFailed.  Perhaps this could be filed as a separate bug?

wrt the second question in comment 5: a script can only be run in its *compartment*, but that is not what ScriptFrameIter vs. AllFramesIter is about: the former only iterates over the frames in a single *context* (which can, of course, span multiple compartments).  Since a JSContext has no associativity with compartments/scripts/anything, AllFramesIter is necessary.

Comment 9

[Christian Holler (:decoder)]

This is causing various crashes in opt-builds, marking as fuzzblocker.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #8)
> Nicolas: assuming I understand the bug correctly, I would think the fix
> would be to add an "|| iter.isIon()" disjunct to the StackIter loop in
> fun_getProperty.  That way ion frames just don't appear and all the other
> issues don't arise.  Given that f.arguments forbids ion compilation of f,
> and we are in unspecified territory anyhow, this should be astronomically
> unlikely.

fun_getProperty should not skip Ion frames because even if this is unlikely we still need to honor f.arguments or f.caller.  One of the case is when a function using f.arguments is used for the first time after the compilation of f, such as the current test case with an eager compilation.  Skipping Ion frames will change the behavior and lookup for arbitrary *f* frame above, which is wrong.

(In reply to Luke Wagner [:luke] from comment #8)
> I *do* think there is a latent issue when a
> script is active on the stack twice and we call
> JSScript::argumentsSpeculationFailed.  Perhaps this could be filed as a
> separate bug?

I will.

Comment 11

[Luke Wagner [:luke]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #10)
> fun_getProperty should not skip Ion frames because even if this is unlikely
> we still need to honor f.arguments or f.caller.

Both of those are unspecified, so there is nothing to honor; just "don't break the web".  Given that any site should warm up and hence prevent ion compilation, I don't see skipping ion frames being a realistic problem.

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Alternately, we can do what (I think) Crankshaft does and not skip optimized frames, but return a new Arguments object every time that is untracked and unaliased and in our case may have missing values.

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #10)
> (In reply to Luke Wagner [:luke] from comment #8)
> > Nicolas: assuming I understand the bug correctly, I would think the fix
> > would be to add an "|| iter.isIon()" disjunct to the StackIter loop in
> > fun_getProperty.  That way ion frames just don't appear and all the other
> > issues don't arise.  Given that f.arguments forbids ion compilation of f,
> > and we are in unspecified territory anyhow, this should be astronomically
> > unlikely.
> 
> fun_getProperty should not skip Ion frames because even if this is unlikely
> we still need to honor f.arguments or f.caller.  One of the case is when a
> function using f.arguments is used for the first time after the compilation
> of f, such as the current test case with an eager compilation.  Skipping Ion
> frames will change the behavior and lookup for arbitrary *f* frame above,
> which is wrong.

Simply returning from fun_getProperty, when we hit the Ion frame, does not pass the test suite too.  The test suite fails with «unexpected null» type errors for "x.arguments".

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: Argument object, Use StackIter instead of StackFrame.

This patch exclude modifications needed for Bug 792398 and provide a stackIter interface for ArgumentObject::create.

Comment 15

[Luke Wagner [:luke]]

(In reply to David Anderson [:dvander] from comment #12)
> Alternately, we can do what (I think) Crankshaft does and not skip optimized
> frames, but return a new Arguments object every time that is untracked and
> unaliased and in our case may have missing values.

That's what we do also :)

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 665074 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

After more discussion, I think this patch is right idea.  I do have one non-trivial change/simplification request below, though:

::: js/src/jsfun.cpp
@@ -106,5 @@
>      }
>      if (iter.done())
>          return true;
>  
> -    StackFrame *fp = iter.fp();

Could we make fp() assert that this is not an ion frame?  If some code really does want to access the StackFrame of an ion frame, we could have an ionEntryFrame() which doesn't assert.

::: js/src/vm/ArgumentsObject.cpp
@@ +79,5 @@
>  
>      /*
> +     * If it exists and the arguments object aliases formals, the call object is
> +     * the canonical location for formals. If this is an IonFrame, the call
> +     * object is ignored.

How about a little more explanation:

If this is an IonFrame we break things a bit by ignoring the call object.  We can make this tradeoff since (1) this case only arises in the case of an Arguments::createUnexpected resulting from f.arguments (which is unspecified in non-strict code and disallowed in strict-mode code) and (2) web compatibility should not be affected since f.arguments disables ion compilation for f which makes this difficult to hit in practice.

@@ +114,5 @@
>  ArgumentsObject *
> +ArgumentsObject::createExpected(JSContext *cx, StackFrame *fp)
> +{
> +    StackIter wrapFp(cx, fp);
> +    return createExpected(cx, wrapFp);

Given that there are only calls to createExpected(cx, fp), not (cx, iter), can you just change the existing createExpected instead of adding a new one?  That would, e.g., avoid the "if (!iter.isIon())" branch above.

I'm also a bit uneasy adding this new StackIter constructor: it makes a degenerate iterator that doesn't support iteration.  Making it non-degenerate would I think make the createExpected case unnecessarily slow.  Can we instead make ArgumentsObject::create not take a StackIter but instead take the data it needs.  It seems there are:
 1. numActualArgs
 2. callee
 3. ability to do forEachCanonicalActualArg

Only the latter is hard, and I'd solve it by making create be:

template <class ForEachArgOp>
ArgumentsObject::create(JSContext *cx, unsigned numActualArgs, JSFunction &callee, ForEachArgOp forEachActualArg);

Lastly, for the UnexpectedForEachArgOp can you just insert the current StackFrame-copying logic that is create() instead of (re-)adding StackIter::forEachCanonicalActualArg?  I removed it in m-c since there is almost no reason we should be doing this (due to argument aliasing) so I didn't want to leave a handgun lying around.  I guess the decls got re-added in an IM merge...

Does this work for you?

Comment 17

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #16)
> Comment on attachment 665074 [details] [diff] [review]
> Argument object, Use StackIter instead of StackFrame.
> 
> Review of attachment 665074 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> After more discussion, I think this patch is right idea.  I do have one
> non-trivial change/simplification request below, though:
> 
> ::: js/src/jsfun.cpp
> @@ -106,5 @@
> >      }
> >      if (iter.done())
> >          return true;
> >  
> > -    StackFrame *fp = iter.fp();
> 
> Could we make fp() assert that this is not an ion frame?  If some code
> really does want to access the StackFrame of an ion frame, we could have an
> ionEntryFrame() which doesn't assert.

They are still some issues related to that, I opened a follow-up bug for that (Bug 797977).

> ::: js/src/vm/ArgumentsObject.cpp
> @@ +79,5 @@
> >  
> >      /*
> > +     * If it exists and the arguments object aliases formals, the call object is
> > +     * the canonical location for formals. If this is an IonFrame, the call
> > +     * object is ignored.
> 
> How about a little more explanation:
> 
> If this is an IonFrame we break things a bit by ignoring the call object. 
> We can make this tradeoff since (1) this case only arises in the case of an
> Arguments::createUnexpected resulting from f.arguments (which is unspecified
> in non-strict code and disallowed in strict-mode code) and (2) web
> compatibility should not be affected since f.arguments disables ion
> compilation for f which makes this difficult to hit in practice.

This is not even that, when IonMonkey is on the stack, nothing else is considered alive except the arguments of the IonMonkey frames.  We are not maintaining the state of the call object of the entry frame.  So we can just not alias it.

> @@ +114,5 @@
> >  ArgumentsObject *
> > +ArgumentsObject::createExpected(JSContext *cx, StackFrame *fp)
> > +{
> > +    StackIter wrapFp(cx, fp);
> > +    return createExpected(cx, wrapFp);
> 
> Given that there are only calls to createExpected(cx, fp), not (cx, iter),
> can you just change the existing createExpected instead of adding a new one?
> That would, e.g., avoid the "if (!iter.isIon())" branch above.

Ok but I will need it for Bug 792398.

> […]
> 
> template <class ForEachArgOp>
> ArgumentsObject::create(JSContext *cx, unsigned numActualArgs, JSFunction
> &callee, ForEachArgOp forEachActualArg);

Ok.

> Lastly, for the UnexpectedForEachArgOp can you just insert the current
> StackFrame-copying logic that is create() instead of (re-)adding
> StackIter::forEachCanonicalActualArg?  I removed it in m-c since there is
> almost no reason we should be doing this (due to argument aliasing) so I
> didn't want to leave a handgun lying around.  I guess the decls got re-added
> in an IM merge...

I've removed it for others than IM. so it must be guarded by isIon().

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: Argument object, Use StackIter instead of StackFrame.

Follow luke's nits, when possible for fx18, as documented in previous comment.

Comment 19

[Luke Wagner [:luke]]

Comment on attachment 668109 [details] [diff] [review]
Argument object, Use StackIter instead of StackFrame.

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

r+ assuming the code organization/nits below.

::: js/src/vm/ArgumentsObject.cpp
@@ +23,5 @@
>  using namespace js::gc;
>  
> +/* Erase formals which are not part of the actuals. */
> +static void
> +eraseNonActualFormalArgs(HeapValue *dstBase, unsigned numActuals, unsigned numFormals)

"erase" sounds odd, can we have SetMissingFormalArgsToUndefined (note capitalization of static function name).

@@ +42,5 @@
> +    { }
> +
> +    inline JSScript *script() const { return fp_->script(); }
> +    inline JSFunction *callee() const { return &fp_->callee(); }
> +    unsigned numActualArgs() const { return fp_->numActualArgs(); }

I think it would be better to pass (script, callee, numActualArgs) as explicit arguments to create().  There is hardly any code duplication to speak of and this would make CopyStack*Args more true to its name.  With this and the other changes below, these structs can be almost-trivial wrappers that call the static helper functions I recommend hoisting out below.

@@ +71,5 @@
> +    /*
> +     * If a call object exists and the arguments object aliases formals, the
> +     * call object is the canonical location for formals.
> +     */
> +    void fixFormalLocation(JSObject *obj, ArgumentsData *data) {

How about maybeForwardToCallObject?

@@ +87,5 @@
> +        if (!argsobj)
> +            return NULL;
> +
> +        fp_->initArgsObj(*argsobj);
> +        return argsobj;

Can you stick this back in ArgumentsObject::createExpected?  I don't see any benefit moving it into this class as a member.

@@ +107,5 @@
> +    void copyArgs(HeapValue *dst) const {
> +        if (!iter_.isIon()) {
> +            /* Prevent code duplication. */
> +            CopyStackFrameArgs sf(iter_.fp());
> +            sf.copyArgs(dst);

I think it would be more straightforward to make copyArgs a static function called from both classes.

@@ +125,5 @@
> +    void fixFormalLocation(JSObject *obj, ArgumentsData *data) {
> +        if (!iter_.isIon()) {
> +            /* Prevent code duplication. */
> +            CopyStackFrameArgs sf(iter_.fp());
> +            sf.fixFormalLocation(obj, data);

I think it would be more straightforward to make fixFormalLocation (maybeForwardToCallObject) a static function called by both classes.

::: js/src/vm/ArgumentsObject.h
@@ +109,4 @@
>      inline ArgumentsData *data() const;
>  
> +    struct CopyStackFrameArgs;
> +    struct CopyStackIterArgs;

IIUC, these don't need to be nested classes, they can be entirely local to ArgumentsObject.cpp.

::: js/src/vm/Stack-inl.h
@@ +592,2 @@
>        case NATIVE:
>          JS_NOT_REACHED("Unused ?");

This function may now only be used on ION frames; can you rename it ionForEachActualArg, take out the switch, and  JS_ASSERT(state_ == ION) at the beginning?

Comment 20

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #19)
> Comment on attachment 668109 [details] [diff] [review]
> Argument object, Use StackIter instead of StackFrame.
> 
> Review of attachment 668109 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/ArgumentsObject.h
> @@ +109,4 @@
> >      inline ArgumentsData *data() const;
> >  
> > +    struct CopyStackFrameArgs;
> > +    struct CopyStackIterArgs;
> 
> IIUC, these don't need to be nested classes, they can be entirely local to
> ArgumentsObject.cpp.

I added this to get the visibility to MAYBE_CALL_SLOT in maybeForwardToCallObject.

Comment 21

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47a17015ef4a

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/47a17015ef4a

Comment 23

[Luke Wagner [:luke]]

Looks great, thanks for being so amenable.

Comment 24

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.