124474 - add native frame support to jsd

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Robert Ginda]

patches coming up to add the ability to include native frames in JSDThreadStates.

Comment 1

[Robert Ginda]

Attachment: patch

adds the following entrypoints to jsdebug.h...

JSD_(Get|Set)ContextFlags
Sets flags for a given JSDContext.  Currently the only flag supported is
JSD_INCLUDE_NATIVE_FRAMES, which enables native frames in JSDThreadStates.

JSD_GetNameForStackFrame
Gets the function name for a stack frame.  Clients used to get this from the
script attached to a given stack frame, but we don't have a script for native
frames.  This lets clients get the function name for a script or native frame
from a single place.

JSD_IsStackFrameNative
Returns true if the stack frame is native.

JSD_IsStackFrameDebugger
Returns true if the stack frame was created by a debugger evaluation.

JSD_IsStackFrameConstructing
Returns true if the stack frame is constructing a new object.



Native frames intorduce the possibility of frames with null scripts.  This is
something that was not previously possible in the debugger.  Instead of turning
on native frames always, which stands a good chance of introducing null
dereferences into existing clients, JSD_SetContxtFlags was introduced.	The
default is *not* to include native frames, so existing code does not need to
change.

Comment 2

[Robert Ginda]

Attachment: small js/src patch

Fix JS_IsContructorFrame typo.

Comment 3

[Robert Ginda]

tested on windows and linux.

I can post the UI patches if anyone cares to have a look.

Comment 4

[Michael Schneider]

JSD_GetNameForStackFrame() crashes if i call the function on a frame which 
started script execution form JS_ExecuteScript() (global script, when fp->fun 
is NULL) in jsapi.c,JS_GetFunctionName.. i think there should be a check for 
NULL in jsd_GetNameForStackFrame() after jsd_GetFrameFunction()...

A second thing is, that when the JSD_INCLUDE_NATIVE_FRAMES option is set, we 
see the dummy stackframe created by the JS_CallFunctionName() api call.. i've 
fixed this by adding 
if (jsdthreadstate && jsdthreadstate->stack.prev && JS_IsNativeFrame(cx, 
((JSDStackFrameInfo*)jsdthreadstate->stack.prev)->fp))
{
 JSDStackFrameInfo *pFrame = (JSDStackFrameInfo*)jsdthreadstate->stack.prev;
 JS_REMOVE_LINK(&pFrame->links);
 _destroyFrame(pFrame);
 jsdthreadstate->stackDepth--;
}

in jsd_stak.c, jsd_NewThreadState(), below the while loop (sorry, i've no diff 
tool at hand).. 
or should this be another option?

Comment 5

[Robert Ginda]

> JSD_GetNameForStackFrame() crashes if i call the function on a frame which 
> started script execution form JS_ExecuteScript() (global script, when fp->fun 
> is NULL) in jsapi.c,JS_GetFunctionName.. i think there should be a check for 
> NULL in jsd_GetNameForStackFrame() after jsd_GetFrameFunction()...

Thanks, I've got that fix in my local tree.

> A second thing is, that when the JSD_INCLUDE_NATIVE_FRAMES option is set, we 
> see the dummy stackframe created by the JS_CallFunctionName() api call.. i've 
> fixed this by adding 

That frame could be used by the debugger to indicate that a function was called
by the embedding, as opposed to running from top level.  Maybe the jsd client
app should be allowed to see it (and filter it itself.)

Comment 6

[Michael Schneider]

i think you can verify that by checking the function name for the stack 
frame.. but it may break a line stepper implementation (at least it does 
mine ;)) if the stack count is suddenly 2.. what about a JSD_HIDE_DUMMY_FRAMES 
flag?

Comment 7

[Robert Ginda]

Attachment: better patch

Yes, you're correct.  A frame with no function name let's us know it's top
level, these dummy frames won't help anyone.  This new patch includes the null
check in JSD_GetNameForStackFrame, and hides "dummy" frames by checking for a
missing |this| object.

There is also some cleanup in jsds_FilterHook.	Return PR_FALSE (don't call
exec hook) if the frame looks invalid, and don't warn about frames without
scripts, which are now OK.

Patch ready for r/sr= so I can move along, any takers?

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 69031 [details] [diff] [review]
better patch

sr=shaver

Comment 9

[John Bandhauer]

Comment on attachment 69031 [details] [diff] [review]
better patch

+	 if (JS_GetFrameThis(cx, fp) &&
+	     (jsdc->flags & JSD_INCLUDE_NATIVE_FRAMES ||
+	      !JS_IsNativeFrame(cx, fp)))


I like to see more parens when bitwise and logical ands and ors are mixed.

+const char*
+jsd_GetNameForStackFrame(JSDContext* jsdc, 
+			  JSDThreadState* jsdthreadstate,
+			  JSDStackFrameInfo* jsdframe)
+{
+    const char *rv = NULL;
+    
+    JSD_LOCK_THREADSTATES(jsdc);
+    
+    if( jsd_IsValidFrameInThreadState(jsdc, jsdthreadstate, jsdframe) )
+    {
+	 JSFunction *fun = JS_GetFrameFunction (jsdthreadstate->context,
+						jsdframe->fp);
+	 if (!fun)
+	     return NULL;
+	 rv = JS_GetFunctionName (fun);
+    }
+    
+    JSD_UNLOCK_THREADSTATES(jsdc);
+    return rv;
+}

You have a return while locked. Must fix.

  if (fun)
    rv = JS_GetFunctionName (fun);

How old is "JS_IsContructorFrame"? That's the kind of typo *I* make :)

Address the above and r=jband

Comment 10

[Robert Ginda]

checked in.

Comment 11

[Aleksey Nogin]

There is a minor issue with the new jsd_IsStackFrameNative

+JSBool
+jsd_IsStackFrameNative(JSDContext* jsdc, 
+                       JSDThreadState* jsdthreadstate,
+                       JSDStackFrameInfo* jsdframe)
+{
+    JSBool rv;
+    
+    JSD_LOCK_THREADSTATES(jsdc);
+
+    if( jsd_IsValidFrameInThreadState(jsdc, jsdthreadstate, jsdframe) )
+    {
+        rv = JS_IsNativeFrame(jsdthreadstate->context, jsdframe->fp);
+    }
+
+    JSD_UNLOCK_THREADSTATES(jsdc);
+    return rv;
+}

It would return an uninitialized JSBool when jsd_IsValidFrameInThreadState is false.

Comment 12

[Robert Ginda]

Attachment: fix uninitialized return value

Call invalid stack frames non-native, I guess.
Are you actually hitting this problem, or just noticed it in the diff?

Comment 13

[Aleksey Nogin]

> Are you actually hitting this problem, or just noticed it in the diff?

No, just noticed. I have a script that tracks "xxx might be used uninitialized"
warnings (see also bug 59652 about those warnings) and I try to invertigate when
a new one is introduced.

Comment 14

[Robert Ginda]

checked in