449464 - Implement jsdICompilationHook to extend jsd to include information on the compilation unit structure

Status: RESOLVED INVALID

(Core :: JavaScript Engine, enhancement)

Description

[John J. Barton]

Currently Firebug has to infer by analyzing the call stack where a jsdIScript object came from. Some of the categories it knows about are:
  top-level: the outer script in an HTML or .js file.
  nested: a script compiled within top-level
  eval-level: the outer script in an eval() call
  eval-nested: a script compiled within eval()
  event: a browser-generated event script
  inline: javascript on a DOM event handler in HTML
  system: wrapper scripts created by Firefox.
The inference mechanism is complex and unnecessary given that Firefox knows the context of the scripts.

Along one path we could create an object type to mean "compilation unit" and put the type info into that object, together with things like the list of scripts compiled for the unit.

Along a different path we could just add a byte to the jsdIScript and set it on calls to create scripts.

We could compromise and add a pointer to jdsIScript that points to jsdICompilationUnit and put the byte there, with the plan to soup up the compilation unit over time.

This change involves more that just jsd, since all of the callers of the Javascript engine would need a small upgrade and the API from these callers would need to be expanded.

Comment 1

[John J. Barton]

I have a prototype patch that implements jsdICompilationUnit that I am working on.

Comment 2

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

feel free to paste it into the attachments for preliminary review and safe-keeping.

Comment 3

[John J. Barton]

More justification prose:

Firebug uses jsdIDebuggerService in a very hacky and bizarre way. To support eval() and browser-generated event listener debugging, Firebug builds a list of the jsdIScripts for each "compilation unit". But there are no APIs for the compilation unit, so we fake it by looking at the call stack, storing meta data, and passing control back into Firebug via extra non-user-set breakpoints. This code is exotic and can be broken by unrelated changes, eg to the call stack structure. We can't support |new Function()| or some kinds of dynamic code injection with |document.write()|. We believe the performance of Firebug is in the absence of breakpoints is dominated by this funky code.

A new API can solve these problems. Based on the idea in  Bug 379410 -  debugger script detection using parent JSScript*, we can create a new jsdICompilationHook that is called after every Javascript compilation. In the hook the you can enumerate all of the jsdIScript objects created by this compilation. 

In addition to solving some major problems in Firebug, this solution would also allow:
  * Debugging of all JS code, eg HTML embedded JS.
  * Give correct line numbers in eval() and document.write() code.
(see http://groups.google.com/group/mozilla.dev.platform/browse_frm/thread/923603bca1588ca1/68bff3011a712d10?lnk=gst&q=document.write#68bff3011a712d10)

Comment 4

[John J. Barton]

Attachment: partial implementation

I need feedback on the this API

Comment 5

[timeless]

Comment on attachment 415501 [details] [diff] [review]
partial implementation

Please lose this totally useless comment:
>+/*
>+* Add support for compilation units
>+* { 0x13da23ef, 0x8787, 0x4dae, \
>+  { 0xb0, 0xe1, 0x55, 0x1d, 0x1a, 0x47, 0xd8, 0x21 } }
>+*/

iirc jsdIDebuggerService isn't frozen. I'm pretty sure I'm willing to just force people to rebuild their code with two jsdIDebuggerService iids (or use 'in' with js)

>+[scriptable, uuid(13da23ef-8787-4dae-b0e1-551d1a47d821)]
>+interface jsdIDebuggerService2 : jsdIDebuggerService 
>+{

I'm not quite sure I like this definition, actually, I'm certain I don't.

The name is also bad.
>+    /**
>+     * Called when a compilation unit is complete and ready to execute.
>+     */
>+    attribute jsdICompilationHook    compilationHook;

trailing whitespace:
and it's a tab! never use tabs. never.
>+	
>+	/* When onCompilation is called, you can enumerate the compiled scripts. Any
>+	 * other time you should get zero callbacks.

The second sentence doesn't make sense to me.

Can someone do:

jsdS.cH = function () {
 jsdS.enumerateCompiledScripts(er);
 jsdS.enumerateCompiledScripts(er);
}

>+	 */
>+	void enumerateCompiledScripts (in jsdIScriptEnumerator enumerator);

Bogus comment:
>+ * Hook instances of this interface up to the
>+ * jsdIDebuggerService2::compilationHook

Interfaces should not really be aware of their consumers.

Lose the iid expansion:
>+ * { 0x77db18bc, 0x36c6, 0x4506, \
>+  { 0x8d, 0x8a, 0x36, 0xa6, 0xbd, 0xa1, 0x71, 0x65 } }
>+ */
>+[scriptable, uuid(77db18bc-36c6-4506-8d8a-36a6bda17165)]
>+interface jsdICompilationHook : nsISupports
>+{
again: don't ever use tabs.
>+	/* script tags, included .js files */

provide a javadoc explanation of what frame and outerScript are.
>+	void onCompilation (in jsdIStackFrame frame, in unsigned long type, in jsdIScript outerScript);

I think that I'd expect the jsdIScript to be passed to enumerateCompiledScripts assuming it's related.

things should really line up, you can use your favorite web browser to look (again, this is damage from using tabs). and please note that there should be some space between ';' and '/':
>     JSCList     hooks;      /* JSCList of JSDExecHooks for this script */
>+    JSDScript*	compiledScripts;/* JSDScripts from same compilation unit */

>-jsd_NewThreadState(JSDContext* jsdc, JSContext *cx);
>+jsd_NewThreadState(JSDContext* jsdc, JSContext *cx, JSStackFrame *caller);

hrm:
>+    JS_SetCompilationHook(jsdc->jsrt, jsd_CompilationHook, jsdc);

Odd.

please add the missing space before 'cx':
>-    if(hook && NULL != (jsdthreadstate = jsd_NewThreadState(jsdc,cx)))
>+    if(hook && NULL != (jsdthreadstate = jsd_NewThreadState(jsdc,cx, NULL)))

as in:
>-    if(hook && NULL != (jsdthreadstate = jsd_NewThreadState(jsdc, cx)))
>+    if(hook && NULL != (jsdthreadstate = jsd_NewThreadState(jsdc, cx, NULL)))

this feels odd:
> jsd_InitScriptManager(JSDContext* jsdc)
>+    jsdc->lastCompiledScript = NULL;
>     return (JSBool) jsdc->scriptsTable;

don't include trailing whitespace:
>+    jsdscript->compiledScripts = jsdc->lastCompiledScript;
>+    jsdc->lastCompiledScript = jsdscript;   
I'm worried that something could cause jsdc->lastCompiledScript to change inside this blob...

>+    if( hook )
>+    {
>+        jsdthreadstate = jsd_NewThreadState(jsdc, cx, caller);  /* may be null */

>+        hook(jsdc, jsdthreadstate, jsdOuterScript, compilationType, callerdata);
>+    }
>+    jsdc->lastCompiledScript = NULL;  /* you had your chance when the hook was called */

>\ No newline at end of file

please leave a newline at the end of file.


>-jsd_NewThreadState(JSDContext* jsdc, JSContext *cx )
>+jsd_NewThreadState(JSDContext* jsdc, JSContext *cx,  JSStackFrame *caller)
> {
>     JSDThreadState* jsdthreadstate;
>     JSStackFrame *  iter = NULL;
>     JSStackFrame *  fp;

i don't see any reason to move these declarations outside the loop.
I'd much rather you not do something like this. (my tree converts jsd to c++ anyway)
>+    JSScript* script;
>+    jsuword  pc;

>     while( NULL != (fp = JS_FrameIterator(cx, &iter)) )
>     {
just leave the decl here
>-        JSScript* script = JS_GetFrameScript(cx, fp);
>-        jsuword  pc = (jsuword) JS_GetFramePC(cx, fp);
>+        if (caller && (fp != caller)) 
>+        }
and split the assignment to here:
>+        script = JS_GetFrameScript(cx, fp);
>+        pc = (jsuword) JS_GetFramePC(cx, fp);

i definitely don't want this (note that you broke the indentation):
>-static jsdService   *gJsds       = 0;
>+static jsdService2   *gJsds       = 0;

jsdService is the name of the impl, its name shouldn't change just because someone has added an interface to it.

lose the useless comment:
>+/* The type of this function must agree with the corresponding one in jsdebug.h */
>+jsds_CompilationHookProc (JSDContext* jsdc, 

lose the useless comment:
>+    /* retrieve the function pointer from the global where we stored it earlier */

lose the useless comment:
>+    if (!hook) /* nobody cares, just exit */
>+       return;

yuck, no no no.
> jsdService *
> jsdService::GetService ()
> {
>     if (!gJsds)
>-        gJsds = new jsdService();
>-        
>+        gJsds = new jsdService2();
>     NS_IF_ADDREF(gJsds);
>     return gJsds;
> }

no no no no no. we do not add extra classes even if we add interfaces.
>+NS_IMPL_ISUPPORTS_INHERITED1(jsdService2, jsdService, jsdIDebuggerService2) // this, superclass, added ifaces
>+
>+NS_IMETHODIMP
>+jsdService2::SetCompilationHook (jsdICompilationHook *aHook)
>+{    
lose the useless comment:
>+    mCompilationHook = aHook;  /* store the hook in the global for use later */

The logic here is actually fairly bad. and the comment is bad.
>+    if (aHook) /* then we stored the hook in global, set the function that will pull it back out */
>+        JSD_SetCompilationHook (cx, jsds_CompilationHookProc, NULL);
>+    else
>+        JSD_SetCompilationHook (cx, NULL, NULL);

Err.
>+/* Same code at EnumerateScripts, but calls JSD_IterateCompiledScripts not JSD_IterateScripts */
>+jsdService2::EnumerateCompiledScripts (jsdIScriptEnumerator *enumerator)

>+    while((script = JSD_IterateCompiledScripts(mCx, &iter))) {
>+        rv = enumerator->EnumerateScript (jsdis);

>+jsdService2::GetCompilationHook (jsdICompilationHook **aHook)

You're shadowing a function by changing only its return type, this is awful.
>+jsdService2 *
>+jsdService2::GetService ()
>+{
>+    if (!gJsds)
>+        gJsds = new jsdService2();
>+        
>+    NS_IF_ADDREF(gJsds);
>+    return gJsds;
>+}
bogus change:
>-  private:
>+  protected:

This is not how we extend things. We don't need a new class. And at this point I'm certain I don't want a new interface.
>+class jsdService2 : public jsdService,
>+                    public jsdIDebuggerService2



we don't use this style, don't add it:
>+/* -------------------------------------------------------------------------------*/

Please write comments to match the content/style of similar comments in the file into which you're adding them
>+/*
>+* Declaration of callback type for notification of compilation of a set of scripts.
>+*  
>+* callback called after the outer script is complete and ready to execute.
>+* 'callerdata' is what was passed to JSD_SetCompilationHook to set the hook.
>+* (this is the C interface)

This is not a useful comment:
>+/* While the compilation hook is running you can get the inner scripts */
>+
>+extern JSD_PUBLIC_API(JSDScript*)
>+JSD_IterateCompiledScripts(JSDContext* jsdc, JSDScript **iterp);

>+/*
>+* Set a hook to be called when compilation unit is created
>+* 'callerdata' can be whatever you want it to be.
>+*/
>+extern JSD_PUBLIC_API(JSBool)
>+JSD_SetCompilationHook(JSDContext* jsdc, JSD_CompilationHookProc hook, void* callerdata);
>+
>+/*
>+* Get the current script hook.
>+*/
>+extern JSD_PUBLIC_API(JSBool)
>+JSD_GetCompilationHook(JSDContext* jsdc, JSD_CompilationHookProc* hook, void** callerdata);

--------------------------
Please do everyone a favor and provide a distinct attachment for js/src from js/jsd -- no one from spidermonkey would read this far to get to this point.

>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp
>+JS_PUBLIC_API(void)
>+JS_SetCompilationHook(JSRuntime *rt, JSCompilationHook hook, void *closure)
>+{
>+    rt->globalDebugHooks.compilationHook = hook;
>+    rt->globalDebugHooks.compilationHookData = closure;
>+}
>+/***************************************************************************/

>+extern JS_PUBLIC_API(void)
>+JS_SetCompilationHook(JSRuntime *rt, JSCompilationHook hook, void *closure);
>+/************************************************************************/

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
>@@ -1523,17 +1523,21 @@ obj_eval(JSContext *cx, JSObject *obj, u
>     ok = js_CheckPrincipalsAccess(cx, scopeobj, principals,
>                                   cx->runtime->atomState.evalAtom);
>     if (ok)
>+    {
>+		/* Tell the debugger about this compilationUnit */
>+		js_CallCompilationHook(cx, caller, script, JSD_EVAL_COMPILATION);
>         ok = js_Execute(cx, scopeobj, script, callerFrame, JSFRAME_EVAL, rval);
>+    }

>diff --git a/js/src/jsparse.cpp b/js/src/jsparse.cpp
>@@ -1019,16 +1019,18 @@ JSCompiler::compileScript(JSContext *cx,
>   out:
>     JS_FinishArenaPool(&codePool);
>     JS_FinishArenaPool(&notePool);
>+
>+    js_CallCompilationHook(cx, callerFrame, script, JSD_SCRIPT_TAG_COMPILATION);

>diff --git a/js/src/jsprvtd.h b/js/src/jsprvtd.h
>+/* called just after the outer script is compiled but before it is executed */
>+typedef void
>+(* JSCompilationHook)(JSContext	   *cx,
>+                      JSStackFrame *caller, 
>+                      JSScript     *script,
>+                      uintN        unitType, 
>+                      void         *callerdata);
>+
>+/* Compilation Unit type numbers, should exactly match values in jsdIDebuggerService.idl */
>+	
>+#define JSD_SCRIPT_TAG_COMPILATION    1	/* script tags, included .js files */
>+#define JSD_EVAL_COMPILATION	      2	/* eval(), new Function() */
>+#define JSD_GENERATED_COMPILATION     3	/* event scripts */

This is a vaguely public structure, additions should be to the end of the structure, not the middle. We don't need to encourage data corruption:
> typedef struct JSDebugHooks {
>     JSTrapHandler       interruptHandler;
>     void                *interruptHandlerData;
>     JSNewScriptHook     newScriptHook;
>     void                *newScriptHookData;
>     JSDestroyScriptHook destroyScriptHook;
>     void                *destroyScriptHookData;
>+    JSCompilationHook	compilationHook;
>+    void                *compilationHookData;
>     JSTrapHandler       debuggerHandler;
>     void                *debuggerHandlerData;
>     JSSourceHandler     sourceHandler;
>     void                *sourceHandlerData;
>     JSInterpreterHook   executeHook;
>     void                *executeHookData;
>     JSInterpreterHook   callHook;
>     void                *callHookData;

>diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp
>+JS_FRIEND_API(void)
>+js_CallCompilationHook(JSContext *cx, JSStackFrame *caller, JSScript *script, uintN compilationType)
>+{
>+    JSCompilationHook hook;
>+
>+    hook = cx->debugHooks->compilationHook;
>+    if (hook) {
>+        JS_KEEP_ATOMS(cx->runtime);
>+        hook(cx, caller, script, compilationType, cx->debugHooks->compilationHookData); 
>+        JS_UNKEEP_ATOMS(cx->runtime);
>+    }
>+}

>diff --git a/js/src/jsscript.h b/js/src/jsscript.h
>+extern JS_FRIEND_API(void)
>+js_CallCompilationHook(JSContext *cx, JSStackFrame *caller, JSScript *script, uintN compilationType);

Comment 6

[:Gijs (out for now; he/him)]

(In reply to comment #4)
> I need feedback on the this API.

OK. From a jsdICompilationUnit (or whatever it ends up being called), I'd like to get script/page source. I haven't looked if SpiderMonkey even has it, but if it doesn't, it should be providing some identifier that lets me get it. And I don't mean the discompiled blurbs, I can get those from the jsdIScripts - I mean the actual written JS. :-)

Further, like timeless, I think it should just extend jsdDebuggerService. It's fine to change that interface (and the class).

Then, I'm really unclear on this enumerate() API and how it solves the eval() usecase. Particularly, say I have a file with 3 functions, one of which uses eval. AIUI, currently, that will have onScriptCreated called 4 times (once for each function, once for the toplevel) when the script is read/compiled, and then maybe again when the eval() actually runs (I'm not actually sure). What happens in your case, and why is it better?

Fourth, can I get a list of compilationunits that have already been created? I'd like to avoid having to run a component from the beginning of time (and then still missing stuff that also runs at the beginning of time), we're just getting away from that thanks to some work from Wladimir Palant...

Fifth, can I have a way to get from a jsdIScript to a compilationunit (ie, all the other scripts for that file? Almost all the other JSD APIs still get me those, and I'll want to know which compilationunit it belongs to. I'm assuming this is easier (and more efficiently) to do on the JSD side than on the UI side, and I think almost any consumer is going to want this.

I think this is about it, if I think of anything else I'll shout. :-)

Comment 7

[:Gijs (out for now; he/him)]

Oh, no, one other thing... what takes care about these units being destroyed? IIRC there's an onScriptDestroyed hook on JSD for the jsdIScripts, which means the JSD consumer can throw its references away. What about this compilation unit thing? When does that go away, whose responsibility is it to get rid of it, and if it's the responsibility of the consumer, how does it hang on to the jsdIScripts - do I have a guarantee those pointers won't go stale, and if I do, how do I decide to get rid of my compilation unit so I free up all the relevant memory?

Comment 8

[:Gijs (out for now; he/him)]

Gah, I'm spamming the bug by now - sorry. I just realized that the jsdICompilationUnit.... actually doesn't exist (despite the bug summary)? Why not? Why a complicated API with an enumerate method that does nothing if it's not called at the right time? This seems to mean that the answer to a bunch of my questions is "you can't do that", which to me kind of defeats the entire point of the API...

I can't rely on the compilationhook to get me all the jsdscripts I want. I can't go from a jsdScript to all the other ones that belong to the same file ("compilation unit"), I can't identify a compilation unit in any way, so I (/Firebug) will still have to do my/its own (unreliable) bookkeeping with respect to where the jsdScripts come from (which seems to me to be part of what you complain about in comment #3).

Which makes me wonder. Can you provide a practical example, and explain to me how the current JSD hooks (functionHook, callHook, onScriptCreated and friends) are not enough, but your new API saves the day? I read your prose in comment #3, but it did not elucidate this point for me.

Comment 9

[John J. Barton]

(In reply to comment #5)
> (From update of attachment 415501 [details] [diff] [review])

Thanks for your detailed comments. I'll take up the API points first and fix the format later today.
...

> I'm not quite sure I like this definition, actually, I'm certain I don't.
> 
> The name is also bad.
> >+    /**
> >+     * Called when a compilation unit is complete and ready to execute.
> >+     */
> >+    attribute jsdICompilationHook    compilationHook;

How about: 
  /*   Called sometime after some related jsdIScript objects are created and before any one of them is executed. Typically all of the jsdIScript objects 
for a file will be in one call */

Any suggestion for a better name? I'm not sure how to guess since I don't know what your issue is with this name.


> >+	/* When onCompilation is called, you can enumerate the compiled scripts. Any
> >+	 * other time you should get zero callbacks.
> 
> The second sentence doesn't make sense to me.

Yep. How about "Any other time the call will return without calling the jsdIScriptEnumerator".

> 
> Can someone do:
> 
> jsdS.cH = function () {
>  jsdS.enumerateCompiledScripts(er);
>  jsdS.enumerateCompiledScripts(er);
> }

Yes.

...
> provide a javadoc explanation of what frame and outerScript are.
> >+	void onCompilation (in jsdIStackFrame frame, in unsigned long type, in jsdIScript outerScript);
> 
> I think that I'd expect the jsdIScript to be passed to enumerateCompiledScripts
> assuming it's related.

The issue here is that we want to know what jsdIScript represents the file-scope global initializer. By passing this as an argument to the onCompilation() we avoid having to store it in the engine. 

...
...
> hrm:
> >+    JS_SetCompilationHook(jsdc->jsrt, jsd_CompilationHook, jsdc);
> 
> Odd.

I'm uncertain what you mean here.

...

> 
> this feels odd:
> > jsd_InitScriptManager(JSDContext* jsdc)
> >+    jsdc->lastCompiledScript = NULL;
> >     return (JSBool) jsdc->scriptsTable;

This initialization is not essential.  

...
> I'm worried that something could cause jsdc->lastCompiledScript to change
> inside this blob...
> 
> >+    if( hook )
> >+    {
> >+        jsdthreadstate = jsd_NewThreadState(jsdc, cx, caller);  /* may be null */
> 
> >+        hook(jsdc, jsdthreadstate, jsdOuterScript, compilationType, callerdata);
> >+    }
> >+    jsdc->lastCompiledScript = NULL;  /* you had your chance when the hook was called */

I don't know how to prevent such a change. I could store the value before the blob and add an ASSERT to test that it did not change.

...
...
> The logic here is actually fairly bad. and the comment is bad.
> >+    if (aHook) /* then we stored the hook in global, set the function that will pull it back out */
> >+        JSD_SetCompilationHook (cx, jsds_CompilationHookProc, NULL);
> >+    else
> >+        JSD_SetCompilationHook (cx, NULL, NULL);

I don't understand what to do other than remove the comment.

...

Comment 10

[John J. Barton]

(In reply to comment #6)
> (In reply to comment #4)
> > I need feedback on the this API.
> 
> OK. From a jsdICompilationUnit (or whatever it ends up being called), I'd like
> to get script/page source. I haven't looked if SpiderMonkey even has it, but if
> it doesn't, it should be providing some identifier that lets me get it. And I
> don't mean the discompiled blurbs, I can get those from the jsdIScripts - I
> mean the actual written JS. :-)

I think that identifier should be jsdIScript.fileName. 


> 
> Further, like timeless, I think it should just extend jsdDebuggerService. It's
> fine to change that interface (and the class).

Ok I'll do that later today.

> 
> Then, I'm really unclear on this enumerate() API and how it solves the eval()
> usecase. Particularly, say I have a file with 3 functions, one of which uses
> eval. AIUI, currently, that will have onScriptCreated called 4 times (once for
> each function, once for the toplevel) when the script is read/compiled, and
> then maybe again when the eval() actually runs (I'm not actually sure). What
> happens in your case, and why is it better?

For eval(), onScriptCreated will be called as you say 4 times. That part will not change.  After the 4th call and before the outer or top-level or file-scope-initializer function is called, onCompilation() will be called if this hook is installed.

The new scheme is better is because 1) 1 call instead of 4, 2) the related scripts are provided together, 3) you know that the compilation of the unit is complete,  4) you know the compilation is an eval(), 5) you have the stack frame so you know what code triggered the eval().

> 
> Fourth, can I get a list of compilationunits that have already been created?

No.

> I'd like to avoid having to run a component from the beginning of time (and
> then still missing stuff that also runs at the beginning of time), we're just
> getting away from that thanks to some work from Wladimir Palant...

I think it is better to force the client (Firebug/Venkman) to store the information that they need rather than forcing jsd to store the information, given that jsd does not know what you need.

On the other hand we could think of a model where the compilation unit is threaded or tagged onto the jsdIScript objects. 

A threaded solution would add a field to jsdIScript say 
   jsdICompilationUnit unit;
and the object would say give you an iterator over the jsdIScripts in the compilation unit. The onCompilation would deliver the "unit" at the end of compilation. This is a more static and nicer API, but it is also larger and more complex with more places to break and more memory overhead.

A tagged solution would add a field to jsdIScript say
   int compilationUnitTag; 
or what ever the right |int| type would be. The value would the same in all scripts from the same compilation unit. This solution is compact but less scalable because it requires you to scan all scripts to assemble the ones in the same unit.

> 
> Fifth, can I have a way to get from a jsdIScript to a compilationunit (ie, all
> the other scripts for that file? Almost all the other JSD APIs still get me
> those, and I'll want to know which compilationunit it belongs to. I'm assuming
> this is easier (and more efficiently) to do on the JSD side than on the UI
> side, and I think almost any consumer is going to want this.

Yes, that is what the enumerateCompiledScripts() does. However in this implementation you can only call it while in onCompilation(). The consumer would need to store the values it wants because the engine is not going to remember it.

This implementation has a list threaded through the jsdIScript objects that is not surfaced to the interface. The head of the list is in the JSDContext and it is not surfaced.  I was really just going for a minimal API to maximize the chance of getting in.

Comment 11

[John J. Barton]

(In reply to comment #7)
> Oh, no, one other thing... what takes care about these units being destroyed?
> IIRC there's an onScriptDestroyed hook on JSD for the jsdIScripts, which means
> the JSD consumer can throw its references away. What about this compilation
> unit thing? When does that go away, whose responsibility is it to get rid of
> it, and if it's the responsibility of the consumer, how does it hang on to the
> jsdIScripts - do I have a guarantee those pointers won't go stale, and if I do,
> how do I decide to get rid of my compilation unit so I free up all the relevant
> memory?

Good question...in the current implementation the consumer would have to mark off the dead jsdIScripts when onScriptDestroyed is called and clean up when their own list was empty. Firebug has such a list for each compilation unit, but does not bother with clean up. In some more dynamic future time it may be important.

Generally jsdIScript are shells around actual scripts. The actual script can go away and jsdIScript is fine, it just answers false to isValid().

Comment 12

[John J. Barton]

(In reply to comment #8)
> Gah, I'm spamming the bug by now - sorry. I just realized that the
> jsdICompilationUnit.... actually doesn't exist (despite the bug summary)? Why
> not? Why a complicated API with an enumerate method that does nothing if it's
> not called at the right time? This seems to mean that the answer to a bunch of
> my questions is "you can't do that", which to me kind of defeats the entire
> point of the API...

Sorry about the naming thing. 

As I explained on the other comment, the reason is simply to minimize the API. 
I was trying to get the information to the consumer and let the consumer build data structures if it needs them.  If you focus on the object system, then yes this API is odd. But if you focus on the dynamics, it's fine: you get the information you need at the time you need it. After that all of the API is up to you: if you want to build a CompilationUnit data structure, go for it.

I'm unclear what you mean by "you can't do that". 

> I can't rely on the compilationhook to get me all the jsdscripts I want. I
> can't go from a jsdScript to all the other ones that belong to the same file
> ("compilation unit"), I can't identify a compilation unit in any way, so I
> (/Firebug) will still have to do my/its own (unreliable) bookkeeping with
> respect to where the jsdScripts come from (which seems to me to be part of what
> you complain about in comment #3).

The proposed API does provide you with all of the jsdIScript objects from the compilation unit. The consumer can build data structures to do all of the things you describe based on information gathered in the compilation hook. Or not, depending on what the consumer decides it needs.


> 
> Which makes me wonder. Can you provide a practical example, and explain to me
> how the current JSD hooks (functionHook, callHook, onScriptCreated and friends)
> are not enough, but your new API saves the day? I read your prose in comment
> #3, but it did not elucidate this point for me.

Take a look at Firebug's onScriptCreated implementation:
http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#1494

I guess it will be difficult for you to figure out that Firebug's onScriptCreated() function attempts to partially emulate the API proposed here.  I failed to deal with new Function() and document.write() despite trying quite hard.

Let me answer your question with some questions: using the current API how can we:
  1) determine the jsdIScripts comprising a compilation unit?
  2) know when the last script from a compilation unit has been compiled?
  3) know which is the outer or top-level script?
  4) know what code triggers a compilation?
  5) know what know of trigger caused the compilation?
These are questions the new API answers.

Comment 13

[:Gijs (out for now; he/him)]

(In reply to comment #10)
> I think that identifier should be jsdIScript.fileName. 

What will that be for eval calls?

> > Then, I'm really unclear on this enumerate() API and how it solves the eval()
> > usecase. Particularly, say I have a file with 3 functions, one of which uses
> > eval. AIUI, currently, that will have onScriptCreated called 4 times (once for
> > each function, once for the toplevel) when the script is read/compiled, and
> > then maybe again when the eval() actually runs (I'm not actually sure). What
> > happens in your case, and why is it better?
> 
> For eval(), onScriptCreated will be called as you say 4 times. That part will
> not change.  After the 4th call and before the outer or top-level or
> file-scope-initializer function is called, onCompilation() will be called if
> this hook is installed.
> 
> The new scheme is better is because 1) 1 call instead of 4, 2) the related
> scripts are provided together, 3) you know that the compilation of the unit is
> complete,  4) you know the compilation is an eval(), 5) you have the stack
> frame so you know what code triggered the eval().

I'm confused. Where is the eval call represented? It's none of those 4 onScriptCreated calls (3 functions + 1 toplevel), and I'm pretty sure the onCompilation() call doesn't know about it either - the eval() won't execute until the function it's in runs. Are you saying js already creates a script for it when it gets parsed? How sure are you that holds true?

When actually reading your code, it seems you issue a separate call to js_CallCompilationHook for the eval - not putting it together with the other scripts at all. And so it's still not possible to tell where an eval call came from, is it? :-\

As for your points:
1) But I'd need to then get the 4 jsdScripts, which makes this be 6+ calls rather than 1 (1 for the onCompilation, 1 for calling enumerate, 4 to get each script from the enumerator). That'll just be slower, because they'll all cross xpconnect boundaries (for Firebug and Venkman, anyway).
2+3) Agreed, that would be nice to have.
4+5) Merm, I guess. I think you basically don't care about eval() unless you're debugging the relevant code, in which case you also already know where the eval() came from.
 
> > Fourth, can I get a list of compilationunits that have already been created?
> 
> No.

:-(

> > I'd like to avoid having to run a component from the beginning of time (and
> > then still missing stuff that also runs at the beginning of time), we're just
> > getting away from that thanks to some work from Wladimir Palant...
> 
> I think it is better to force the client (Firebug/Venkman) to store the
> information that they need rather than forcing jsd to store the information,
> given that jsd does not know what you need.
> 
> On the other hand we could think of a model where the compilation unit is
> threaded or tagged onto the jsdIScript objects. 
> 
> A threaded solution would add a field to jsdIScript say 
>    jsdICompilationUnit unit;
> and the object would say give you an iterator over the jsdIScripts in the
> compilation unit. The onCompilation would deliver the "unit" at the end of
> compilation. This is a more static and nicer API, but it is also larger and
> more complex with more places to break and more memory overhead.
> 
> A tagged solution would add a field to jsdIScript say
>    int compilationUnitTag; 
> or what ever the right |int| type would be. The value would the same in all
> scripts from the same compilation unit. This solution is compact but less
> scalable because it requires you to scan all scripts to assemble the ones in
> the same unit.
> 
> > 
> > Fifth, can I have a way to get from a jsdIScript to a compilationunit (ie, all
> > the other scripts for that file? Almost all the other JSD APIs still get me
> > those, and I'll want to know which compilationunit it belongs to. I'm assuming
> > this is easier (and more efficiently) to do on the JSD side than on the UI
> > side, and I think almost any consumer is going to want this.
> 
> Yes, that is what the enumerateCompiledScripts() does. However in this
> implementation you can only call it while in onCompilation(). The consumer
> would need to store the values it wants because the engine is not going to
> remember it.

No, I wanted to get the compilation unit from a jsdIScript, not the other way around. So basically, either of the two approaches you outlined in reply to my other question.

(In reply to comment #11)
> (In reply to comment #7)
> > Oh, no, one other thing... what takes care about these units being destroyed?
> <snip>
So this doesn't actually apply if there's no real object holding these things. It might be important in your implementation, but not for a consumer. :-)



(In reply to comment #12)
> (In reply to comment #8)
> > Gah, I'm spamming the bug by now - sorry. I just realized that the
> > jsdICompilationUnit.... actually doesn't exist (despite the bug summary)? Why
> > not? Why a complicated API with an enumerate method that does nothing if it's
> > not called at the right time? This seems to mean that the answer to a bunch of
> > my questions is "you can't do that", which to me kind of defeats the entire
> > point of the API...
> 
> Sorry about the naming thing. 
> 
> As I explained on the other comment, the reason is simply to minimize the API. 
> I was trying to get the information to the consumer and let the consumer build
> data structures if it needs them.  If you focus on the object system, then yes
> this API is odd. But if you focus on the dynamics, it's fine: you get the
> information you need at the time you need it.

OK, so it's this bit I disagree with. :-)
I mostly use Venkman to debug add-on or browser/app JS. In this case, the JS is long loaded by the time Venkman comes around the block, so the API doesn't help me if I have no way of getting to those "compilation units" and so on. Similarly, for webpages, it'd force your add-on to have been watching (causing perf impact!) when the page was loading, in order for it to work well after it has done so.

> I'm unclear what you mean by "you can't do that". 

I meant that, as you once affirmed, and once apparently didn't understand my question (my fault probably...), I couldn't do the things I wanted to do with this API. :-)

> > I can't rely on the compilationhook to get me all the jsdscripts I want. I
> > can't go from a jsdScript to all the other ones that belong to the same file
> > ("compilation unit"), I can't identify a compilation unit in any way, so I
> > (/Firebug) will still have to do my/its own (unreliable) bookkeeping with
> > respect to where the jsdScripts come from (which seems to me to be part of what
> > you complain about in comment #3).
> 
> The proposed API does provide you with all of the jsdIScript objects from the
> compilation unit. The consumer can build data structures to do all of the
> things you describe based on information gathered in the compilation hook. Or
> not, depending on what the consumer decides it needs.

OK. But it only does that if I have the hook registered at the Right Time. I don't think that's as trivial as you suggest.

> > Which makes me wonder. Can you provide a practical example, and explain to me
> > how the current JSD hooks (functionHook, callHook, onScriptCreated and friends)
> > are not enough, but your new API saves the day? I read your prose in comment
> > #3, but it did not elucidate this point for me.
> 
> Take a look at Firebug's onScriptCreated implementation:
> http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#1494
> 
> I guess it will be difficult for you to figure out that Firebug's
> onScriptCreated() function attempts to partially emulate the API proposed here.
>  I failed to deal with new Function() and document.write() despite trying quite
> hard.
> 
> Let me answer your question with some questions: using the current API how can
> we:
>   1) determine the jsdIScripts comprising a compilation unit?
>   2) know when the last script from a compilation unit has been compiled?
>   3) know which is the outer or top-level script?
>   4) know what code triggers a compilation?
>   5) know what know of trigger caused the compilation?
> These are questions the new API answers.

I think 4 and 5 are the same. Then, for 1: the order is known (toplevels come last), 2: see 1, 3: it's marked as such (I don't remember the magic identifier, but there is one, IIRC for the functionName or whatever it is). 4: you're already debugging, otherwise you don't care, typically. :-)

I'll summarize in another comment, just to keep things clear.

Comment 14

[:Gijs (out for now; he/him)]

Here's what I deduce from what you've told me so far. You want an API that:

- Organizes jsdScripts into things that are to do with the same file.
- Lets you know that an eval() script is an eval() script (similar for new Function(), document.write, setTimeout/Interval + String argument, and friends), and where it "came from".

I'm not sure how the two fit together, that is, whether an eval script has to do with an earlier file (obviously the line numbers need magic applied to them, but we also care that eval() was called in a specific file/window, etc.).

I think these are noble aims. However, at the moment I'm very worried about the performance of JSD and associated debuggers. We've already seen issues with Firebug and venkman causing performance issues throughout normal web use. Additionally, we know for Chromebug/Venkman's sake, that we can't rely on scripts always being created after our own component has started up. So, *relying on our JSD consumer code running when the script gets compiled* is not something I want. Your implementation suggestion doesn't deal with this problem, and it additionally gets us two alternate methods to get jsd scripts (onScriptCreated as well as onCompilationHook) As I said in an earlier comment, we just had some great work go into the tree to make jsd scripts be around after the fact. I think we should be building on this when we fix these issues, too.

So, I think your suggestion to tag the scripts is much better. As a consumer, we can keep using onScriptCreated, and gather scripts into arrays/dictionaries for each unique tag, thus creating exactly the same "compilation unit" structures consumers would get from using your approach.

Additionally, I would suggest we could add an API that fetches all the jsdscripts given such a unique tag. This way, jsd consumers won't miss a beat no matter when they get enabled, and it will be clear, if necessary, when you have all the scripts for a file (so when it's finished compiling). I'm guessing that, since we can find jsd scripts for js scripts, and we should be able to find the parent js script, it should be easy to identify all scripts that are relevant this way, and no extra caching or w/e will be necessary.

Finally, we need a solution for identifying eval. From what I can tell, there's no reason we couldn't add the flags you're using to the onScriptCreated call. Same thing for the stackframe (although that should be null for "normal" script loads, like <script> tags and so on). As timeless said, we can happily extend jsdDebuggerService, so let's. :-)

Comment 15

[John J. Barton]

(In reply to comment #13)
> (In reply to comment #10)
> > I think that identifier should be jsdIScript.fileName. 
> 
> What will that be for eval calls?

Well what ever it is for eval calls. This proposal does not directly address the problem that fileName is not very useful for eval() calls. Rather it provides you with the information that this was an eval() call and hence one can, as Firebug does, invent a better identifier.

I'm not saying that fixing the fileName is not a good thing. I am only saying that fixing the fileName is not what we are doing here.

> 
> > > Then, I'm really unclear on this enumerate() API and how it solves the eval()
> > > usecase. Particularly, say I have a file with 3 functions, one of which uses
> > > eval. AIUI, currently, that will have onScriptCreated called 4 times (once for
> > > each function, once for the toplevel) when the script is read/compiled, and
> > > then maybe again when the eval() actually runs (I'm not actually sure). What
> > > happens in your case, and why is it better?
> > 
> > For eval(), onScriptCreated will be called as you say 4 times. That part will
> > not change.  After the 4th call and before the outer or top-level or
> > file-scope-initializer function is called, onCompilation() will be called if
> > this hook is installed.
> > 
> > The new scheme is better is because 1) 1 call instead of 4, 2) the related
> > scripts are provided together, 3) you know that the compilation of the unit is
> > complete,  4) you know the compilation is an eval(), 5) you have the stack
> > frame so you know what code triggered the eval().
> 
> I'm confused. Where is the eval call represented? It's none of those 4
> onScriptCreated calls (3 functions + 1 toplevel), and I'm pretty sure the
> onCompilation() call doesn't know about it either

The second argument to onCompilation() is an int that tells us that this compile is an eval().

>  - the eval() won't execute
> until the function it's in runs. Are you saying js already creates a script for
> it when it gets parsed? How sure are you that holds true?

When the eval() runs, that is when onScriptCreated() is called, and thus that is also when onCompilation() will be called.

> 
> When actually reading your code, it seems you issue a separate call to
> js_CallCompilationHook for the eval - not putting it together with the other
> scripts at all. And so it's still not possible to tell where an eval call came
> from, is it? :-\

The first argument to onCompilation() is a stack frame. In the eval() case the newest frame will be the eval() call site.

> 
> As for your points:
> 1) But I'd need to then get the 4 jsdScripts, which makes this be 6+ calls
> rather than 1 (1 for the onCompilation, 1 for calling enumerate, 4 to get each
> script from the enumerator). That'll just be slower, because they'll all cross
> xpconnect boundaries (for Firebug and Venkman, anyway).

Yes, the calls are 
  onCompilation,
   enumerateCompiledScripts
    4 x callbacks for jsdIScripts
But if you trace through the code that Firebug runs onScriptCreated() it does a lot of work, including setting breakpoints on PC=0 of the outer (top-level, file-scope initializing) script, halting on that BP and removing the breakpoint. 

> 2+3) Agreed, that would be nice to have.
> 4+5) Merm, I guess. I think you basically don't care about eval() unless you're
> debugging the relevant code, in which case you also already know where the
> eval() came from.

I'm lost about what 4+5 is and I don't know what a 'Merm' is.
...

...

> I mostly use Venkman to debug add-on or browser/app JS. In this case, the JS is
> long loaded by the time Venkman comes around the block, so the API doesn't help
> me if I have no way of getting to those "compilation units" and so on.
> Similarly, for webpages, it'd force your add-on to have been watching (causing
> perf impact!) when the page was loading, in order for it to work well after it
> has done so.

The start up problem is a separate issue. Chromebug is able to get (almost) everything as it happens by starting itself as a high priority command line component. We need to be able to halt JS at any point in the execution, so we need support for debugging at every execution point. Yes it is hard and Venkman may not quite have it all yet, but that should be our goal.

We may still want to be able to enumerate the compilation units from enumeration of the jsdIScripts. But this is a separate desire, a performance or API-niceness requirement, not an essential requirement. I'm fine if we decide that we want to support although ideally I'd like to take it in two bugs to maximize our chances of landing this one.

> 
> > I'm unclear what you mean by "you can't do that". 
> 
> I meant that, as you once affirmed, and once apparently didn't understand my
> question (my fault probably...), I couldn't do the things I wanted to do with
> this API. :-)
> 
> > > I can't rely on the compilationhook to get me all the jsdscripts I want. I
> > > can't go from a jsdScript to all the other ones that belong to the same file
> > > ("compilation unit"), I can't identify a compilation unit in any way, so I
> > > (/Firebug) will still have to do my/its own (unreliable) bookkeeping with
> > > respect to where the jsdScripts come from (which seems to me to be part of what
> > > you complain about in comment #3).
> > 
> > The proposed API does provide you with all of the jsdIScript objects from the
> > compilation unit. The consumer can build data structures to do all of the
> > things you describe based on information gathered in the compilation hook. Or
> > not, depending on what the consumer decides it needs.
> 
> OK. But it only does that if I have the hook registered at the Right Time. I
> don't think that's as trivial as you suggest.

Not trivial, but possible.  Chromebug can do it, in fact I have found startup bugs in Firefox. If we implemented persistent or command line breakpoints in chromebug we could do it routinely.

> 
> > > Which makes me wonder. Can you provide a practical example, and explain to me
> > > how the current JSD hooks (functionHook, callHook, onScriptCreated and friends)
> > > are not enough, but your new API saves the day? I read your prose in comment
> > > #3, but it did not elucidate this point for me.
> > 
> > Take a look at Firebug's onScriptCreated implementation:
> > http://code.google.com/p/fbug/source/browse/branches/firebug1.5/components/firebug-service.js#1494
> > 
> > I guess it will be difficult for you to figure out that Firebug's
> > onScriptCreated() function attempts to partially emulate the API proposed here.
> >  I failed to deal with new Function() and document.write() despite trying quite
> > hard.
> > 
> > Let me answer your question with some questions: using the current API how can
> > we:
> >   1) determine the jsdIScripts comprising a compilation unit?
> >   2) know when the last script from a compilation unit has been compiled?
> >   3) know which is the outer or top-level script?
> >   4) know what code triggers a compilation?
> >   5) know what know of trigger caused the compilation?
> > These are questions the new API answers.
> 
> I think 4 and 5 are the same. 

Analyzing the call stack to determine the cause of the compilation seems unnecessarily complex since the call point of the hook knows the exact answer and it only costs us an int argument.

Knowing that we have a eval() is not enough unless we also fix the jsdIScript.fileName problem. So we need the call stack also.

>Then, for 1: the order is known (toplevels come
> last),

I know this is true for HTML script tags, eval(), and browser generated event handlers. Any other case I don't know.

> 2: see 1,

Which script is the top-level script? 

> 3: it's marked as such (I don't remember the magic identifier,
> but there is one, IIRC for the functionName or whatever it is). 

By looking at the jsdIScripts as they enter onScriptCreated() I determined that a null functionName is the top level for the cases that Firebug supports. I have no idea if this is by design or not.

> 4: you're
> already debugging, otherwise you don't care, typically. :-)

Maybe I was not clear in #4: I meant "know whether the compilation is eval, new Function, event handler, ..."

> 
> I'll summarize in another comment, just to keep things clear.

I omitted some stuff, let me know if I've missed something.

Comment 16

[John J. Barton]

(In reply to comment #14)
> Here's what I deduce from what you've told me so far. You want an API that:
> 
> - Organizes jsdScripts into things that are to do with the same file.

Just to be clear, "file" here should be "compilation unit", more like a buffer.

> - Lets you know that an eval() script is an eval() script (similar for new
> Function(), document.write, setTimeout/Interval + String argument, and
> friends), and where it "came from".
> 
> I'm not sure how the two fit together, that is, whether an eval script has to
> do with an earlier file (obviously the line numbers need magic applied to them,
> but we also care that eval() was called in a specific file/window, etc.).

I don't follow you here. What 'earlier file'? Yes the eval() fileName and line number stuff is a mess, yes we should fix it. But we don't need to do it here.

If you tell me that we can get the compile buffer input at every point where we need to call onCompilation(), then we could consider adding it to the hook callback. I can't say anything about that, I don't understand the C++ code well enough.

> 
> I think these are noble aims. However, at the moment I'm very worried about the
> performance of JSD and associated debuggers. We've already seen issues with
> Firebug and venkman causing performance issues throughout normal web use.

I have investigated many such rumors for Firebug and I can tell you that these issues are not related to anything we are discussing here. Rather the two cases are 1) Error processing and 2) unreproducible reports of excess memory use.

I think the proposed API is efficient and more efficient than API where we add more features like compilation units. I am confident that this proposed API would speed up Firebug. So I'm confused about why performance is an issue.

> Additionally, we know for Chromebug/Venkman's sake, that we can't rely on
> scripts always being created after our own component has started up. So,
> *relying on our JSD consumer code running when the script gets compiled* is not
> something I want. Your implementation suggestion doesn't deal with this
> problem, and it additionally gets us two alternate methods to get jsd scripts
> (onScriptCreated as well as onCompilationHook) 

I answered this point on Comment 15, sorry our message crossed paths.

> As I said in an earlier comment,
> we just had some great work go into the tree to make jsd scripts be around
> after the fact. I think we should be building on this when we fix these issues,
> too.

I don't know anything about that, but to me this is a minor point. If jsd was perfect in storing info it would not change the requirement of being able to debug web/xul at every point in time. Since we need the debugger to be fully operational and in control at all points, then the problem missing stuff just goes away.

> 
> So, I think your suggestion to tag the scripts is much better. As a consumer,
> we can keep using onScriptCreated, and gather scripts into arrays/dictionaries
> for each unique tag, thus creating exactly the same "compilation unit"
> structures consumers would get from using your approach.
> 
> Additionally, I would suggest we could add an API that fetches all the
> jsdscripts given such a unique tag. This way, jsd consumers won't miss a beat
> no matter when they get enabled, and it will be clear, if necessary, when you
> have all the scripts for a file (so when it's finished compiling). I'm guessing
> that, since we can find jsd scripts for js scripts, and we should be able to
> find the parent js script, it should be easy to identify all scripts that are
> relevant this way, and no extra caching or w/e will be necessary.

I'll look into a compilation unit api.  However I can write a thousand lines of JS code in the time it takes to get 100 lines of C++ into the build. Consequently I am very reluctant to increase the C++ footprint we need to succeed.

> 
> Finally, we need a solution for identifying eval. From what I can tell, there's
> no reason we couldn't add the flags you're using to the onScriptCreated call.
> Same thing for the stackframe (although that should be null for "normal" script
> loads, like <script> tags and so on). As timeless said, we can happily extend
> jsdDebuggerService, so let's. :-)

Ok that would be fine.

Comment 17

[John J. Barton]

(In reply to comment #5)
> (From update of attachment 415501 [details] [diff] [review])
...
> i don't see any reason to move these declarations outside the loop.
> I'd much rather you not do something like this. (my tree converts jsd to c++
> anyway)
> >+    JSScript* script;
> >+    jsuword  pc;
> 
> >     while( NULL != (fp = JS_FrameIterator(cx, &iter)) )
> >     {
> just leave the decl here
> >-        JSScript* script = JS_GetFrameScript(cx, fp);
> >-        jsuword  pc = (jsuword) JS_GetFramePC(cx, fp);
> >+        if (caller && (fp != caller)) 
> >+        }
> and split the assignment to here:
> >+        script = JS_GetFrameScript(cx, fp);
> >+        pc = (jsuword) JS_GetFramePC(cx, fp);

I could not get the code to compile if I move the decl. Note the file is jsd_stak.c, not C++.

Comment 18

[John J. Barton]

Attachment: revised prototype partial implementation patch

This attempts to address timeless' comments. (It only compiles, no runtime tests). Now I will look into Gijs' API request.

Comment 19

[:Gijs (out for now; he/him)]

(In reply to comment #15)
> Well what ever it is for eval calls. This proposal does not directly address
> the problem that fileName is not very useful for eval() calls. Rather it
> provides you with the information that this was an eval() call and hence one
> can, as Firebug does, invent a better identifier.
> 
> I'm not saying that fixing the fileName is not a good thing. I am only saying
> that fixing the fileName is not what we are doing here.

Right, but I was hoping that I could determine the file from which eval() was called. I suppose the stackframe could, then...

> The second argument to onCompilation() is an int that tells us that this
> compile is an eval().

Right, but it's not put in the same compilation unit as the script that calls eval(), which is what I was unclear about.


> > As for your points:
> > 1) But I'd need to then get the 4 jsdScripts, which makes this be 6+ calls
> > rather than 1 (1 for the onCompilation, 1 for calling enumerate, 4 to get each
> > script from the enumerator). That'll just be slower, because they'll all cross
> > xpconnect boundaries (for Firebug and Venkman, anyway).
> 
> Yes, the calls are 
>   onCompilation,
>    enumerateCompiledScripts
>     4 x callbacks for jsdIScripts
> But if you trace through the code that Firebug runs onScriptCreated() it does a
> lot of work, including setting breakpoints on PC=0 of the outer (top-level,
> file-scope initializing) script, halting on that BP and removing the
> breakpoint. 

Right. I agree we should limit this work, but I think the API I proposed does better than this, as it just needs the same 4 calls if the debugger is active (which we have now), doesn't require much change from existing debugger implementations, doesn't duplicate existing functionality (onScriptCreated vs. onCompilation), and still works if you attach a debugger after the page has loaded.

> > 2+3) Agreed, that would be nice to have.
> > 4+5) Merm, I guess. I think you basically don't care about eval() unless you're
> > debugging the relevant code, in which case you also already know where the
> > eval() came from.
> 
> I'm lost about what 4+5 is and I don't know what a 'Merm' is.

> > 4) you know the compilation is an eval(), 5) you have the stack
> > frame so you know what code triggered the eval().

And I was pointing out that, if I care about the eval, I should already be debugging the script, which means I *know* where the call came from.

As for 'Merm', http://en.wiktionary.org/wiki/erm, http://en.wiktionary.org/wiki/meh

> The start up problem is a separate issue. Chromebug is able to get (almost)
> everything as it happens by starting itself as a high priority command line
> component. We need to be able to halt JS at any point in the execution, so we
> need support for debugging at every execution point. Yes it is hard and Venkman
> may not quite have it all yet, but that should be our goal.
> 
> We may still want to be able to enumerate the compilation units from
> enumeration of the jsdIScripts. But this is a separate desire, a performance or
> API-niceness requirement, not an essential requirement. I'm fine if we decide
> that we want to support although ideally I'd like to take it in two bugs to
> maximize our chances of landing this one.

I don't think the API I suggested is harder to implement than yours. I do think it's genuinely better, because of the reasons I outlined above:
- no duplication of existing APIs.
- same or lower number of cross-xpconnect-boundary calls in the same situations.
- works if you attach the debugger post-compilation
- requires little adaptation on the part of jsd consumers.
 
> > OK. But it only does that if I have the hook registered at the Right Time. I
> > don't think that's as trivial as you suggest.
> 
> Not trivial, but possible.  Chromebug can do it, in fact I have found startup
> bugs in Firefox. If we implemented persistent or command line breakpoints in
> chromebug we could do it routinely.

I haven't looked into the implementation chromebug uses, but I'm pretty sure this is not possible. In general, given some API 'A' that you use to register early, any other add-on/Firefox/app/xulrunner component can use the same API 'A'. If you suggest there is some API 'B' that goes before 'A', the same rule applies to API 'B'. QED.

What's worse, having the debugger service active on startup and effectively listening for onScriptCreated (or onCompilationHook + enumerate calls) leads to real perf problems. See bug 483282 and the stats Robert Kaiser lists there. Doing this on startup leads to real startup perf degradation. Given an API that gives us the same data without perf degradation, I'm rooting for the second one.

> Analyzing the call stack to determine the cause of the compilation seems
> unnecessarily complex since the call point of the hook knows the exact answer
> and it only costs us an int argument.

Fine, so stick that argument in onScriptCreated.

> Knowing that we have a eval() is not enough unless we also fix the
> jsdIScript.fileName problem. So we need the call stack also.

Same answer, I guess, although again, I'm not sure of a usecase where you care and don't already debug, and so "know" where you were before in terms of the stack.

> >Then, for 1: the order is known (toplevels come
> > last),
> 
> I know this is true for HTML script tags, eval(), and browser generated event
> handlers. Any other case I don't know.
> 
> > 2: see 1,
> 
> Which script is the top-level script? 

toplevels come last, as I said just above that.


> By looking at the jsdIScripts as they enter onScriptCreated() I determined that
> a null functionName is the top level for the cases that Firebug supports. I
> have no idea if this is by design or not.

I can't recall as it is. What is it for function() {} ? If that's an empty string, then presumably the answer to your question is yes.


(In reply to comment #16)
> (In reply to comment #14)
> > - Lets you know that an eval() script is an eval() script (similar for new
> > Function(), document.write, setTimeout/Interval + String argument, and
> > friends), and where it "came from".
> > 
> > I'm not sure how the two fit together, that is, whether an eval script has to
> > do with an earlier file (obviously the line numbers need magic applied to them,
> > but we also care that eval() was called in a specific file/window, etc.).
> 
> I don't follow you here. What 'earlier file'? Yes the eval() fileName and line
> number stuff is a mess, yes we should fix it. But we don't need to do it here.
> 
> If you tell me that we can get the compile buffer input at every point where we
> need to call onCompilation(), then we could consider adding it to the hook
> callback. I can't say anything about that, I don't understand the C++ code well
> enough.

I don't think we can, and even if we could I'm not sure passing crap that big along for every call is the right thing to do. As for eval() and its filename, I was suggesting that your idea of adding a stack was meant to allow one to know where eval() was called from.

> > I think these are noble aims. However, at the moment I'm very worried about the
> > performance of JSD and associated debuggers. We've already seen issues with
> > Firebug and venkman causing performance issues throughout normal web use.
> 
> I have investigated many such rumors for Firebug and I can tell you that these
> issues are not related to anything we are discussing here. Rather the two cases
> are 1) Error processing and 2) unreproducible reports of excess memory use.

Please refer to my bug link above, which has perf numbers.

> I think the proposed API is efficient and more efficient than API where we add
> more features like compilation units. I am confident that this proposed API
> would speed up Firebug. So I'm confused about why performance is an issue.

Because you can have the same features your API proposes by taking the three steps I outlined in comment 14, with the combination of calls you need in that case being smaller than your proposed API. Additionally, the steps I suggested allow you to get the info you want *retrospectively* which means you don't need to have the debugger active all the time, which is a perf gain.


> > As I said in an earlier comment,
> > we just had some great work go into the tree to make jsd scripts be around
> > after the fact. I think we should be building on this when we fix these issues,
> > too.
> 
> I don't know anything about that, but to me this is a minor point. If jsd was
> perfect in storing info it would not change the requirement of being able to
> debug web/xul at every point in time. Since we need the debugger to be fully
> operational and in control at all points, then the problem missing stuff just
> goes away.

That's odd, you were CC'd on, and commented on, the relevant bug (bug 480765). Which is exactly about not needing the debugger being fully operational and in control "at all points", and instead only activating it when you *actually* want to debug stuff. 


> I'll look into a compilation unit api.  However I can write a thousand lines of
> JS code in the time it takes to get 100 lines of C++ into the build.
> Consequently I am very reluctant to increase the C++ footprint we need to
> succeed.

I think this (adding an int tag to jsdscripts that's a unique compilation unit indicator, and modifying onScriptCreated with stack + compilation type indicator) can replace the API you're suggesting, and would not need significantly more lines of C/C++ than your proposed API (I actually strongly suspect it'd need fewer lines, but I haven't tried implementing it yet, so maybe I'm about to find out the hard way :-) ).

If I missed anything, that is, if there's something your proposed API does that my suggestion doesn't do, then please let me know.

Comment 20

[:Gijs (out for now; he/him)]

To be clear:

> Because you can have the same features your API proposes by taking the three
> steps I outlined in comment 14 ...

> I think this (adding an int tag to jsdscripts that's a unique compilation unit
> indicator, and modifying onScriptCreated with stack + compilation type
> indicator) ...

I think having an API that returns all scripts given such a unique compilation unit indicator tag thingummywhatsit (so the third thing, the difference between the "three steps" and my enumeration with two steps later), is optional. I'm not sure one would really need it, it just seemed like something that would essentially give you the same kind of enumerator that you're currently proposing (except you could call it at any time). Because the jsd functionHook already gives you all the scripts right before you'd want them (see bug 480765), I'm not sure it's actually necessary.

Comment 21

[John J. Barton]

(In reply to comment #19)
> (In reply to comment #15)
> > Well what ever it is for eval calls. This proposal does not directly address
> > the problem that fileName is not very useful for eval() calls. Rather it
> > provides you with the information that this was an eval() call and hence one
> > can, as Firebug does, invent a better identifier.
> > 
> > I'm not saying that fixing the fileName is not a good thing. I am only saying
> > that fixing the fileName is not what we are doing here.
> 
> Right, but I was hoping that I could determine the file from which eval() was
> called. I suppose the stackframe could, then...

Yes, if you know that the compile was for an eval(), then the frame gives you the call site.

> 
> > The second argument to onCompilation() is an int that tells us that this
> > compile is an eval().
> 
> Right, but it's not put in the same compilation unit as the script that calls
> eval(), which is what I was unclear about.

The compilation unit of the eval buffer is different from the compilation unit of the caller. 

> 
> 
> > > As for your points:
> > > 1) But I'd need to then get the 4 jsdScripts, which makes this be 6+ calls
> > > rather than 1 (1 for the onCompilation, 1 for calling enumerate, 4 to get each
> > > script from the enumerator). That'll just be slower, because they'll all cross
> > > xpconnect boundaries (for Firebug and Venkman, anyway).
> > 
> > Yes, the calls are 
> >   onCompilation,
> >    enumerateCompiledScripts
> >     4 x callbacks for jsdIScripts
> > But if you trace through the code that Firebug runs onScriptCreated() it does a
> > lot of work, including setting breakpoints on PC=0 of the outer (top-level,
> > file-scope initializing) script, halting on that BP and removing the
> > breakpoint. 
> 
> Right. I agree we should limit this work, but I think the API I proposed does
> better than this, as it just needs the same 4 calls if the debugger is active
> (which we have now), doesn't require much change from existing debugger
> implementations, doesn't duplicate existing functionality (onScriptCreated vs.
> onCompilation), and still works if you attach a debugger after the page has
> loaded.

Can you outline your proposed API? It sounds like you want to take the parameters I propose for onCompilation() and add them onto then end of onScriptCreated() so the debugger gets the same results by the time the compile is complete. I guess that is ok if we have some way to know the first call for a compilation unit the last one. 

(I don't agree about this being any simpler, because Firebug's JS source tracking would be completely re-written in either case).

> 
> > > 2+3) Agreed, that would be nice to have.
> > > 4+5) Merm, I guess. I think you basically don't care about eval() unless you're
> > > debugging the relevant code, in which case you also already know where the
> > > eval() came from.
> > 
> > I'm lost about what 4+5 is and I don't know what a 'Merm' is.
> 
> > > 4) you know the compilation is an eval(), 5) you have the stack
> > > frame so you know what code triggered the eval().
> 
> And I was pointing out that, if I care about the eval, I should already be
> debugging the script, which means I *know* where the call came from.

How can you know where the call is from without the call stack?

> 
> As for 'Merm', http://en.wiktionary.org/wiki/erm,
> http://en.wiktionary.org/wiki/meh
> 
> > The start up problem is a separate issue. Chromebug is able to get (almost)
> > everything as it happens by starting itself as a high priority command line
> > component. We need to be able to halt JS at any point in the execution, so we
> > need support for debugging at every execution point. Yes it is hard and Venkman
> > may not quite have it all yet, but that should be our goal.
> > 
> > We may still want to be able to enumerate the compilation units from
> > enumeration of the jsdIScripts. But this is a separate desire, a performance or
> > API-niceness requirement, not an essential requirement. I'm fine if we decide
> > that we want to support although ideally I'd like to take it in two bugs to
> > maximize our chances of landing this one.
> 
> I don't think the API I suggested is harder to implement than yours. I do think
> it's genuinely better, because of the reasons I outlined above:
> - no duplication of existing APIs.
> - same or lower number of cross-xpconnect-boundary calls in the same
> situations.
> - works if you attach the debugger post-compilation

This one I don't understand. You have to get the information from onScriptCreated(), so you have to be attached when onScriptCreated is called.

> - requires little adaptation on the part of jsd consumers.
> 
> > > OK. But it only does that if I have the hook registered at the Right Time. I
> > > don't think that's as trivial as you suggest.
> > 
> > Not trivial, but possible.  Chromebug can do it, in fact I have found startup
> > bugs in Firefox. If we implemented persistent or command line breakpoints in
> > chromebug we could do it routinely.
> 
> I haven't looked into the implementation chromebug uses, but I'm pretty sure
> this is not possible. In general, given some API 'A' that you use to register
> early, any other add-on/Firefox/app/xulrunner component can use the same API
> 'A'. If you suggest there is some API 'B' that goes before 'A', the same rule
> applies to API 'B'. QED.

Well it's not rocket science or math: I just register at "@@@ Chromebug". If you register before that, you can't be debugged. That corner case is not worth any time or energy.

> 
> What's worse, having the debugger service active on startup and effectively
> listening for onScriptCreated (or onCompilationHook + enumerate calls) leads to
> real perf problems. See bug 483282 and the stats Robert Kaiser lists there.
> Doing this on startup leads to real startup perf degradation. Given an API that
> gives us the same data without perf degradation, I'm rooting for the second
> one.

Bug 483282 turns off jsd on startup. That prevents all information from being collected from startup until you turn jsd on.  So it's not related to any difference in jsd API's. Whether you have your jsd hooks installed or not does not matter if jsd is inactive. 

> 
> > Analyzing the call stack to determine the cause of the compilation seems
> > unnecessarily complex since the call point of the hook knows the exact answer
> > and it only costs us an int argument.
> 
> Fine, so stick that argument in onScriptCreated.

Yes, that would work, assuming we have some way to know:
  The first call,
  The last call,
  That the calls are in order and never nest.
> 
> > Knowing that we have a eval() is not enough unless we also fix the
> > jsdIScript.fileName problem. So we need the call stack also.
> 
> Same answer, I guess, although again, I'm not sure of a usecase where you care
> and don't already debug, and so "know" where you were before in terms of the
> stack.

? I don't understand this sentence. 

> 
> > >Then, for 1: the order is known (toplevels come
> > > last),
> > 
> > I know this is true for HTML script tags, eval(), and browser generated event
> > handlers. Any other case I don't know.
> > 
> > > 2: see 1,
> > 
> > Which script is the top-level script? 
> 
> toplevels come last, as I said just above that.
> 
> 
> > By looking at the jsdIScripts as they enter onScriptCreated() I determined that
> > a null functionName is the top level for the cases that Firebug supports. I
> > have no idea if this is by design or not.
> 
> I can't recall as it is. What is it for function() {} ? If that's an empty
> string, then presumably the answer to your question is yes.

I don't want to presume anymore. Why can't we just get explicit information on what scripts compiled together? (for |function(){}| you get "anonymous"; you also get "anonymous" for |function anonymous(){}|. Cool huh?)

> 
> (In reply to comment #16)
> > (In reply to comment #14)
> > > - Lets you know that an eval() script is an eval() script (similar for new
> > > Function(), document.write, setTimeout/Interval + String argument, and
> > > friends), and where it "came from".
> > > 
> > > I'm not sure how the two fit together, that is, whether an eval script has to
> > > do with an earlier file (obviously the line numbers need magic applied to them,
> > > but we also care that eval() was called in a specific file/window, etc.).
> > 
> > I don't follow you here. What 'earlier file'? Yes the eval() fileName and line
> > number stuff is a mess, yes we should fix it. But we don't need to do it here.
> > 
> > If you tell me that we can get the compile buffer input at every point where we
> > need to call onCompilation(), then we could consider adding it to the hook
> > callback. I can't say anything about that, I don't understand the C++ code well
> > enough.
> 
> I don't think we can, and even if we could I'm not sure passing crap that big
> along for every call is the right thing to do. As for eval() and its filename,
> I was suggesting that your idea of adding a stack was meant to allow one to
> know where eval() was called from.

? As I understand it you are proposing to add arguments to every onScriptCreated() call. How can adding one pointer to a string buffer to one call per compilation unit be worse than adding several arguments to a call for every script creation (tens of thousands to one).


> 
> > > I think these are noble aims. However, at the moment I'm very worried about the
> > > performance of JSD and associated debuggers. We've already seen issues with
> > > Firebug and venkman causing performance issues throughout normal web use.
> > 
> > I have investigated many such rumors for Firebug and I can tell you that these
> > issues are not related to anything we are discussing here. Rather the two cases
> > are 1) Error processing and 2) unreproducible reports of excess memory use.
> 
> Please refer to my bug link above, which has perf numbers.

As I said earlier, bug 483282 is not relevant. It concerns "jsd Off" vs "jsd On". Here we are discussing only the case where "jsd On"; "jsd Off" means none of our APIs are called.

> 
> > I think the proposed API is efficient and more efficient than API where we add
> > more features like compilation units. I am confident that this proposed API
> > would speed up Firebug. So I'm confused about why performance is an issue.
> 
> Because you can have the same features your API proposes by taking the three
> steps I outlined in comment 14, with the combination of calls you need in that
> case being smaller than your proposed API. Additionally, the steps I suggested
> allow you to get the info you want *retrospectively* which means you don't need
> to have the debugger active all the time, which is a perf gain.

Ok I did not understand what bug 480765 did. Firebug mostly does not use any jsdIScript information stored in the engine. It's too confusing to sort out.


> 
> 
> > > As I said in an earlier comment,
> > > we just had some great work go into the tree to make jsd scripts be around
> > > after the fact. I think we should be building on this when we fix these issues,
> > > too.
> > 
> > I don't know anything about that, but to me this is a minor point. If jsd was
> > perfect in storing info it would not change the requirement of being able to
> > debug web/xul at every point in time. Since we need the debugger to be fully
> > operational and in control at all points, then the problem missing stuff just
> > goes away.
> 
> That's odd, you were CC'd on, and commented on, the relevant bug (bug 480765).
> Which is exactly about not needing the debugger being fully operational and in
> control "at all points", and instead only activating it when you *actually*
> want to debug stuff. 

Well I disagreed with bug 480765 then and still do. The jsdIScript objects don't have enough information to create a proper debugger.

> 
> > I'll look into a compilation unit api.  However I can write a thousand lines of
> > JS code in the time it takes to get 100 lines of C++ into the build.
> > Consequently I am very reluctant to increase the C++ footprint we need to
> > succeed.
> 
> I think this (adding an int tag to jsdscripts that's a unique compilation unit
> indicator, and modifying onScriptCreated with stack + compilation type
> indicator) can replace the API you're suggesting, and would not need
> significantly more lines of C/C++ than your proposed API (I actually strongly
> suspect it'd need fewer lines, but I haven't tried implementing it yet, so
> maybe I'm about to find out the hard way :-) ).
> 
> If I missed anything, that is, if there's something your proposed API does that
> my suggestion doesn't do, then please let me know.

So if I understand what you are suggesting, today we have:

...
onScriptCreated(script)
onScriptCreated(script)
onScriptCreated(script)
...

mine:

onCompilation(frame, type, outerScript);

yours:

onScriptCreated(script, frame, type);

So they have to be equivalent except for  "when does the compile start and end and which is the outerScript".

I am working on another version which has

onCompilation(frame, jsdICompilationUnit);

jsdICompilationUnit has
   type,
   includer (script tag, function, or HTML element)
   source
   scripts

Comment 22

[:Gijs (out for now; he/him)]

(In reply to comment #21)
> > Right. I agree we should limit this work, but I think the API I proposed does
> > better than this, as it just needs the same 4 calls if the debugger is active
> > (which we have now), doesn't require much change from existing debugger
> > implementations, doesn't duplicate existing functionality (onScriptCreated vs.
> > onCompilation), and still works if you attach a debugger after the page has
> > loaded.
> 
> Can you outline your proposed API? It sounds like you want to take the
> parameters I propose for onCompilation() and add them onto then end of
> onScriptCreated() so the debugger gets the same results by the time the compile
> is complete. I guess that is ok if we have some way to know the first call for
> a compilation unit the last one. 

Actually, I changed my mind. I think the type of compile should go on the jsdIScript as well. I'll just hack up the idl and stick it on the bug after I finish commenting.

> > And I was pointing out that, if I care about the eval, I should already be
> > debugging the script, which means I *know* where the call came from.
> 
> How can you know where the call is from without the call stack?

Because you're already debugging (stopped in the JS, and so have a call stack) before the eval() happens.

> > I don't think the API I suggested is harder to implement than yours. I do think
> > it's genuinely better, because of the reasons I outlined above:
> > - no duplication of existing APIs.
> > - same or lower number of cross-xpconnect-boundary calls in the same
> > situations.
> > - works if you attach the debugger post-compilation
> 
> This one I don't understand. You have to get the information from
> onScriptCreated(), so you have to be attached when onScriptCreated is called.

If you're not attached when onScriptCreated is called, and we put ID tags (and type tags, apparently) on the jsdIScripts, then you can identify which compilation unit something belongs to whenever you get a jsdIScript - including if that is through some other hook, because you attached the debugger way after the script compiled.
 
> Well it's not rocket science or math: I just register at "@@@ Chromebug".

I don't understand how you "register at" a string, which is clearly not a Category Manager category (like xpcom-startup, app-startup, yada yada). Maybe link me some code instead?


> Bug 483282 turns off jsd on startup. That prevents all information from being
> collected from startup until you turn jsd on.  So it's not related to any
> difference in jsd API's. Whether you have your jsd hooks installed or not does
> not matter if jsd is inactive. 

Yes, but whether you turn jsd off or on at startup matters to perf. If the API allows us to get script info even if jsd was turned off at initial compile, then we can leave JSD off. If it doesn't, we need to turn JSD on ASAP after startup, which sucks for perf.


> > Same answer, I guess, although again, I'm not sure of a usecase where you care
> > and don't already debug, and so "know" where you were before in terms of the
> > stack.
> 
> ? I don't understand this sentence.

If you are stepping (stopped) in some script X, which evals something and creates script Y, you know that it's script X creating script Y, and so you have (can keep around) a stacktrace (or a reference to the calling script, or whatever you want to have) without it being explicitly passed through the onScriptCreated call.

 
> I don't want to presume anymore. Why can't we just get explicit information on
> what scripts compiled together? (for |function(){}| you get "anonymous"; you
> also get "anonymous" for |function anonymous(){}|. Cool huh?)

If you don't want to presume, look at the js/jsd code? Seems you're doing that already anyway...
 
 
> ? As I understand it you are proposing to add arguments to every
> onScriptCreated() call. How can adding one pointer to a string buffer to one
> call per compilation unit be worse than adding several arguments to a call for
> every script creation (tens of thousands to one).

Because the pointer isn't the problem, the memory required to store the data is.

> Ok I did not understand what bug 480765 did. Firebug mostly does not use any
> jsdIScript information stored in the engine. It's too confusing to sort out.

What's confusing about it? AIUI all the interfaces are documented, there are usecase examples in Venkman... not sure what's unclear.
 
> Well I disagreed with bug 480765 then and still do. The jsdIScript objects
> don't have enough information to create a proper debugger.

What info are they missing?
 
> So if I understand what you are suggesting, today we have:
> 
> ...
> onScriptCreated(script)
> onScriptCreated(script)
> onScriptCreated(script)
> ...
> 
> mine:
> 
> onCompilation(frame, type, outerScript);
> 
> yours:
> 
> onScriptCreated(script, frame, type);
> 
> So they have to be equivalent except for  "when does the compile start and end
> and which is the outerScript".
> 
> I am working on another version which has
> 
> onCompilation(frame, jsdICompilationUnit);
> 
> jsdICompilationUnit has
>    type,
>    includer (script tag, function, or HTML element)
>    source
>    scripts

Your version doesn't actually tell you when the compilation starts, just when it ends. You can know that already when you get an onScriptCreated call with a script's toplevel (with a null functionName, as discussed earlier). The start of a compile would be more or less implied by the last onScriptCreated call from a previous compile. If you want explicit start/end events to time compilations, that's a separate issue imho.

Comment 23

[:Gijs (out for now; he/him)]

Attachment: Proposed API

I could be persuaded that the stackframe should be an attribute too; but I'm not sure if it remains valid when the compiler moves on, and not at all sure it remains useful after the compiler has done so.

Otherwise, obviously the relevant UUIDs should be rev'd, I didn't bother with that here. I hope the comments clarify my intent, if not, please ask questions.

Comment 24

[John J. Barton]

I think we are making progress so rather than go into more details I'll just answer two of your specific questions then try to get my version 2 api up.

(In reply to comment #22)
> (In reply to comment #21)
...
> I don't understand how you "register at" a string, which is clearly not a
> Category Manager category (like xpcom-startup, app-startup, yada yada). Maybe
> link me some code instead?
> 
Ok the chromebug startup observer:
http://code.google.com/p/fbug/source/browse/chromebug/branches/chromebug1.5/components/chromebug-startup-observer.js
...

> > Well I disagreed with bug 480765 then and still do. The jsdIScript objects
> > don't have enough information to create a proper debugger.
> 
> What info are they missing?

Bug 480765 supports a very narrow range of debugging needs, the ability to set breakpoints in currently existing functions. So much of the debugger functionality requires it to be active while the code runs that this capability does not help. I suppose its ok, but it's not really useful.

In my opinion, the general problem of jsd causing overhead for normal browsing should be faced head on and solved, not by adding features that try to work around the problems.

Comment 25

[:Gijs (out for now; he/him)]

(In reply to comment #24)
> Ok the chromebug startup observer:
> http://code.google.com/p/fbug/source/browse/chromebug/branches/chromebug1.5/components/chromebug-startup-observer.js

That observes app-startup, which means, AIUI, that you miss things which occur at xpcom-startup.

> > > Well I disagreed with bug 480765 then and still do. The jsdIScript objects
> > > don't have enough information to create a proper debugger.
> > 
> > What info are they missing?
> 
> Bug 480765 supports a very narrow range of debugging needs, the ability to set
> breakpoints in currently existing functions. So much of the debugger
> functionality requires it to be active while the code runs that this capability
> does not help. I suppose its ok, but it's not really useful.

You're answering the question "What *can* you do with jsdIScripts", rather than "What is missing from jsdIScripts that would make it easier to create a 'proper' debugger?", which was my question. For bonus points, mention things that your proposed API solves and my proposal doesn't. :-)

> In my opinion, the general problem of jsd causing overhead for normal browsing
> should be faced head on and solved, not by adding features that try to work
> around the problems.

We were adding features anyway, I'm trying to add them so we don't make perf worse, or defeat modifications that enabled perf gains. Having the eval/compilation unit info tagged onto the script means one can determine where function scripts came from even if one gets them long after the original script was compiled. This allows the work from bug 480765 to be useful.

If you've got proposals on how to reduce the perf hit Firefox currently takes from enabling JSD, please do feel free to file separate bugs for them.

Comment 26

[John J. Barton]

(In reply to comment #23)
> Created an attachment (id=416148) [details]
> Proposed API
> 
> I could be persuaded that the stackframe should be an attribute too; but I'm
> not sure if it remains valid when the compiler moves on, and not at all sure it
> remains useful after the compiler has done so.

I agree that frames should not be data, it will just be confusing.

> 
> Otherwise, obviously the relevant UUIDs should be rev'd, I didn't bother with
> that here. I hope the comments clarify my intent, if not, please ask questions.

How can we set the compilationTag if we don't set it at compilation time?

Comment 27

[John J. Barton]

Attachment: API idl version 2

This is a different approach that attempts to address Gijs issues. I think it is a much nicer API than my first one (though it requires quite a bit more implementation).

Not shown here that jsdIScript would have
   readonly attribute jsdICompilationUnit    unit;
and an enumerator over all jsdICompilationUnits, I guess in jsdIContext.

You will see a begin and end call that would bracket every compile. These calls would also serve as events to mark begin and end of compilation.

This version does not change anything about onScriptCreated/onScriptDestoyed.

An important question of implementation relates to Gijs' desire to get information about past compiles. To support that we'd have to have some way to relate JS scripts to their compilation unit so when we create the (late) jsdIScript wrappers we can also create the (late) jsdICompilationUnit. This is the same issue I raised in Comment 26 and hopefully we could use the same answer.

Comment 28

[John J. Barton]

(In reply to comment #25)
> If you've got proposals on how to reduce the perf hit Firefox currently takes
> from enabling JSD, please do feel free to file separate bugs for them.

I started an answer on bug 449452.

Comment 29

[:Gijs (out for now; he/him)]

Comment on attachment 416206 [details]
API idl version 2

>[scriptable, uuid(F3915B2E-9BC4-4f32-BF52-F9FE243CF597)]
>interface jsdICompilationUnit : jsdIEphemeral

When does isValid (in jsdIEphemeral) become false?

>{
>    /* script tags, included .js files */
>    const unsigned long TYPE_SCRIPT_TAG_COMPILATION = 1;
>    /* eval(), new Function() */
>    const unsigned long TYPE_EVAL_COMPILATION = 2;
>    /* event scripts */
>    const unsigned long TYPE_GENERATED_COMPILATION = 3;
>
>    /**
>     * One of the TYPE_* values above.
>     */
>    readonly attribute unsigned long  unitType;
>    /*
>     * The script for the unit-scope initialization or top level function.
>     * For <script> tags in HTML pages, this script runs after the onCompilation hook call.
>     * Maybe null for some compilation units.
>     * After this script runs outerScript.isValid will typically be false.
>     */
>    readonly attribute jsdIScript     outerScript;

What is this in the onCompilationBegin call? The outer script hasn't been compiled yet at that point, I don't think.

As a separate point, the javadoc comment says it might be null - when?

Finally, you already get this when you call getScripts(), assuming it's been compiled at that point.


>    /*
>     * The object responsible for causing the compilation.
>     * For TYPE_SCRIPT_TAG_COMPILATION, this will be a <script> element in the DOM
>     * For TYPE_EVAL_COMPILATION, this will be a jsdIScript
>     * For TYPE_GENERATED_COMPILATION, this will be an element in the DOM.
>     */
>    readonly attribute jsdIValue      includer;
Shudder. JSD holding references to dom elements like this is extremely scary. That'll leak, or (if you null it out all the time) this value can't be depended upon. Doesn't seem worth it to me. If you want this, stick it in the notification call(s), at least then we don't need to promise it'll stay around.

>    /**
>     * Source code for this compilation unit
>     */
>    readonly attribute AString source;
jsdIScript has an attribute "sourceText". I don't think you want to duplicate it.

>    /*
>     * scripts resulting from the compilation of this unit.
>     */
>    void getScripts([optional] out unsigned long count,
>                    [array, size_is(count), retval] out jsdIScript scripts);
>};
>/**
> * Hook instances of this interface up to the
> * jsdIDebuggerService::compilationHook
> */
>[scriptable, uuid(77db18bc-36c6-4506-8d8a-36a6bda17165)]
>interface jsdICompilationHook : nsISupports
>{
>    /*
>    * @param frame A jsdIStackFrame object representing the oldest stack frame causing the compilation. May be null.
>    * @param type  One of the jsdICompilationHook::TYPE_ constants.
>    * @param outerScript  the outer or top level or file-scope initializiation script for the the compilation.
>    */
>    void onCompilationBegin (in jsdIStackFrame frame, in jsdICompilationUnit unit);
>        /*
>    * @param frame A jsdIStackFrame object representing the oldest stack frame causing the compilation. May be null.
>    * @param type  One of the jsdICompilationHook::TYPE_ constants.
>    * @param outerScript  the outer or top level or file-scope initializiation script for the the compilation.
>    */
>    void onCompilationEnd (in jsdIStackFrame frame, in jsdICompilationUnit unit);
>};

Your javadocs don't match the actual code. I presume you really did want to get rid of the type param, as they're now on compilation units? As said, before script compile, there ain't no outerScript to pass along. In the case of jsdICompilationUnit, is it going to be an empty shell of sorts, given that it won't have scripts yet, and maybe even won't have sourceText yet? Sounds like we need an extra attribute on it to determine whether it's been fully compiled, because otherwise a consumer might not know (I don't think jsdIEphemerals with isValid === false are supposed to come back to life, so we can't reuse that...)

If I attach a debugger handler between an onCompilationBegin and onCompilationEnd call, what happens? Do I still get the latter one? I'm not sure I want one (because I won't have the other part of the pair, and it might confuse my code), and yet if I don't get one I might miss info. This is a vaguely interesting issue.

Then there's the whole story of having a jsdICompilationUnit that has references to jsdIScripts, with the jsdIScripts also having references to the jsdICompilationUnit. That's going to be a nightmare to keep from leaking, unless you've got a very good story for the lifecycle of these objects.



That is the reason I went with a tag rather than an object: it defers (memory) management of the "compilation unit" stuff to the debugger consumer, which means it's up to them to make sure stuff doesn't leak. I think the JSD API will have a very hard time making sure it all works as expected, without keeping around every script it ever compiled (which would clearly be horrible for memory usage).


So yeah... I could see having actual objects being more natural than tags on the scripts, but I'm quite scared of the implications for memory usage and sanity (when does which object come into being, when does each object go away). A bunch of the attributes on the objects also seem pointless or extremely leak-prone to me (see above).

If you come up with a good story for the lifecycle of a jsdICompilationUnit object and its jsdIScript objects, I could live with that, otherwise I'd prefer tagging jsdIScript objects instead.

Comment 30

[John J. Barton]

(In reply to comment #29)
> (From update of attachment 416206 [details])
> >interface jsdICompilationUnit : jsdIEphemeral
> 
> When does isValid (in jsdIEphemeral) become false?

It becomes false if the containing js context is destroyed or if all of the |compiledScripts| are destroyed.

...
> >    readonly attribute jsdIScript     outerScript;
> 
> What is this in the onCompilationBegin call? The outer script hasn't been
> compiled yet at that point, I don't think.

Correct, it will be null (I'll document that).

> 
> As a separate point, the javadoc comment says it might be null - when?

During the onCompilationBegin call ;-). But Boris also says there are cases where the outerScript is not created. I've not seen these yet.

> 
> Finally, you already get this when you call getScripts(), assuming it's been
> compiled at that point.

Correct, but we don't know which one is the outerScript. 

> 
> 
> >    /*
> >     * The object responsible for causing the compilation.
> >     * For TYPE_SCRIPT_TAG_COMPILATION, this will be a <script> element in the DOM
> >     * For TYPE_EVAL_COMPILATION, this will be a jsdIScript
> >     * For TYPE_GENERATED_COMPILATION, this will be an element in the DOM.
> >     */
> >    readonly attribute jsdIValue      includer;
> Shudder. JSD holding references to dom elements like this is extremely scary.
> That'll leak, or (if you null it out all the time) this value can't be depended
> upon. Doesn't seem worth it to me. If you want this, stick it in the
> notification call(s), at least then we don't need to promise it'll stay around.

We hold a lot of references to DOM elements, one more won't make any difference. We already have a non-API mechanism to avoid leaks: we delete our meta-data when the nsIDOMWindow is deleted. 

> 
> >    /**
> >     * Source code for this compilation unit
> >     */
> >    readonly attribute AString source;

> jsdIScript has an attribute "sourceText". I don't think you want to duplicate
> it.

I could not find that attribute. I guess you meant |functionSource|? If so, then that attribute is incorrectly documented and it has an unfortunate name. In practice accessing the attribute causes the script to be decompiled and converted to a source-like text string. (I'm in favor of changing the API docs and attribute name on another bug if you like).
...
(I'll fix up the comments)
...
> If I attach a debugger handler between an onCompilationBegin and
> onCompilationEnd call, what happens? Do I still get the latter one? I'm not
> sure I want one (because I won't have the other part of the pair, and it might
> confuse my code), and yet if I don't get one I might miss info. This is a
> vaguely interesting issue.

We have a similar issue for onScriptCreated/onScriptDestroyed. Here the answer can depend upon our decision regarding storing jsdICompilationUnits. Currently the code will not call onCompilationEnd unless there is a value for the current jsdICompilationUnit. The current unit is created if onCompilationBegin is called. If we implement the ability to create compilation units late, then we can set current late (that is after onCompilationBegin but before the matching onCompilationEnd). I believe that the only way for the debugger to get control currently is via onScriptCreated, so I don't think we need to worry about this case.

> 
> Then there's the whole story of having a jsdICompilationUnit that has
> references to jsdIScripts, with the jsdIScripts also having references to the
> jsdICompilationUnit. That's going to be a nightmare to keep from leaking,
> unless you've got a very good story for the lifecycle of these objects.

Thanks for reminding me about this. Every call to onScriptDestroyed() will immediately be preceded by removing the corresponding script from the compilation unit. Then we can check the list and see if it is empty. If so, we can release the refs and set the unit isValid false. Then jsdICompilationUnit object lifetime can be controlled by garbage collection. 

...
> A bunch of the attributes on the objects also seem pointless or extremely
> leak-prone to me (see above).

None of these attributes is pointless, but rather they are vital to making the debugger more correct and timely. In particular:
 |unitType| helps us correctly describe and handle the scripts,
 |includer| is a dynamic replacement for the source URL/line number that is so problematic for dynamic code,
 |outerScript| points to the entry point for the file-scope, (we could use the jsdIScript.tag here but since jsdIScript is already jsdIEphemeral I don't the the value,
 |source| this is vital, we have no more than hacky ways to get it now. We could try to use JSDSourceText, but it looks like abandonware.

I hope this addresses your concerns.

Comment 31

[:Gijs (out for now; he/him)]

(In reply to comment #30)
> > >    readonly attribute jsdIScript     outerScript;
> > 
> > What is this in the onCompilationBegin call? The outer script hasn't been
> > compiled yet at that point, I don't think.
> 
> Correct, it will be null (I'll document that).
> 
> > 
> > As a separate point, the javadoc comment says it might be null - when?
> 
> During the onCompilationBegin call ;-). But Boris also says there are cases
> where the outerScript is not created. I've not seen these yet.

If there are different cases in which they could be null, they should be distinguishable (as I wrote earlier, you could add an attribute - say, "compileFinished" or w/e).

> > Finally, you already get this when you call getScripts(), assuming it's been
> > compiled at that point.
> 
> Correct, but we don't know which one is the outerScript. 

Sure we do, the one with a null functionName. You can go and make that the last one in the array if you like, too (it's already compiled last so that's probably what would happen anyway). If the last item in the array has a non-null functionName, there's no outerScript (yet? see comment about needing to distinguish this from there not being any)

> > >    /*
> > >     * The object responsible for causing the compilation.
> > >     * For TYPE_SCRIPT_TAG_COMPILATION, this will be a <script> element in the DOM
> > >     * For TYPE_EVAL_COMPILATION, this will be a jsdIScript
> > >     * For TYPE_GENERATED_COMPILATION, this will be an element in the DOM.
> > >     */
> > >    readonly attribute jsdIValue      includer;
> > Shudder. JSD holding references to dom elements like this is extremely scary.
> > That'll leak, or (if you null it out all the time) this value can't be depended
> > upon. Doesn't seem worth it to me. If you want this, stick it in the
> > notification call(s), at least then we don't need to promise it'll stay around.
> 
> We hold a lot of references to DOM elements, one more won't make any
> difference. We already have a non-API mechanism to avoid leaks: we delete our
> meta-data when the nsIDOMWindow is deleted.

Who are "we"? I'm pretty sure JSD doesn't, in general. I'm:
- not sure why you need this. Why do you need the script tag element?
- pretty sure it will be very hard to implement (as js and jsd are oblivious as to where their scripts "came from")
- afraid it will be very hard to not leak stuff. Firebug might know about nsIDOMWindows going away, but JSD doesn't.

> > >    /**
> > >     * Source code for this compilation unit
> > >     */
> > >    readonly attribute AString source;
> 
> > jsdIScript has an attribute "sourceText". I don't think you want to duplicate
> > it.
> 
> I could not find that attribute. I guess you meant |functionSource|? If so,
> then that attribute is incorrectly documented and it has an unfortunate name.
> In practice accessing the attribute causes the script to be decompiled and
> converted to a source-like text string. (I'm in favor of changing the API docs
> and attribute name on another bug if you like).

Ah, yes, that's what I meant, sorry it was late...

I think you'll find that if you need gross hacks to get something better than that, the code to get you this sourceText would need similar gross hacks.

> ...
> (I'll fix up the comments)
> ...
> > If I attach a debugger handler between an onCompilationBegin and
> > onCompilationEnd call, what happens? Do I still get the latter one? I'm not
> > sure I want one (because I won't have the other part of the pair, and it might
> > confuse my code), and yet if I don't get one I might miss info. This is a
> > vaguely interesting issue.
> 
> We have a similar issue for onScriptCreated/onScriptDestroyed. Here the answer
> can depend upon our decision regarding storing jsdICompilationUnits. Currently
> the code will not call onCompilationEnd unless there is a value for the current
> jsdICompilationUnit. The current unit is created if onCompilationBegin is
> called. If we implement the ability to create compilation units late, then we
> can set current late (that is after onCompilationBegin but before the matching
> onCompilationEnd). I believe that the only way for the debugger to get control
> currently is via onScriptCreated, so I don't think we need to worry about this
> case.

I think we need the ability to create compilation units "late", yes.

> > Then there's the whole story of having a jsdICompilationUnit that has
> > references to jsdIScripts, with the jsdIScripts also having references to the
> > jsdICompilationUnit. That's going to be a nightmare to keep from leaking,
> > unless you've got a very good story for the lifecycle of these objects.
> 
> Thanks for reminding me about this. Every call to onScriptDestroyed() will
> immediately be preceded by removing the corresponding script from the
> compilation unit. Then we can check the list and see if it is empty. If so, we
> can release the refs and set the unit isValid false. Then jsdICompilationUnit
> object lifetime can be controlled by garbage collection. 

You would still need to keep a valid ref from the jsdIScript to the jsdICU so the consumer an check which jsdICU to (possibly) throw away.


> ...
> > A bunch of the attributes on the objects also seem pointless or extremely
> > leak-prone to me (see above).
> 
> None of these attributes is pointless, but rather they are vital to making the
> debugger more correct and timely. In particular:
>  |unitType| helps us correctly describe and handle the scripts,
OK, fine, this I agree with.

>  |includer| is a dynamic replacement for the source URL/line number that is so
> problematic for dynamic code,
I disagree with this. I think it's a pain to implement and leak-prone.

>  |outerScript| points to the entry point for the file-scope, (we could use the
> jsdIScript.tag here but since jsdIScript is already jsdIEphemeral I don't the
> the value,
This last part of the sentence doesn't quite make sense... Anyway, I think you don't need it because it can simply be part of the script list. It's not that much work to get it from there...

>  |source| this is vital, we have no more than hacky ways to get it now. We
> could try to use JSDSourceText, but it looks like abandonware.
Getting it within JSD will still be hacky (eg. I bet JS only ever knows about the JS part of the source, whereas for inline JS in HTML stuff the URL+line number will be talking about the entire file, which you still won't have -- getting everything will be at least as hacky as what you have now).

> 
> I hope this addresses your concerns.

Comment 32

[John J. Barton]

(In reply to comment #31)
> (In reply to comment #30)
> > > >    readonly attribute jsdIScript     outerScript;
...
> > Correct, but we don't know which one is the outerScript. 
> 
> Sure we do, the one with a null functionName. 

Ok I propose that as part of this bug fix the null functionName is documented as always and only marking the outerScript. Then I can remove it here.
...
> > We hold a lot of references to DOM elements, one more won't make any
> > difference. We already have a non-API mechanism to avoid leaks: we delete our
> > meta-data when the nsIDOMWindow is deleted.
> 
> Who are "we"? I'm pretty sure JSD doesn't, in general. I'm:
> - not sure why you need this. Why do you need the script tag element?
> - pretty sure it will be very hard to implement (as js and jsd are oblivious as
> to where their scripts "came from")
> - afraid it will be very hard to not leak stuff. Firebug might know about
> nsIDOMWindows going away, but JSD doesn't.

Firebug certainly holds lots of refs. jsd does as well in eg functionObject.
JSD certainly knows about js contexts going away.
We absolutely need to track the origin of compilation and that origin needs to point into the page. That is what we need to support eval() and document.write() etc. 

Also note that the places where we call onCompilation() are not part of the JS engine, but they will be at the point where we call into it. So this is exactly the point where we know the context of the compile.  The JS engine can still live in its pretend world of text in, scripts out.

We can pass the |includer| via the callback, but to me this is just a huge waste. It will mean that Firebug has to create its own CompilationUnit and can't use jdsdICU.  If we do that I think we should go back to some hybrid of my first solution and this one, where we don't have jsdICU, but pass the info to the hook. But then we can't do the late recovery thing.

> 
> > > >    /**
> > > >     * Source code for this compilation unit
> > > >     */
> > > >    readonly attribute AString source;
> > 
> > > jsdIScript has an attribute "sourceText". I don't think you want to duplicate
> > > it.
> > 
> > I could not find that attribute. I guess you meant |functionSource|? If so,
> > then that attribute is incorrectly documented and it has an unfortunate name.
> > In practice accessing the attribute causes the script to be decompiled and
> > converted to a source-like text string. (I'm in favor of changing the API docs
> > and attribute name on another bug if you like).
> 
> Ah, yes, that's what I meant, sorry it was late...
> 
> I think you'll find that if you need gross hacks to get something better than
> that, the code to get you this sourceText would need similar gross hacks.

Ok, whatever it takes. I can't see how the JS engine can work on text that I cannot get a copy of.

> ...
> 
> I think we need the ability to create compilation units "late", yes.

Ok, we'll accept the overhead then.

> 
> > > Then there's the whole story of having a jsdICompilationUnit that has
> > > references to jsdIScripts, with the jsdIScripts also having references to the
> > > jsdICompilationUnit. That's going to be a nightmare to keep from leaking,
> > > unless you've got a very good story for the lifecycle of these objects.
> > 
> > Thanks for reminding me about this. Every call to onScriptDestroyed() will
> > immediately be preceded by removing the corresponding script from the
> > compilation unit. Then we can check the list and see if it is empty. If so, we
> > can release the refs and set the unit isValid false. Then jsdICompilationUnit
> > object lifetime can be controlled by garbage collection. 
> 
> You would still need to keep a valid ref from the jsdIScript to the jsdICU so
> the consumer an check which jsdICU to (possibly) throw away.

The consumer would check jsdICU.isValid.
...
I think we covered the rest above.

Comment 33

[:Gijs (out for now; he/him)]

(In reply to comment #32)
> > Who are "we"? I'm pretty sure JSD doesn't, in general. I'm:
> > - not sure why you need this. Why do you need the script tag element?
> > - pretty sure it will be very hard to implement (as js and jsd are oblivious as
> > to where their scripts "came from")
> > - afraid it will be very hard to not leak stuff. Firebug might know about
> > nsIDOMWindows going away, but JSD doesn't.
> 
> Firebug certainly holds lots of refs. jsd does as well in eg functionObject.
> JSD certainly knows about js contexts going away.

But it doesn't know how those correspond to DOM objects or not.

> We absolutely need to track the origin of compilation and that origin needs to
> point into the page. That is what we need to support eval() and
> document.write() etc. 

Why? And do you need it after that call has happened still? TBH I'd be OK with a jsdIScript ref, which is what it'd be for eval() and document.write(). JSD can deal with scripts going away easily. I don't like the DOM part of it, is all, because I don't think JSD can easily deal with that. I also think implementing the jsdIScript one should be very doable, whereas getting the right DOM elements would be harder.

> ...
> > I think you'll find that if you need gross hacks to get something better than
> > that, the code to get you this sourceText would need similar gross hacks.
> 
> Ok, whatever it takes. I can't see how the JS engine can work on text that I
> cannot get a copy of.

The point is the text the JS engine doesn't work on, as I wrote further down.
 
> > 
> > You would still need to keep a valid ref from the jsdIScript to the jsdICU so
> > the consumer an check which jsdICU to (possibly) throw away.
> 
> The consumer would check jsdICU.isValid.

No, how would it know which jsdICU to check? It's getting an onScriptDestroyed call, with no references to a jsdICU, just to a jsdIScript. Hence having to keep that reference.

> ...
> I think we covered the rest above.

I guess. I'm still not clear why you need |includer| to ever have DOM elements. I think |source| should be a followup bug if anything, because it's so hard to get, and I think we now agree that we don't need |outerScript| as such. Do you agree we might need a way to distinguish the cases without an outerscript (no script in the script list w/ no function name) between ones where there genuinely isn't one, and ones where the compile hasn't finished yet? That'd seem sane to me, anyway.

Comment 34

[John J. Barton]

I want to change my analysis of why jsdICompilationUnit can have DOM or any other refs for that matter: they can't leak by design. That is the magic in jsdIEphemeral.

When the js context is destroyed, all of its contained jsdICUs will also be set invalid. In the this process they will null all of their refs. At the end of the context destruction the jsdICUs can not hold refs to DOM or any other objects and thus cannot leak.  A debugger can still hold refs to jsdICUs after a context is destroyed but that can't cause the debugger to hold on the the DOM nodes or any other objects refed by the jsdICUs.

This the same reason jsdIScripts can't leak even though they point to JS function objects.

I hope this answers one of your major concerns.

Comment 35

[John J. Barton]

(In reply to comment #33)
> I'm still not clear why you need |includer| to ever have DOM elements.
...
> I think |source| should be a followup bug if anything, because it's so hard to

Hopefully you will agree that the support of eval(), new Function(), document.write(), script-tag injection, and inline DOM element event handlers are all lame compared to the support we have for script tags. Why is that? I believe that a large part of the problem is a mis-match between the support needed for these features and the file/lineNumber model we inherited from C language systems.

If we have an error or an issue in any code from these dynamic features, how can we help a developer understand and fix the error? The developer needs two kinds of information: the text that represents the code for the developer and the context in which that text was analyzed by the runtime.

In a C-like file-oriented world, the text is the source file and the context is the include sequence (HTML source in our case). Attempts to apply this model to the dynamic cases gives the chaotic results we see when Javascript uses any of the techniques I mention above. Through a lot of hokey code I was able to solve one of these cases most of the time. I spent a lot of time trying to crack the others without success. I think these paths are dead ends.

Now I can see that the solution is completely obvious: we need to store the text at the time is is compiled and we need to store an object-trace-back chain for the context. (This is not completely bullet proof: the context can change because of dynamics, but it's good enough and way better than now). 

For script tags in static-code pages we could fall back to using files of source code, but then we'd just have to have a special case when script tags are injected or written via document.write(). It seems more coherent to handle all of the cases by pointing to the script element.

Is this clearer and more convincing?

Comment 36

[:Gijs (out for now; he/him)]

(In reply to comment #34)
> I want to change my analysis of why jsdICompilationUnit can have DOM or any
> other refs for that matter: they can't leak by design. That is the magic in
> jsdIEphemeral.
> 
> When the js context is destroyed, all of its contained jsdICUs will also be set
> invalid. In the this process they will null all of their refs. <snip>

You're assuming that the js context will still get destroyed even if jsdICUs hold refs to DOM elements, which might hold refs to (bits of) the js context. In other words, if there's a reference cycle. There might not be, I don't know, but I haven't heard someone who knows this code well claim so, nor have I seen you providing evidence to that effect. Hence, I remain skeptical, and I'm trying to point out this problem *now* to save you or whoever implements this from headaches later :-).


(In reply to comment #35)
> (In reply to comment #33)
> > I'm still not clear why you need |includer| to ever have DOM elements.
> ...
> > I think |source| should be a followup bug if anything, because it's so hard to
> 
> Hopefully you will agree that the support of eval(), new Function(),
> document.write(), script-tag injection, and inline DOM element event handlers
> are all lame compared to the support we have for script tags. Why is that? I
> believe that a large part of the problem is a mis-match between the support
> needed for these features and the file/lineNumber model we inherited from C
> language systems.

What kind of support are we talking about? Error reporting isn't JSD's job, that's nsIScriptError and friends. Solutions for the file/lineNumber model need to include that, and that is *definitely* a separate bug (and a much more involved one, at that).

> If we have an error or an issue in any code from these dynamic features, how
> can we help a developer understand and fix the error? The developer needs two
> kinds of information: the text that represents the code for the developer and
> the context in which that text was analyzed by the runtime.
> 
> In a C-like file-oriented world, the text is the source file and the context is
> the include sequence (HTML source in our case). Attempts to apply this model to
> the dynamic cases gives the chaotic results we see when Javascript uses any of
> the techniques I mention above. Through a lot of hokey code I was able to solve
> one of these cases most of the time. I spent a lot of time trying to crack the
> others without success. I think these paths are dead ends.
> 
> Now I can see that the solution is completely obvious: we need to store the
> text at the time is is compiled and we need to store an object-trace-back chain
> for the context. (This is not completely bullet proof: the context can change
> because of dynamics, but it's good enough and way better than now). 
> 
> For script tags in static-code pages we could fall back to using files of
> source code, but then we'd just have to have a special case when script tags
> are injected or written via document.write(). It seems more coherent to handle
> all of the cases by pointing to the script element.
> 
> Is this clearer and more convincing?

Frankly, no. I don't see how a pointer to a script element (that you can't visualize except by the source code, which was a different attribute!) solves any problems for the user.

As I said earlier, all your eval/Function/document.write cases are covered by a jsdIScript ref. to the script that runs that code. If it's about a stacktrace, I think the same goes for the script tag injection (rather than a pointer to a script element in page, which won't tell you where that tag came from - it's not in the HTML source code!). For an element's inline event handler it's trivial to get the reference to the right element (which will be |this| when the handler is called).

Regardless, you seem to be presenting the following case:

1) source file URL + line numbers is shit when dealing with dynamically generated script.
2) So we need better information for the user
3...n-1 ?????
n) So we need a reference to the DOM element to which a script "belongs".

I don't see how n fits with 1 and 2. I agree with 1, and to some extent with 2 (although there's a nice debate to be had about whose responsibility it is to get that info), but I don't see how the DOM element references equal better info for the end user. If you want to know how that dynamic script came to be, I think you want a reference to the jsdscript that created it (whether it was through eval, new Function, document.write, dynamic script tag injection/modification, or some other method). Then you can actually show the user something ("I'm running this script X that you document.wrote from script Y, here's the source code"). I'm not sure what info a DOM element would give you that the source text of the script itself wouldn't.

Again, I think source text might well be very difficult (quite apart from memory issues), and would advise you to make that into a followup bug. For now let's get compilation structure right: what with jsdscripts being generated all over the place now, it's very hard to see which ones belong together, and so this API can be very useful. As-is, it also gives you info about compilation time. I think that's already a great step.

Comment 37

[:Gijs (out for now; he/him)]

(In reply to comment #36)
> Then you can actually show the
> user something ("I'm running this script X that you document.wrote from script
> Y, here's the source code"). I'm not sure what info a DOM element would give
> you that the source text of the script itself wouldn't.

Actually, upon further contemplation, even a jsdScript is much less useful than the stacktrace you get from onCompilationBegin. The script might be very big, you'll need line number/pc info to figure out where the relevant eval et al. call came from.

Comment 38

[rauldmiller]

Is this work bug going anywhere?

Specifically, is this being split into several bugs (if so, can we get links to those bugs)?

Alternatively, is this going to become a "won't fix"?  If so, could the workaround be posted?

Or perhaps this will turn into some combination of those?  (Where this bug gets split into several bugs some of which become "won't fix" with a brief description of how to work around the issue.)?

Or perhaps this bug depends on some other [perhaps unreported] bug / design issue, and can not be addressed until the other issue is resolved?  If so, could we get a link to that bug?  [If this is the case, I am guessing it has something to do with "how do we represent javascript which is not cachable".]

[My perspective: no browser that I have ever encountered implements meaningful debugging (with breakpoints or single stepping) for dynamic javascript.  I have never seen a proof of concept anywhere, for this kind of code.  As such, I think discussion about the most efficient way to implement this kind of thing should take back seat to discussion about the simplest way to implement this kind of thing.  But I do not understand firefox internals and I may be overlooking something important here, so I would rather someone else describe the way forward here.  Also: http://code.google.com/p/fbug/issues/detail?id=2912&can=1&colspec=ID%20Type%20Status%20Owner%20Test%20Summary&start=2800 ]

Comment 39

[John J. Barton]

(In reply to comment #38)
> Is this work bug going anywhere?

I continued the implementation, but then I realized there is a simpler implementation: rather than created objects in C++ we can post events at the start and end of compilation that provide the JS side with information so it can create the objects. I suppose Gijs would completely reject this approach. 

> 
> Specifically, is this being split into several bugs (if so, can we get links to
> those bugs)?

I have no such plans.

> 
> Alternatively, is this going to become a "won't fix"?  If so, could the
> workaround be posted?

I'll eventually set this to INVALID if JSDebugv2 happens.

> 
> Or perhaps this will turn into some combination of those?  (Where this bug gets
> split into several bugs some of which become "won't fix" with a brief
> description of how to work around the issue.)?
> 
> Or perhaps this bug depends on some other [perhaps unreported] bug / design
> issue, and can not be addressed until the other issue is resolved?  If so,
> could we get a link to that bug?  [If this is the case, I am guessing it has
> something to do with "how do we represent javascript which is not cachable".]

This bug and your problem does not relate to cache.

You can read about JSDebugv2, https://wiki.mozilla.org/Platform/JSDebugv2


> 
> [My perspective: no browser that I have ever encountered implements meaningful
> debugging (with breakpoints or single stepping) for dynamic javascript.  I have
> never seen a proof of concept anywhere, for this kind of code.  As such, I
> think discussion about the most efficient way to implement this kind of thing
> should take back seat to discussion about the simplest way to implement this
> kind of thing.  But I do not understand firefox internals and I may be
> overlooking something important here, so I would rather someone else describe
> the way forward here.  Also:
> http://code.google.com/p/fbug/issues/detail?id=2912&can=1&colspec=ID%20Type%20Status%20Owner%20Test%20Summary&start=2800
> ]

Firebug supports dynamic javascript via eval() for over two years. It also supports dynamic javascript via script tag injection and browser-generated even handlers.

I tried and failed to get |new Function()|; the problem is detecting when the compiler is working on such code. A simple event like "I'm starting to compile new Function()" would suffice.

I looked at your version of dynamic JS briefly, it probably isn't too hard support it. I don't know why the script tag does not appear in the DOM.