449454 - Add frame argument to every jsd callback, eg jsdIScriptHook

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect)

Description

[John J. Barton]

jsdIExecutionHook provides callbacks of the form:
 onExecute ( jsdIStackFrame frame , PRUint32 type , inout jsdIValue val)
but the callbacks for jsdIScriptHook do not provide the stack frame:
onScriptCreated ( jsdIScript script )
Since, in addition, jsdIScript provides inadequate information for debuggers, Firebug has to infer information about the script from the call stack. Which it does not have access to when the script is newly created and sent to onScriptCreated.  

The hacky workaround is to set a breakpoint on the script passed to onScriptCreated, then process the script in the onBreakpoint handler. This works but is inefficient and adds lots of complications to Firebug.
(See http://www.almaden.ibm.com/u/bartonjj/fireclipse/test/DynLoadTest/WebContent/DynamicJavascriptErrors.htm).

A much better solution would extend the callback to include "frame" argument.

This fix could be completely backward compatible.  It could be implemented as either a new argument on the current api or jsdIScriptHookWithFrames inheriting from jsdIScriptHook.  

This fix should be a relatively small bit of work. 

Verification tests would necessarily be a bit more complex:
  component to connect to jsd, set a hook to print the stack;
  open a window from the component to cause js compilations.
  compare string output from the hook to reference.

Comment 1

[John J. Barton]

Attachment: Extension to test patch for scriptHook

The extension prints info to the command line to verify 
1) That the old API still works,
2) That the new API at least gives a valid jdsIEphemeral object.

Comment 2

[John J. Barton]

The attachment from comment #1 running in 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.3) Gecko/2008092417 Firefox/3.0.3
prints:
----
WARNING: set nglayout.debug.disable_xul_fastload  and nglayout.debug.disable_xul_cache true
chromebug_command_line sets jsd service, isOn:true initAtStartup:true
createScript start
script created 1 file:/F:/bartonjj/projects/fireclipse/chromebug/chromebug/test/bugzilla-449454/[e-mail]/components/testOnScriptCreatedWithFrame.js
script created 2 file:/F:/bartonjj/projects/fireclipse/chromebug/chromebug/test/bugzilla-449454/[e-mail]/components/testOnScriptCreatedWithFrame.js
script destroyed 2 file:/F:/bartonjj/projects/fireclipse/chromebug/chromebug/test/bugzilla-449454/[e-mail]/components/testOnScriptCreatedWithFrame.js
createScript end
hooks removed
createScript start
OLD version no frame in script destroyed 3 file:/F:/bartonjj/projects/fireclipse/chromebug/chromebug/test/bugzilla-449454/[e-mail]/components/testOnScriptCreatedWithFrame.js
OLD version no frame in script destroyed 4 file:/F:/bartonjj/projects/fireclipse/chromebug/chromebug/test/bugzilla-449454/[e-mail]/components/testOnScriptCreatedWithFrame.js
OLD version no frame script destroyed 4 file:/F:/bartonjj/projects/fireclipse/chromebug/chromebug/test/bugzilla-449454/[e-mail]/components/testOnScriptCreatedWithFrame.js
createScript end
hooks removed

Comment 3

[John J. Barton]

The frame object needs a jsdthreadstate  which is created with code like this:
http://mxr.mozilla.org/mozilla-central/source/js/jsd/jsd_hook.c#165

Comment 4

[John J. Barton]

Here is how jsds_ExecutionHookProc creates a frame from a context wrapper (jsdc) and an jsdthreadstate:
    JSDStackFrameInfo *native_frame = JSD_GetStackFrame (jsdc, jsdthreadstate);
    nsCOMPtr<jsdIStackFrame> frame =
        getter_AddRefs(jsdStackFrame::FromPtr(jsdc, jsdthreadstate,
                                              native_frame));
So from a jsdc and a cx we can create a frame.

Comment 5

[John J. Barton]

The call to OnScriptCreated is in jsd_xpc.cpp, jsds_ScriptHookProc(). 

It looks like this is itself a hook handler called by jsd_NewScriptHookProc in jsd_scpt.c, registered at jsdc->scriptHook.

And jsdNewScriptHookProc is itself a hook handler defined in jsscript.cpp and registered in cx->debugHooks->newScriptHook;

To add a 'frame' argument to OnScriptCreated we have to 
1) modify jsd_NewScriptHookProc() to compute the frame from the jsdc and cx available there. 
and
2) pass the frame into jsds_ScriptHookProc() as i) a new argument, ii) new field in an existing argument, or iii) some global. 

Arguments:
jsds_ScriptHookProc (JSDContext* jsdc, JSDScript* jsdscript, JSBool creating,
                     void* callerdata)

Note that jsd_NewScriptHookProc() obtains jsdc from the void* callerdata.
Note that jsds_ScriptHookProc does not use its callerdata.

So one strategy to avoid changing the argument list would pass frame as the callerdata in the case that the scriptHookData was null:

    /* local in case jsdc->scriptHook gets cleared on another thread */
    JSD_LOCK();
    hook = jsdc->scriptHook;
    hookData = jsdc->scriptHookData;
    JSD_UNLOCK();

    if ( ! hookData)
        hookData = jsd_getStackFrame(jsdc, cx);

    if( hook )
        hook(jsdc, jsdscript, JS_TRUE, hookData);

where jsd_getStackFrame() implements the code like comment 4. At worst other users of jsd  who also don't use scriptHookData would get an extra argument.

This is slightly cheap, but changing the jsd_ScriptHookProc API to add an argument may be more painful than its worth.

Comment 6

[John J. Barton]

Ok the first effort fails. Apparently the jsd_NewThreadState is confused because the newly created script is on the top of the stack cause the top frame pointer to be "not native" and yet jsd_FindJSDScript does not find the script in the context hashtable. A proper frame cannot be constructed this way.

Comment 7

[David Mandelin [:dmandelin]]

Presumed taken care of in jsdbg2.