656462 - JS debugger needs to be able to recover calls to JSNatives that aren't from script

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

Bug 625000 is about tracking active calls to JSNatives from script.

--- requirements

Ideally there would be a common base class (or otherwise a sum type) of js::StackFrame and another type, js::NativeCall.  The base class must support asking "is this a StackFrame or a NativeCall?".

NativeCall needs to expose the vp and argc of the call, so that the debugger can show the callee, this-value, and arguments.

The common base class needs a method, debugPrev(), analogous to prev() but which exposes both StackFrames and NativeCalls.

ContextStack needs a method, debugfp(), analogous to fp().

--- discussion

Luke and I don't see a way to avoid overhead entirely. This will likely cost a few stores and a conditional branch in CallJSNative, even when debug mode is off.

However most calls to JSNatives are from script, and that path can be zero-overhead; see bug 625000.

Assigning to Luke.

Comment 1

[Luke Wagner [:luke]]

When I started working on this, I ran up against some corner cases in the stack code concerning the handling of Invoke and pushings args for Invoke.  Ultimately, I realized I could make some big simplifications to how stack stuff is handled.  With these changes, comment 0 can be solved by adding a single iterator class without changing any stack logic.  Patches to follow.

Comment 2

[Luke Wagner [:luke]]

Attachment: rm initialVarObj

This removes the initialVarObj member from StackSegment and computes the varobj in a more direct manner.

Comment 3

[Luke Wagner [:luke]]

Attachment: merge ContextStack get* and push* members

I also realized that the old reasons for splitting the get* and push* members are gone and they can be merged into a single atomic push* member.  This simplifies everything and I should have done this earlier.  I wouldn't spend any time on Stack.cpp since its about to get rewritten by a later patch.

Comment 4

[Luke Wagner [:luke]]

Attachment: a few syntactic touchups

Hardly worth a review, but two syntactic changes I wanted to isolate.  s/running/hasfp/ because, as is made obvious by this bug, "running" should be try even if there is no frame (but there is a native call) and all the callers of "running" currently just want to know "is there a frame?".  (Most if not all these callers should eventually be removed by future compartment/global work.)

Comment 5

[Luke Wagner [:luke]]

Attachment: simplify stack

Main simplification patch.  Takes sizeof(StackSegment) down to 4 words (from 8).  The FIXME is fixed in the next patch.

Comment 6

[Luke Wagner [:luke]]

Attachment: make JS_SaveFrameChain fallible

One simplification was removing the "saved" state of a segment and just pushing an empty segment for JS_SaveFrameChain. This means JS_SaveFrameChain can hit OOM.  One caller is already fallible.  The other two just want to clear the chain for the sake of NS_ScriptReportError (see bug 489671), so failing seems innocuous.

Comment 7

[Luke Wagner [:luke]]

Attachment: the new iterator

Finally, the iterator.  I added a shell function dumpStack() for testing purposes.  Preliminary testing shows that it can pick up all native calls with one exception: a native called through Function.prototype.{call,apply} optimized by the mjit (see the last test in testStackIter.js).

Comment 8

[Luke Wagner [:luke]]

Attachment: merge ContextStack get* and push* members

Rebasing over indirect-eval required some changes that suggest a better way to handle the diversity of cases that flow into js::Execute and factor the code better between js::Execute, its callers, and the stack code.

Comment 9

[Luke Wagner [:luke]]

Attachment: simplify stack + new stack iterator

I added some tests with the shell evalInFrame which found some bugs in the previous version.  Supporting evalInFrame actually wants to use the StackIter from within the other stack code (to do the right thing for the native callstack, since its not given as a parameter to JS_EvaluateUCInStackFrame).  Thus, I merged the "simplify stack" patch with the "add the new iterator" patch.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 534918 [details] [diff] [review]
rm initialVarObj

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

::: js/src/jsobj.h
@@ +469,5 @@
>      inline void syncSpecialEquality();
>  
> +    /* See StackFrame::varObj. */
> +    inline bool isVarObj() const { return flags & VAROBJ; }
> +    inline void setVarObj() { flags |= VAROBJ; }

I read "set" as modifying some property of the thing on which it's being called.  I don't read it as modifying the thing itself.  I think "makeVarObj" would be better for this reason.  An analog in our current naming (if one that much more strongly wants the prefix) is makeDenseArraySlow.

::: js/src/methodjit/StubCalls.cpp
@@ +2609,2 @@
>  
> +    JSObject *obj = &fp->varObj();

JSObject *obj = &f.fp()->varObj();

::: js/src/vm/Stack-inl.h
@@ -73,4 @@
>      /* Whether this segment was suspended by JS_SaveFrameChain. */
>      bool                saved_;
>  
> -    /* Align at 8 bytes on all platforms. */

Add a static assertion for this, or point me at the one that already exists.

::: js/src/vm/Stack.h
@@ +684,5 @@
>  
>      /*
> +     * Variables object
> +     *
> +     * Given that a (non-dummy) StackFrame corresonds roughly to a ES5

"corresponds"

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 534966 [details] [diff] [review]
a few syntactic touchups

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

::: js/src/vm/Stack.h
@@ +291,5 @@
>      void                *annotation_;   /* perhaps remove with bug 546848 */
>  
>      static void staticAsserts() {
> +        JS_STATIC_ASSERT(offsetof(StackFrame, rval_) % sizeof(Value) == 0);
> +        JS_STATIC_ASSERT(sizeof(StackFrame) % sizeof(Value) == 0);

...or I guess this was the static-assert-sizeof-StackFrame-is-equal-zero-mod-8 thing.  Although 8 wasn't actually the right measure, sizeof(Value) was.

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #10)

Good point about markVarObj().

> > +    JSObject *obj = &fp->varObj();
> 
> JSObject *obj = &f.fp()->varObj();

?

Comment 13

[Jeff Walden [:Waldo]]

Well, I said "make", but "mark" (or "markAs"?) works as well.

I answered the "?" IRL (the |fp| used there was a one-off variable created just above, no need to name it that way).

Comment 14

[Jeff Walden [:Waldo]]

Comment on attachment 535400 [details] [diff] [review]
merge ContextStack get* and push* members

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

I am not actually confident to the utmost in this (surprise!), but I expect badnesses shouldn't be that hard to debug.  (Hopefully.)

::: js/src/jsfun.cpp
@@ +1487,5 @@
>                           */
>                          if (shape->isMethod() && shape->methodObject() == funobj) {
>                              if (!thisp->methodReadBarrier(cx, *shape, vp))
>                                  return false;
> +                            mutableCalleev().setObject(vp->toObject());

overwriteCallee(JSObject& obj) instead maybe?  Then you don't need to expose the location (well, not most places), you can assert same-looking-ness, &c.

::: js/src/jsinterp.cpp
@@ +655,2 @@
>      if (fun->isNative())
>          return CallJSNative(cx, fun->u.n.native, args.argc(), args.base());

I think it might be the case that all CallJSNative calls now pass in values derived from a |CallArgs| instance, or very nearly so.  Simplify in a followup?

@@ +4532,2 @@
>          goto error;
> +    regs.sp = &args.rval() + 1;

This (and in other places) is kind of abstraction-violating, but I'm not entirely sure how you'd do something else here that wouldn't do so.  CallArgs::postCallSp()?  Seems better than this.

::: js/src/jstracer.cpp
@@ +1794,5 @@
>          }
>  
>          if (JS_UNLIKELY(fp->isEvalFrame())) {
>              visitor.setStackSlotKind("eval");
> +            if (!visitor.visitStackSlots(&fp->mutableCalleev(), 2, fp))

Hmm, a place where you actually do need the location of the callee, and you need it to be mutable.  I suggest friending harder on mutableCalleev to keep it not the preferred way to overwrite the callee slot.

::: js/src/vm/Stack-inl.h
@@ +413,3 @@
>          exec.script = script;
> +#ifdef DEBUG
> +        args.script = (JSScript *)0xbad;

reinterpret_cast<>

::: js/src/vm/Stack.cpp
@@ +534,5 @@
> +
> +    /* For the debugger's sake, always prev-link to the current frame. */
> +    StackFrame *prev = evalInFrame ? evalInFrame : maybefp();
> +
> +    /* Initialize regs, frame, global vars (GVAR ops expect NULL). */

This comment has always bothered me.  "GVAR ops expect NULL" is overly terse, and I don't think it meaningfully explains anything to the reader except that he should look somewhere else for the why of it.

I would suggest improving it, but when I went to look at said ops to figure out what to say, I discovered there are no GVAR ops any more!  (And haven't been, in JM at least, since June 12 last year.)  Does anything depend on initializing the global var slots to null?  Remove this null-initialization (in a separate revision for utmost safety) if a try-run or similar says nothing does, as I think is the case.  \o/

::: js/src/vm/Stack.h
@@ +1017,5 @@
>  
> +static inline uintN
> +ToReportFlags(MaybeConstruct construct)
> +{
> +    return (uintN)construct;

uintN(construct)

@@ +1025,5 @@
> +ToFrameFlags(MaybeConstruct construct)
> +{
> +    JS_STATIC_ASSERT((int)CONSTRUCT == (int)StackFrame::CONSTRUCTING);
> +    JS_STATIC_ASSERT((int)NO_CONSTRUCT == 0);
> +    return (StackFrame::Flags)construct;

StackFrame::Flags(construct)

Comment 15

[Luke Wagner [:luke]]

Attachment: make JS_SaveFrameChain fallible

Oops, the attached patch wasn't qref'd.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

What does the failure case look like here? It seems like if we fail to save the frame chain, things occur as if we didn't try to set aside the stack frame at all? That makes me slightly worried, since we do use SaveFrameChain for security-type stuff. Luke, do you think that compartments save us in that case?

Comment 17

[Luke Wagner [:luke]]

(In reply to comment #14)
> overwriteCallee(JSObject& obj) instead maybe?  Then you don't need to expose
> the location (well, not most places), you can assert same-looking-ness, &c.

I considered doing this but balked since not all uses of mutableValue() could be replaced (VisitFrameSlots still wants mutable Value*).  But you're right; overwriteCallee() would be good to use where possible.

> I think it might be the case that all CallJSNative calls now pass in values
> derived from a |CallArgs| instance, or very nearly so.  Simplify in a
> followup?

By jove, you're right!  I do it now :)

> This (and in other places) is kind of abstraction-violating, but I'm not
> entirely sure how you'd do something else here that wouldn't do so. 
> CallArgs::postCallSp()?  Seems better than this.

Again, you read my mind.  I wimped out b/c I thought it might be overkill.  But if you think so too, I'll do it.  spAfterCall sounds good to me (and avoids mixed-case 'sp').

> > +        args.script = (JSScript *)0xbad;
> 
> reinterpret_cast<>

I disagree here: reinterpret_cast is useful as a syntactic way of drawing attention to a cast.  I don't think there is any way to be mistaken about the poisoning going on here.

> This comment has always bothered me.  "GVAR ops expect NULL" is overly
> terse, and I don't think it meaningfully explains anything to the reader
> except that he should look somewhere else for the why of it.
> 
> I would suggest improving it, but when I went to look at said ops to figure
> out what to say, I discovered there are no GVAR ops any more!  (And haven't
> been, in JM at least, since June 12 last year.)  Does anything depend on
> initializing the global var slots to null?  Remove this null-initialization
> (in a separate revision for utmost safety) if a try-run or similar says
> nothing does, as I think is the case.  \o/

You will be happy to know that you are right and this exact change already awaits you in the next patch...

Comment 18

[Luke Wagner [:luke]]

(In reply to comment #16)
> What does the failure case look like here? It seems like if we fail to save
> the frame chain, things occur as if we didn't try to set aside the stack
> frame at all? That makes me slightly worried, since we do use SaveFrameChain
> for security-type stuff. Luke, do you think that compartments save us in
> that case?

There are only three uses of JS_SaveFrameChain (after dead code is removed).  I think the only security-related one is the the context stack Push/Pop operations.  For this, Push is already fallible (if the Append call OOMs), so JS_SaveFrameChain just uses the same path.  The other two are what I talked about in comment 6.

Comment 19

[Luke Wagner [:luke]]

Attachment: simplify stack + new stack iterator

Rebased and with a (preexisting) bug fix to maybeMigrateVersionOverride to fix mochitest hang.

Comment 20

[Luke Wagner [:luke]]

Attachment: make JS_SaveFrameChain fallible

Rebased.  Also fixes a (preexisting, but apparently innocuous) bug where mozJSComponentLoader was calling JS_RestoreFrameChain before having popped a dummy frame that was pushed after JS_SaveFrameChain.

Comment 21

[Luke Wagner [:luke]]

...and with those fixes, seems to be green on try.

Comment 22

[Jeff Walden [:Waldo]]

Comment on attachment 536490 [details] [diff] [review]
simplify stack + new stack iterator

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

::: js/src/jit-test/tests/basic/testStackIter.js
@@ +1,1 @@
> +function stackToString(stack) {

Throughout this file you use == and !=.  == and != are bad form, and === and !== are good form, but as it's test code I don't really care.  Just something to keep in mind when writing JS code.

@@ +60,5 @@
> +        return;
> +    }
> +    f(x-1);
> +})(N);
> +

Add some tests verifying the stack for underflow, overflow, and equal arguments, for a custom function, Function.prototype.apply, Function.prototype.call, and for some random native function like String or something.

::: js/src/jscntxt.cpp
@@ +763,5 @@
>       * Walk stack until we find a frame that is associated with some script
>       * rather than a native frame.
>       */
> +    StackFrame *fp = js_GetScriptedCaller(cx, NULL);
> +    if (fp) {

if (SF* fp = ...)

::: js/src/jsfun.h
@@ +218,5 @@
>      }
>  
> +    js::Native native() const {
> +        return u.n.native;
> +    }

Heh, I think I have this in a patch in my tree, waiting for review.  Not that that should stop you or anything.  :-)

::: js/src/jsscript.cpp
@@ +1644,5 @@
>      return result;
>  }
>  
>  uintN
>  js_FramePCToLineNumber(JSContext *cx, StackFrame *fp)

Hmm, this should probably be named to indicate slowness.  Followup fodder.

::: js/src/jstracer.cpp
@@ +384,5 @@
>      return '?';
>  }
> +
> +static inline uintN
> +FramePCOffset(JSContext *cx, js::StackFrame* fp)

This should be named to indicate slowness, seems to me.  More followup.

::: js/src/shell/js.cpp
@@ +2758,5 @@
> +    if (!globalStr)
> +        return false;
> +
> +    StackIter iter(cx);
> +    ++iter;  /* Skip DumpStack. */

Maybe assert |iter.fp()->native() == DumpStack|?  Just for sanity.

@@ +2761,5 @@
> +    StackIter iter(cx);
> +    ++iter;  /* Skip DumpStack. */
> +
> +    jsint index = 0;
> +    for (; !iter.done(); ++index, ++iter) {

This doesn't invoke the method read barrier, so you're handing out primordial functions here, which is not kosher.  I'd prefer if you handed out the name of the function here instead (or "callable" or something for non-function callables).

I'd prefer if the output distinguished between direct and indirect eval somehow, not entirely sure what that should look like.

::: js/src/vm/Stack.cpp
@@ +118,5 @@
> +{
> +    PodZero(this);
> +    flags_ = DUMMY | HAS_PREVPC | HAS_SCOPECHAIN;
> +    initPrev(cx);
> +    chain.isGlobal();

Erm.  This statement seems to be an elaborate way to do nothing, right?

@@ +259,5 @@
> +                 ? Max(regs_->sp, calls_->end())
> +                 : calls_->end()
> +               : regs_
> +                 ? regs_->sp
> +                 : slotsBegin();

This is a really ugly assignment.  I guess case-wise it's moderately sensible, but I still don't much like it.

@@ +651,5 @@
> +     * callstack looks right to the debugger (via CAN_EXTEND). This is safe
> +     * since the scope chain is what determines name lookup and access, not
> +     * prev-links.
> +     *
> +     * Eval-in-frame is the exception since it prev-linking to an arbitrary

it's, I think?

@@ +898,5 @@
> +                    ++tmp;
> +                JS_ASSERT(tmp.state_ == SCRIPTED && tmp.seg_ == seg_ && tmp.fp_ == fp_);
> +                *this = tmp;
> +                return;
> +            } else {

else after return

@@ +910,5 @@
> +         * In case of both a scripted frame and call record, use linear memory
> +         * ordering to decide which was the most recent.
> +         */
> +        if (containsFrame && (!containsCall || (Value *)fp_ >= calls_->argv())) {
> +            /* Nobody wants to see dummy frames. */

WORD.

@@ +935,5 @@
> +             * to 'replace' will not appear on the callstack.
> +             *
> +             *   (String.prototype.replace).call('a',/a/,function(){debugger});
> +             *
> +             * Function.prototype.call will however appear hence the debugger

"...appear hence..." is run-on-y.  Insert a comma, make it "appear, so", or something like that.

@@ +938,5 @@
> +             *
> +             * Function.prototype.call will however appear hence the debugger
> +             * can, by inspecting 'args.thisv', give some useful information.
> +             */
> +            if (*pc_ == JSOP_CALL || *pc_ == JSOP_FUNCALL) {

You *know* somebody's going to start trapping this and making stuff fail.  Use whatever that method is to get the real opcode here.

@@ +949,5 @@
> +                    state_ = IMPLICIT_NATIVE;
> +                    args_ = CallArgsFromVp(argc, vp);
> +                    return;
> +                }
> +            } else if (*pc_ == JSOP_FUNAPPLY) {

And here.

::: js/src/vm/Stack.h
@@ +103,5 @@
>   * (js::StackFrame) which is associated with the values (called "slots") before
>   * and after it. The frame contains bookkeeping information about the activation
>   * and links to the previous frame.
>   *
>   * The slots preceeding a (function) StackFrame in memory are the arguments of

Barely in patch context, but "preceding".

@@ +137,5 @@
> + *       |                               prev                  ^
> + *       `-----------------------------------------------------'
> + *                                  calls
> + *
> + * Here there are two native calls on the stack. The begin of each native arg

s/begin/start/

@@ +1112,5 @@
>          JS_STATIC_ASSERT(offsetOfFp == offsetof(FrameRegs, fp_));
>      }
>  
>      /* For generator: */
>      void rebaseFromTo(const FrameRegs &from, StackFrame *to) {

Seems like |to| should be a reference here if you're asserting non-nullness and all.

@@ +1136,4 @@
>      }
>  
>      /* For stubs::CompileFunction, ContextStack: */
>      void prepareToRun(StackFrame *fp, JSScript *script) {

Again here too.

@@ +1144,4 @@
>      }
>  
>      /* For pushDummyFrame: */
>      void initDummyFrame(StackFrame *fp) {

And here.

@@ +1181,5 @@
> +
> +    /* A segment is followed in memory by the arguments of the first call. */
> +
> +    Value *slotsBegin() const {
> +        return (Value *)(this + 1);

reinterpret_cast<>

@@ +1302,5 @@
> +     * Finding the pc for an arbitrary frame requires finding its next frame
> +     * which, since prev-linkage goes the other direction, requires searching
> +     * the stack. To avoid O(n^2) complexity, consider FrameRegsIter.
> +     */
> +    jsbytecode *pcForFrameSlow(StackFrame *fp) const;

I was originally thinking of it as a joke ("hahahaha wouldn't it be funny if we named it that"), but what do you think of pcForFrameQuadratic?  There's no pleading ignorance with that name.

@@ +1425,5 @@
>  #endif
>  
> +    /* Implementation details of push* public interface. */
> +    StackSegment *pushSegment(JSContext *cx);
> +    enum MaybeExtend { CANT_EXTEND = false, CAN_EXTEND = true };

Mega-nit, but my eyes catch just a little at CANT then CAN -- suggest CAN then CANT.

@@ +1480,5 @@
>      /* The StackSpace currently hosting this ContextStack. */
>      StackSpace &space() const    { assertSpaceInSync(); return *space_; }
>  
> +    /* Return whether the given frame is in this context's stack. */
> +    bool containsSlow(const StackFrame *target) const;

containsSlowLinear, or containsLinear?  Or something similarly indicating of lengthy-walk?

@@ +1540,5 @@
> +    void restoreFrameChain();
> +
> +    /*
> +     * As an optimization, the interpreter/mjit can operate on a local
> +     * FrameRegs instance repoint the ContextStack to this local instance.

???

@@ +1547,5 @@
> +
> +    /*** For JSContext: ***/
> +
> +    /*
> +     * To avoid indirection, ContextSpace caches a pointers to the StackSpace.

pointers

Comment 23

[Luke Wagner [:luke]]

(In reply to comment #22)

Thanks for all the reviews!

> > +    chain.isGlobal();
> 
> Erm.  This statement seems to be an elaborate way to do nothing, right?

Hehe, good eye; yeah it should be JS_ASSERT(chain.isGlobal()) :)

> > +            if (*pc_ == JSOP_CALL || *pc_ == JSOP_FUNCALL) {
> 
> You *know* somebody's going to start trapping this and making stuff fail. 
> Use whatever that method is to get the real opcode here.

Nice.

> > +    jsbytecode *pcForFrameSlow(StackFrame *fp) const;
> 
> I was originally thinking of it as a joke ("hahahaha wouldn't it be funny if
> we named it that"), but what do you think of pcForFrameQuadratic?  There's
> no pleading ignorance with that name.

Already landed on trunk with bug 538293 :)

> > +    /* Return whether the given frame is in this context's stack. */
> > +    bool containsSlow(const StackFrame *target) const;
> 
> containsSlowLinear, or containsLinear?  Or something similarly indicating of
> lengthy-walk?

Note here that containsSlow() is doing a linear search of *segments*, with a constant-time search within a segment.  With this patch, segments only get pushed when changing contexts which means heavy-duty browser reentrance which will blow out the C stack before getting more than a few segments.  So I think we can just leave it at "slow".

> > +     * As an optimization, the interpreter/mjit can operate on a local
> > +     * FrameRegs instance repoint the ContextStack to this local instance.
> 
> ???

Its true...

Comment 24

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/9cdb856cf910
http://hg.mozilla.org/tracemonkey/rev/780888b1548c
http://hg.mozilla.org/tracemonkey/rev/8ab0930a7b83
http://hg.mozilla.org/tracemonkey/rev/bb9e5496b0ac

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/9cdb856cf910
http://hg.mozilla.org/mozilla-central/rev/780888b1548c
http://hg.mozilla.org/mozilla-central/rev/8ab0930a7b83
http://hg.mozilla.org/mozilla-central/rev/bb9e5496b0ac