379410 - debugger script detection using parent JSScript*

Status: NEW

(Core :: JavaScript Engine, enhancement)

Description

[Mike Moening]

I would like to propose a simple change to allow a debugger using the NewScriptHook debug hook to be able to better tell the difference between a "top-level" JSScript and any JSScript that is generated as result of an eval(). Also I would like to allow a debugger to tell if a function JSScript was created by "top-level" function declaration or if the function was generated inside an eval using "new" or statically.

Specifically I am proposing a simple to modification to the JSScript struct by adding a parent JSScript* member.  For a "top-level" script this would be NULL. For any script generated for a eval or function declaration the parent member would point back to either the top-level script or the child eval script which owns the script or function. 

Ultimately this would provide much more powerful debugging capabilities for debuggers like Venkman to be able to properly handle "AJAX" like frameworks which frequently use evals.
 
Currently there are no good ways to distinguish evals or functions within evals that do not involve hacks or double hooks. From my experience these hacks are complex and don't work 100% of the time.

I value your feedback. Please respond.
Once I get some consensus I'll attach a patch.

P.S. I implemented this already once and it worked well without changing the JS API.

Thanks!
Mike M.

Comment 1

[Mike Moening]

Would using a parent JSScript* require me to root the parent JSScript to protect it from being GC'd out from under me?

I could also be done with a simple boolean eval flag member on JSScript but I think knowing the parent lineage gives you much more power.

Comments?

Comment 2

[Igor Bukanov]

To Mike Moening:

It turned out that the current form of newScripHook prevents some optimizations that changing the hook signature/behavior would allow (see bug 398219 comment 11). I would like to know is this bug still actual for you and would you test an altered newScriptHook to see if it would address this bug?

Comment 3

[Mike Moening]

Yes absolutely!

I'm not 100% up on what you are proposing in your change. Can you send me a private email and give me the details?

Comment 4

[Igor Bukanov]

Here is a sketch of the proposed changes to the hook:

1. The hook will only be called once per compilation. It would not be called for function and their JSScript objects that are defined by the compiled script or function.

2. The api would be extended so an embedding can enumerate the nested functions and their scripts defined by the compiled script.

3. The hook signature would be changed so it would not contain JSFunction. Rather it would come an with extra flag parameter describing compilation context so an embedding can know that the script was compiled as a consequence of eval/Function invocation or as an internal API invocation.

To Mike Moening: would this work for you?

Comment 5

[Mike Moening]

So that means that the DestroyScriptHook will only be called once too?
Please pass the extra flag information to the destroy hook too so the API's are in sync.

Make sure your patch will handle the goals defined at the top of this bug.
Let me know if it falls short in any way.

So I take it that by the time the NewScriptHook is called all nested functions and their scripts will be available for enumeration from within this hook?

Right now debuggers need to maintain a list/map of function scripts in order to properly set a breakpoint on a line within the function.  We parse the script to determine function name (or anonymous) as well as last line for breakpoints etc... I think this extra information that is kept for each function will still need to be maintained in the debugger.  With this patch do you see any way for the functions to be destroyed and this invalidating the extra information without the debugger being notified somehow?

All in all I think your proposal is very good.  Having the newScriptHook being fired for functions always seemed a bit weird to me anyway.

Igor, lets do it!

When will the patch be available?

Comment 6

[Brendan Eich [:brendan]]

This needs wider discussion, please use the m.d.t.js-engine group. I cc'ed Firebug owner but I don't know who else to cc off the top of my head. Timeless?

/be

Comment 7

[John J. Barton]

I absolutely agree that the current JSD layer needs work.  I am excited to hear that there may be people able and interested in working in that layer.

However I am a little confused by this proposal.

(In reply to comment #0)
> I would like to propose a simple change to allow a debugger using the
> NewScriptHook debug hook to be able to better tell the difference between a
> "top-level" JSScript and any JSScript that is generated as result of an eval().

Yes, distinguishing eval would be great.

> Also I would like to allow a debugger to tell if a function JSScript was
> created by "top-level" function declaration or if the function was generated
> inside an eval using "new" or statically.

I don't understand "statically".  

> 
> Specifically I am proposing a simple to modification to the JSScript struct by
> adding a parent JSScript* member.  For a "top-level" script this would be NULL.
> For any script generated for a eval or function declaration the parent member
> would point back to either the top-level script or the child eval script which
> owns the script or function. 

But how can I tell eval from nested in top-level or nested in eval?

> 
> Ultimately this would provide much more powerful debugging capabilities for
> debuggers like Venkman to be able to properly handle "AJAX" like frameworks
> which frequently use evals.
> 
> Currently there are no good ways to distinguish evals or functions within evals
> that do not involve hacks or double hooks. From my experience these hacks are
> complex and don't work 100% of the time.

I agree that my Firebug solution is a hack and complex. And you didn't mention slow. But I don't know of a case it fails and no one has brought one to my attention.


> 
> I value your feedback. Please respond.
> Once I get some consensus I'll attach a patch.

I think this needs work. It begs to the question: who will use the new API? I also think there is lots more to be done on JSD.



> 
> P.S. I implemented this already once and it worked well without changing the JS
> API.
> 
> Thanks!
> Mike M.
> 

Comment 8

[Mike Moening]

John, In response to your questions in comment #7.

My initial proposal was to put a parent JSScript* in the JSScript struct.
By following the chain of parents you can determine what you need to know.

However, this is not what Igor is proposing (I think).  There are many ways to solve a problem of course.  I am awaiting his patch to see how things shake out...

On last commment on a new API for enumerating functions in the script.
How will the debugger be made aware of functions created during execution of the script?  i.e. those created using "new".  I think we would need at least a way of knowing more functions exist so that we can enumerate them again and pick up the new ones. So some callback that tells me there has been a change would be nice..

Comment 9

[John J. Barton]

Ok, I'll look back at Igor comment. Overall I don't understand the context of these proposals. I don't use JSScript, NewScriptHook or DestroyScriptHook, so maybe my comments aren't useful here. 

From the perspective of Firebug/Venkman its easy to distinguish "eval()" in JSD's onScriptCreated if we have the callstack: eval() is called but top-level is not. My original hack was just a way to get the callstack. If JSD passed it into onScriptCreated all that junk would go away.  (I recently tried Components.utils.stack, it might work here as well, but I don't understand the issues it might raise).

Detecting "new Function()" and browser generated event handlers is less clear and I currently only have heuristics that seem to work.

Comment 10

[John J. Barton]

(In reply to comment #4)
> Here is a sketch of the proposed changes to the hook:
> 
> 1. The hook will only be called once per compilation. It would not be called
> for function and their JSScript objects that are defined by the compiled script
> or function.

Ok, would it be called before or after the compilation?  I guess after.

> 
> 2. The api would be extended so an embedding can enumerate the nested functions
> and their scripts defined by the compiled script.

I'm not sure what an embedding is and how a function and script differ. If you are suggesting an extension to jsdIDebuggerService, then the API would be an enumeration would be a jsdIScriptEnumerator right?

> 
> 3. The hook signature would be changed so it would not contain JSFunction.
> Rather it would come an with extra flag parameter describing compilation
> context so an embedding can know that the script was compiled as a consequence
> of eval/Function invocation or as an internal API invocation.

Hmm, again I am missing something since I don't know what hook signature is being discussed or what an embedding is.

> 
> To Mike Moening: would this work for you?
> 

Comment 11

[Igor Bukanov]

(In reply to comment #10)
> > 1. The hook will only be called once per compilation. It would not be called
> > for function and their JSScript objects that are defined by the compiled script
> > or function.
> 
> Ok, would it be called before or after the compilation?  I guess after.

It will be called when the script is fully compiled and is ready for execution. 

> 
> > 
> > 2. The api would be extended so an embedding can enumerate the nested functions
> > and their scripts defined by the compiled script.
> 
> I'm not sure what an embedding is and how a function and script differ.

Consider the following script:

var a = 1;

function X()
{
    function Y();
}

function Z()
{

}

Currently the hook is called 4 times. The first time it will be called for a JSScript instance representing the bytecode of function Y, then for an JSScript instance with bytecode for the function X, then for Z's bytecode and finally for a JSScript that points to the top level script. In the first 3 cases with JSSCript for functions the newScript will also receive a JSFunction parameter that points to a data structure that SM uses to represent the internals of functions. 

It turned out that exposing JSFunction this way prevents certain optimizations. So the idea is to modify the semantics of the hook so it would only be called once for the whole script. But this would prevent an embedding from accessing bytecode for functions defined in the script (X-Y-Z function in the above example) and set, for example, brekpoints there. 

Hence to compensate for that a new API would be provided that allows from JSScript to get all JSScript instances representing functions in the script, something like:

JSScript **nestedScripts = JS_GetNestedScript(cx, sctript);

In the above example it would return JSScript instances for the functions X and Z. From X an embedding can recursively get the JSScript for Y. 

> Hmm, again I am missing something since I don't know what hook signature is
> being discussed or what an embedding is.

The idea is to extend newScript signature with extra flags or enumeration describing the nature of script.

Comment 12

[John J. Barton]

Ok let me ask a different way: how is the proposal related to jsdIDebuggerService?  That is the API used by Venkman and Firebug. It doesn't have a function called newScript.

John.

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Ok let me ask a different way: how is the proposal related to
> jsdIDebuggerService?

jsdIDebuggerService is implemented without using newScriptHook. The only place where newScriptHook in the browser-related code is xpconnect profiler according to:

http://lxr.mozilla.org/seamonkey/ident?i=JS_SetNewScriptHook

Comment 14

[John J. Barton]

Thanks Igor. I'll look into implementing something similar as an addition to jsdIDebuggerService. 
John. 

Comment 15

[John J. Barton]

I have a prototype implementation using the following API:

interface jsdIDebuggerService2 : jsdIDebuggerService 
{
    /**
     * Called when a compilation unit is complete and ready to execute.
     */
    attribute jsdICompilationHook    compilationHook;
	
	/* When onCompilation is called, you can enumerate the compiled scripts. Any
	 * other time you should get zero callbacks.
	 */
	void enumerateCompiledScripts (in jsdIScriptEnumerator enumerator);
};

interface jsdICompilationHook : nsISupports
{
	/* script tags, included .js files */
	const unsigned long SCRIPT_TAG_COMPILATION = 1;
	/* eval(), new Function() */
	const unsigned long EVAL_COMPILATION = 2;
	/* event scripts */
	const unsigned long GENERATED_COMPILATION = 3;

	void onCompilation (in jsdIStackFrame frame, in unsigned long type, in jsdIScript outerScript);
};

There are corresponding APIs for C calls.

The implementation could eventually cause this bug to be FIXED: Mike and Igor shall I use this bug report to (eventually) submit and review the patch or use another one?

Comment 16

[Mike Moening]

Is this an implementation of patch described in comment #4?
I need to see a patch for the spidermonkey API.  Not JSD.
The underlying framework needs patching first...
Can you provide this?

Comment 17

[John J. Barton]

The implementation is similar to the outline in comment #4.

I don't know what the spidermonkey API is, but if you tell me which header file you are interested in I can post the additions. 

The files I've modified:
M js\jsd\idl\jsdIDebuggerService.idl
M js\jsd\jsd.h
M js\jsd\jsd_high.c
M js\jsd\jsd_scpt.c
M js\jsd\jsd_xpc.cpp
M js\jsd\jsd_xpc.h
M js\jsd\jsdebug.c
M js\jsd\jsdebug.h
M js\src\jsdbgapi.cpp
M js\src\jsdbgapi.h
M js\src\jsobj.cpp
M js\src\jsprvtd.h
M js\src\jsscript.cpp
M js\src\jsscript.h

Comment 18

[Mike Moening]

Any .c or .h file you listed that doesn't start with "jsd_" in the front part of base spidermonkey.

Just post a patch to this bug.

Comment 19

[John J. Barton]

Ok, the non-jsd part is the easy part. But I've got to read up on hg to figure out how to create the patch.

The jsd part is hard because during compilation the call stack seems to be in a state that jsd is not prepared to deal with. It's like the frame is on the stack but the script can't be set for the jsd version of the frame (since it is not yet executing). This explains why onScriptCreated does not have a frame argument. I may have to give up on having a frame argument to onCompilation()...

Comment 20

[John J. Barton]

Attachment: hg export 97048115a5e4

prototype patch against trunk around Oct 8

Comment 21

[John J. Barton]

The patch on comment 20 is for discussion and suggestions. Some of the problems:
 1) I still don't understand how to get the caller's stack frame to JS through jsd.
 2) Not all of the compile points are instrumented.
 3) I don't know how to obtain more information about the compilation unit, eg browser generated, moz script load, etc.

These could affect the API if I can't resolve them.

Comment 22

[Rob Campbell [:rc] (:robcee)]

Found this during triage. There's been no discussion on this for a year. Is it still valid? Is there still work to be done?

Comment 23

[Mike Moening]

Yes still valid and yes there work to be done.

The original bug got a little muddled (no offence John B) by lots of talk about JSD (which is a possible downstream consumer of the fix) which threw the topic off quite a bit.

Igor's comment in #4 and #11 lays things out pretty simply.

My only question about your proposal:

As a debugger is stepping though lines of code where new functions are being created would the debugger have to constantly call your new API JS_GetNestedScript() to see them?
How would the debugger see additions/deletions of dynamic scripts?

Igor, lets flush our your idea.  I think it's solid.

I personally would like to see a patch generated by Igor to his original ideas proposed here.

Igor, could you do this?

Mike M.

Comment 24

[John J. Barton]

I have worked on extending jsd to support compilation units (my version of this idea). The code works ok but my implementation requires adding a couple of lines where code is compiled. Since I don't even know all of the places I set it aside until I learn more.

A solution to this problem that was usable by Firebug would have a big impact on Firebug source and performance.

Comment 25

[John J. Barton]

I've moved my work on to 
 Bug 449464 -  Implement jsdICompilationUnit to extend jsd to include information on the compilation unit structure

Sorry to side track this bug.