Last Comment Bug 595243 - Expose debugMode to JSD
: Expose debugMode to JSD
Status: RESOLVED FIXED
[firebug-p1] fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows Vista
: -- normal with 3 votes (vote)
: ---
Assigned To: Robert Sayre
: jsd
Mentors:
: 585233 (view as bug list)
Depends on: 642801 595837 614557
Blocks: JaegerDebug 594054 595743
  Show dependency treegraph
 
Reported: 2010-09-10 10:35 PDT by Robert Sayre
Modified: 2011-07-08 00:24 PDT (History)
29 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta7+


Attachments
expose it (2.06 KB, patch)
2010-09-10 10:39 PDT, Robert Sayre
no flags Details | Diff | Review
use public API everywhere (2.06 KB, patch)
2010-09-10 10:44 PDT, Robert Sayre
odvarko: review-
Details | Diff | Review
Make JSD activation asynchronous (18.52 KB, patch)
2010-10-06 15:28 PDT, Robert Sayre
no flags Details | Diff | Review
revised patch (24.29 KB, patch)
2010-10-21 01:03 PDT, Robert Sayre
no flags Details | Diff | Review
partial patch for Firebug (2.51 KB, patch)
2010-10-21 01:07 PDT, Robert Sayre
no flags Details | Diff | Review
rebased (23.96 KB, patch)
2010-10-29 11:23 PDT, Robert Sayre
gal: review+
Details | Diff | Review
only compile main thread scripts for debug mode, and only do it from the main thread (8.06 KB, patch)
2010-10-30 11:53 PDT, Robert Sayre
gal: review+
Details | Diff | Review

Description Robert Sayre 2010-09-10 10:35:14 PDT
+++ This bug was initially created as a clone of Bug #594054 +++

This will help Firebug not crash on JM.
Comment 1 Robert Sayre 2010-09-10 10:39:11 PDT
Created attachment 474116 [details] [diff] [review]
expose it
Comment 2 Robert Sayre 2010-09-10 10:41:32 PDT
Comment on attachment 474116 [details] [diff] [review]
expose it

> If I understand it well JSContext (C++) is accessible through jsdIContext
> interface so, what about having an additional method setDebugMode(debug) in it?
>
> void setDebugMode (in boolean debug);
> 
> As soon as there is such method, I'll patch Firebug to use it.

honza, could you tell me whether this works?
Comment 3 Robert Sayre 2010-09-10 10:44:19 PDT
Created attachment 474118 [details] [diff] [review]
use public API everywhere

same patch, just using JS_* functions in both places.
Comment 4 John J. Barton 2010-09-13 07:09:10 PDT
Unfortunately there does not seem to be any documentation on jsdIContext. We don't know how they are related to our objects. How do we know which one to call setDebugMode() on and which objects will be affected? When does a new jsdIContext appear and how can we find that out? Are these objects related to nsIDOMWindows? setTimeouts?
Comment 5 Jan Honza Odvarko [:Honza] 2010-09-13 08:27:11 PDT
The idea was to call context.debugMode = true for a script being created. I didn't realize that script.JSDContext is not scriptable, but if it would, we could enable particular context within onScriptCreated like as follows:

onScriptCreated: function(script)
{
    script.JSDContext.debugMode = true;
}

Thus we wouldn't need an event for context creation, since we would enable it with the first script, does that make sense?

And if jsdIContext had a parent window, we could easily filter out those contexts that don't belong to the debugged tab/window.

Honza
Comment 6 John J. Barton 2010-09-13 08:45:27 PDT
Is onScriptCreated called for scripts in a jsdIConext with debugMode===false?

In general we can't tell in onScriptCreated whether or not the script is one we want to debug (because we have no stack frame, no scope). So we have to set a breakpoint on the first PC of the script, then wait until it hits to decide. So I guess we need to set debugMode true always, then set it to false later.
Comment 7 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-13 08:51:54 PDT
Is Firebug stable on beta builds to date? Since the bug indicated by sayrer is related to APIs, I'm wondering if this (and that!) should block beta 6.
Comment 8 John J. Barton 2010-09-13 09:05:37 PDT
Firebug worked yesterday (and chromebug for the first time yay!). Today it crashes on bug 595743
Comment 9 Jan Honza Odvarko [:Honza] 2010-09-13 09:11:43 PDT
Honza(In reply to comment #7)
> Is Firebug stable on beta builds to date? Since the bug indicated by sayrer is
> related to APIs, I'm wondering if this (and that!) should block beta 6.
Both, this bug and bug 595743 should be marked as blockers. Firebug needs a stable way to enable debug mode introduced in JaegerMonkey (and not enabling the mode should not cause crash).

Honza
Comment 10 Jan Honza Odvarko [:Honza] 2010-09-13 09:19:35 PDT
(In reply to comment #6)
> Is onScriptCreated called for scripts in a jsdIConext with debugMode===false?
Yes

Honza
Comment 11 John J. Barton 2010-09-13 09:36:25 PDT
(In reply to comment #10)
> (In reply to comment #6)
> > Is onScriptCreated called for scripts in a jsdIConext with debugMode===false?
> Yes

Ok then I think we only need to do is in your comment #5 expose jsdIScript.JSDContext and set debugMode on in onScriptCreated. 

We control onScriptCreated via jsd.on/off(), jsd.pause/unpause(), and jsdIFilter, so we don't need to control which scripts call debugMode.  

But what about the reverse? I guess we have to enumerate the jsdIScripts when we pause and set jsdIScript.jsdIContext.debugMode false, then revert it when we unpause? Or will the engine do this?

This all sounds like a lot of ways to create bugs. There are too many ways to sort of control things we don't really understand.
Comment 12 Jan Honza Odvarko [:Honza] 2010-09-13 09:44:27 PDT
> Ok then I think we only need to do is in your comment #5 expose
> jsdIScript.JSDContext and set debugMode on in onScriptCreated. 
> 
> We control onScriptCreated via jsd.on/off(), jsd.pause/unpause(), and
> jsdIFilter, so we don't need to control which scripts call debugMode.  
But this would mean that we would enable even scripts that don't belong to the debugged window, correct?

> But what about the reverse? I guess we have to enumerate the jsdIScripts when
> we pause and set jsdIScript.jsdIContext.debugMode false, then revert it when we
> unpause? Or will the engine do this?
We could also enumerate all contexts directly using jsdIDebuggerService.enumerateContexts
Again, if every context had a parent window we could optimize.

> This all sounds like a lot of ways to create bugs. There are too many ways to
> sort of control things we don't really understand.
So, what would be the ideal APIs (managing debugMode) for Firebug?

Honza
Comment 13 John J. Barton 2010-09-13 10:09:12 PDT
(In reply to comment #12)
> > Ok then I think we only need to do is in your comment #5 expose
> > jsdIScript.JSDContext and set debugMode on in onScriptCreated. 
> > 
> > We control onScriptCreated via jsd.on/off(), jsd.pause/unpause(), and
> > jsdIFilter, so we don't need to control which scripts call debugMode.  
> But this would mean that we would enable even scripts that don't belong to the
> debugged window, correct?

We use jsdIFilter to skip all debugging for chrome. We turn off jsd if the selected Firefox tab is not a debugged window. Scripts introduced in a debugged window can get lost if the user selected a non-debug window during load or during ajax calls that load code.  No one has reported this bug, I guess because they can't reproduce it.

> 
> > But what about the reverse? I guess we have to enumerate the jsdIScripts when
> > we pause and set jsdIScript.jsdIContext.debugMode false, then revert it when we
> > unpause? Or will the engine do this?
> We could also enumerate all contexts directly using
> jsdIDebuggerService.enumerateContexts
> Again, if every context had a parent window we could optimize.

We need to know the relationship between jsdIContext and nsIDOMWindow even in the non-optimize case. Otherwise all we can do is turn debugMode all on.

> 
> > This all sounds like a lot of ways to create bugs. There are too many ways to
> > sort of control things we don't really understand.
> So, what would be the ideal APIs (managing debugMode) for Firebug?

Since debugMode is a property of jdsIContext and we don't know what a jsdIContext is, I can't answer that.  Every jsdIScript has a jsdIContext; some jsdIContext.globalObject == an nsIDOMWindow. that is pretty much all we know.

If we can't unravel these details, then the ideal API would be 'none': if jsd is active then all jsdIContext.debugMode == true, else false.  

> 
> Honza
Comment 14 timeless 2010-09-13 18:06:02 PDT
why does jsdI need to expose this? why can't/shouldn't jsdI just do the right thing? surely if someone is trying to set a breakpoint they want to be in debug mode.
Comment 15 Jan Honza Odvarko [:Honza] 2010-09-14 12:02:15 PDT
Comment on attachment 474118 [details] [diff] [review]
use public API everywhere

This doesn't seem to be the proper solution and I agree with timeless that JSD should set the debugMode automatically when jsdService::On() is called (ie Firebug's debugger enabled).

Would it be possible to do it for all compartments in the given runtime (e.g within the jsdService::OnForRuntime() method) ?

What do you think?

Honza
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2010-09-28 21:30:00 PDT
Is this still required to get Firebug working with Beta 7, or will that be resolved by bug 599400? Speculatively setting this to block b7 until Honza gives us word.
Comment 17 Jan Honza Odvarko [:Honza] 2010-09-30 09:09:03 PDT
(In reply to comment #16)
> Is this still required to get Firebug working with Beta 7, or will that be
> resolved by bug 599400? Speculatively setting this to block b7 until Honza
> gives us word.

Yes it's still needed.

The bug 599400 was about a crash that happened when the debugMode was forced to true (using a C++ patch, attachment 465522 [details] [diff] [review]). This bug is about APIs that allows forcing the debugMode from Javascript - or - automate debugMode as needed (e.g. when jsd.on() is called).

Honza
Comment 18 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-04 08:41:50 PDT
OK, then it needs an ETA for completion.
Comment 19 Robert Sayre 2010-10-05 08:56:10 PDT
I pretty much got this working yesterday, by placing an event on the Gecko event loop. This *usually* gets a you a place that's to recompile everything, but there's a catch. Sometimes, a Sync XHR or similar will spin the event loop with a bunch of JS on the stack. So, that's a failure.

The backup plan is to set some flags on the nsXPConnect singleton, and wait until the context stack is at zero. This should work, it's just much uglier.
Comment 20 Jan Honza Odvarko [:Honza] 2010-10-05 12:47:38 PDT
Great to see the progress on this!

Note that testing Firebug's debugger features is possible only with manual patching C++ code (forcing debugMode to true by default), which prevents Firebug users from using nightlies - and us (Firebug team) from getting feedback from them.

Honza
Comment 21 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-06 15:06:25 PDT
Please note that we have now created a branch for beta 7 work. In addition to landing your fix on mozilla-central default, please also land it on mozilla-central GECKO20b7pre_20101006_RELBRANCH

(note: when landing on mozilla-central default, you will be given priority in the landing queue at https://wiki.mozilla.org/LandingQueue )
Comment 22 Robert Sayre 2010-10-06 15:28:55 PDT
Created attachment 481355 [details] [diff] [review]
Make JSD activation asynchronous

This was the only way I could get a hook with no script on the stack. It's ugly, but it does seem to work. I can get Firebug to enter and exit script debugging mode.

Firebug expects to be able to do this operation synchronously:

> jsd = DebuggerService.getService(jsdIDebuggerService);
> 
> jsd.on();
> jsd.flags |= DISABLE_OBJECT_TRACE;
> this.hookScripts();
> 
> jsd.debuggerHook = { onExecute: hook(this.onDebugger, RETURN_CONTINUE) };
> jsd.debugHook = { onExecute: hook(this.onDebug, RETURN_CONTINUE) };
> jsd.breakpointHook = { onExecute: hook(this.onBreakpoint, RETURN_CONTINUE) };
> jsd.throwHook = { onExecute: hook(this.onThrow, RETURN_CONTINUE_THROW) };
> jsd.errorHook = { onError: hook(this.onError, true) };

but we have to find a time with no script on the stack. So remaining adjustment will need to occur.
Comment 23 Robert Sayre 2010-10-06 15:29:46 PDT
Comment on attachment 481355 [details] [diff] [review]
Make JSD activation asynchronous

Asking for review/feedback to check that this approach is sane. I know the patch has some printfs and things in it.
Comment 24 Robert Sayre 2010-10-06 15:32:50 PDT
Comment on attachment 481355 [details] [diff] [review]
Make JSD activation asynchronous

>diff --git a/dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp
> 
>     // Check if there's a user for the debugger service that's 'on' for us
>     if (NS_SUCCEEDED(rv)) {
>       jsds->GetDebuggerHook(getter_AddRefs(jsdHook));
>       jsds->GetIsOn(&jsds_IsOn);
>-      if (jsds_IsOn) { // If this is not true, the next call would start jsd...
>-        rv = jsds->OnForRuntime(cx->runtime);
>-        jsds_IsOn = NS_SUCCEEDED(rv);
>-      }

I renamed OnForRuntime. This was the only in-tree user, and that call will always exit immediately, since jsds->GetIsOn would only be true if OnForRuntime had already been called. So I removed this code.
Comment 25 Robert Kaiser (not working on stability any more) 2010-10-06 15:53:24 PDT
I think venkman might need a change as well if semantics of JSD activation change, so bringing people who can potentially fix that into the loop.
Comment 26 John J. Barton 2010-10-06 16:25:42 PDT
(In reply to comment #22)
> Created attachment 481355 [details] [diff] [review]
> Make JSD activation asynchronous
...
> but we have to find a time with no script on the stack. So remaining adjustment
> will need to occur.

I understand that jsd.on() will return without the debugger being activated. But I can't figure out how to determine when it is active. Do we poll jsd.on until it is true?

I assume that once jsd is activated, the JIT is off until jsd.off() is called. In particular I assume that jsd.pause() only deactivates the debugging but does not turn on JIT actions.
Comment 27 David Mandelin [:dmandelin] 2010-10-06 17:28:44 PDT
Comment on attachment 481355 [details] [diff] [review]
Make JSD activation asynchronous

>diff --git a/dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp

>+    [noscript] void ActivateDebugger(in JSRuntime rt);
>+
>+    /**
>+     * 
>+     */
>+    [noscript] void RecompileForDebugMode(in JSRuntime rt, in PRBool mode);
>+

Empty comment block above--looks like you meant to put a doc comment there.

>diff --git a/js/jsd/jsd_scpt.c b/js/jsd/jsd_scpt.c

>-#ifdef LIVEWIRE
>-    if( 1 == lineno )
>-        jsdlw_PreLoadSource(jsdc, LWDBG_GetCurrentApp(), filename, JS_TRUE );
>-#endif
>-    

I have no idea what this is for, I assume it's obsolete?

>+    /* We use the private API here, because the public one checks
>+       for script on the stack that we have already compiled as
>+       debugMode code. */
>+    if (!js_SetDebugMode(cx, JS_TRUE)) {
>+        printf("js_SetDebugMode failed?\n");
>+        return;
>+    }
>+

Style nit on stars in the comment, which is a bit confusingly worded. 

But I think there is a more direct fix anyway. I think what the comment means is
that JS_SetDebugMode asserts that there are no JSScripts on the stack, which we 
won't want here because we're calling it multiple times per context. Instead, we
could just make JS_SetDebugMode(cx, b) do nothing if debug mode is already |b|,
and then call the API version, and then it all works, right?

> NS_IMETHODIMP
>-jsdService::OnForRuntime (JSRuntime *rt)
>+jsdService::RecompileForDebugMode (JSRuntime *rt, JSBool mode) {
>+  JSContext *cx;
>+  JSContext *iter = NULL;
>+
>+  printf("RecompileForDebugMode: %d", mode);
>+  while ((cx = JS_ContextIterator (rt, &iter))) {
>+      JS_SetDebugMode(cx, mode);
>+  }
>+
>+  return NS_OK;
>+}

Stray printf left in?

>+
>+    /**
>+     * When we place the browser in JS debug mode, there's can't be any
>+     * JS on the stack. This method will schedule turn debug mode on or
>+     * off when the context stack reaches zero length.
>+     */
>+    [noscript] void setDebugModeWhenPossible(in PRBool mode);

Some typos in the comment ("there's", "turn"). Also more precise wording is
"When we change the JS debug mode, ..."

The condition given in the comment is a conservative approximation to what's
really required. It may (or may not) make sense to note that.

>+void 
>+nsXPConnect::CheckForDebugMode(JSRuntime *rt) {
>+    if (gDebugMode != gDesiredDebugMode) {
>+        nsresult rv;
>+        const char jsdServiceCtrID[] = "@mozilla.org/js/jsd/debugger-service;1";
>+        nsCOMPtr<jsdIDebuggerService> jsds = do_GetService(jsdServiceCtrID, &rv);
>+        if (NS_SUCCEEDED(rv)) {
>+            if (gDesiredDebugMode == PR_FALSE) {
>+                rv = jsds->RecompileForDebugMode(rt, PR_FALSE);
>+            } else {
>+                rv = jsds->ActivateDebugger(rt);
>+            }
>+        }
>+        if (NS_SUCCEEDED(rv))
>+            gDebugMode = gDesiredDebugMode;
>+        else
>+            gDesiredDebugMode = gDebugMode;

I think the intent on the previous line of code is to cancel the debug mode
request if the attempt to change it failed. It's probably nice to have a
comment either there or on the function as a whole to clarify the intent for
readers.

> /* JSContext Pop (); */
> NS_IMETHODIMP
> nsXPConnect::Pop(JSContext * *_retval)
> {
>     XPCPerThreadData* data = XPCPerThreadData::GetData(nsnull);
> 
>     if(!data)
>     {
>         if(_retval)
>             *_retval = nsnull;
>         return NS_ERROR_FAILURE;
>     }
> 
>-    return data->GetJSContextStack()->Pop(_retval);
>+    nsresult rv;
>+    rv = data->GetJSContextStack()->Pop(_retval);
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    PRInt32 count;
>+    rv = data->GetJSContextStack()->GetCount(&count);
>+    if (NS_SUCCEEDED(rv) && count == 0)
>+        CheckForDebugMode(mRuntime->GetJSRuntime());
>+    
>+    return rv;

Looks right to me. It may be slightly more efficient to check that there is
actually a pending debug mode request change before querying the count, but
maybe not--I suppose they're both pretty simple tests.

> /* void Push (in JSContext cx); */
> NS_IMETHODIMP
> nsXPConnect::Push(JSContext * cx)
> {
>     XPCPerThreadData* data = XPCPerThreadData::GetData(cx);
> 
>     if(!data)
>         return NS_ERROR_FAILURE;
> 
>+    PRInt32 count;
>+    nsresult rv;
>+    rv = data->GetJSContextStack()->GetCount(&count);
>+    if (NS_FAILED(rv))
>+        return rv;
>+
>+    if (count == 0)
>+        CheckForDebugMode(mRuntime->GetJSRuntime());
>+
>     return data->GetJSContextStack()->Push(cx);

I was a little surprised to see this--I would expect we had already set
debug mode if the stack is empty. I know this isn't the case with the
current SetDebugModeWhenPossible (see also below). It seems that ideally
we wouldn't need this check here. But, if there are conditions where
it can happen, then I suppose this is the most reliable fix for that.


>+NS_IMETHODIMP
>+nsXPConnect::SetDebugModeWhenPossible(PRBool mode)
>+{
>+    gDesiredDebugMode = mode;
>+    return NS_OK;
>+}
>+

It seems like it would be good to turn on debug mode immediately if possible.
I guess that could be done simply by calling CheckDebugMode here.
Comment 28 timeless 2010-10-06 18:22:02 PDT
lw was livewire (google: netscape java livewire).

the block you're removing was added in bug 341764. offhand it just looks like he got the logic wrong (gijs was new to c++ at the time iirc). I'd rather you just fix the logic....

since you're already breaking the vtable order, please put off() right after on().

the indentation here is wrong (i think you're using 2-4 instead of 4-4):
+  while ((cx = JS_ContextIterator (rt, &iter))) {
+      JS_SetDebugMode(cx, mode);

     xpc->InitClasses (cx, glob);
+    xpc->SetDebugModeWhenPossible(PR_TRUE);

file style in some portions (really more like per function style) is clearly function (args) not function(args). I don't like the style, but I never got around to enforcing another. Please consider conforming.
Comment 29 Paul Wright 2010-10-16 16:53:48 PDT
Is this bug fixed?  There has been no activity (at least no commented activity) since 10-6, has ETA 10/5 in the whiteboard, and is marked as blocking beta 7.
Comment 30 Robert Sayre 2010-10-18 11:49:10 PDT
I'll have a new patch up for this shortly
Comment 31 Tomas 2010-10-20 03:58:47 PDT
Any news?
Comment 32 erpman1 2010-10-20 10:32:34 PDT
(In reply to comment #30)
> I'll have a new patch up for this shortly

will the new patch be available by the end of this week, Robert Sayre?

(In reply to comment #31)
> Any news?

none yet, Tomas.  we'll have to wait longer.
Comment 33 Robert Sayre 2010-10-21 01:03:15 PDT
Created attachment 484983 [details] [diff] [review]
revised patch

this seems to work, but the Firebug UI isn't fully ready for the asynchronous interface.
Comment 34 Robert Sayre 2010-10-21 01:07:10 PDT
Created attachment 484984 [details] [diff] [review]
partial patch for Firebug

I was able to verify that debugMode was successfully turned off/on with this patch. The Firebug UI seems to look for jsd.isOn synchronously and thus doesn't activate.
Comment 35 John J. Barton 2010-10-21 08:18:25 PDT
(In reply to comment #33)
> this seems to work, but the Firebug UI isn't fully ready for the asynchronous
> interface.

Excellent news! What is the new API?
Comment 36 John J. Barton 2010-10-21 08:51:01 PDT
Ok I see the .idl updates in comment 34. So all we need know how we trigger the callback jsd.asyncOn().  That is: the user has ordered us to activate debugging. We call jsd.asyncOn() and... what ?  I suppose the answer might be: return to the UI event loop, eg we would call setTimeout( ourFunctionToBeginDebugging ). Is it correct?
Comment 37 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-10-21 09:13:33 PDT
If you look at the IDL (which is in the first attachment to this bug)... asyncOn takes an argument.  A jsdIActivationCallback, to be exact.  And then:

+[scriptable, uuid(6da7f5fb-3a84-4abe-9e23-8b2045960732)]
+interface jsdIActivationCallback : nsISupports
+{
+    void onDebuggerActivated ();
+};

sayrer, if you toss "function" into those interface flags, then JS callers will be able to just pass in a JS function object as the jsdIActivationCallback, and calling OnDebuggerActivated on the C++ end will automagically call the function that was passed in.  Then Firebug does:

   asyncOn(function() { /* all debugging is go */ });

and life should be good.
Comment 38 Mike Beltzner [:beltzner, not reading bugmail] 2010-10-21 21:08:44 PDT
JJB, is this enough for you to go on? Would like to know if there's further action required from the JS team on this.
Comment 39 John J. Barton 2010-10-22 09:48:49 PDT
I think 'yes' we can go on. As soon as this appears in a nightly we will try it. We don't have any way to know it will work/not work otherwise. We don't have any model for when the activation callback will be called. 

I guess that the callback will be fired after the caller of jsd.asyncOn() returns and its caller, recursively, returns until there is no JS calls. Thus we need to 1) not call web page code in the call stack we call jsd.asyncOn() and 2) the platform cannot call web page code on that call stack.

We currently turn jsd on during onLocationChange and onStateChange events. I believe that both of these event handlers empty the JS stack when those methods return and the platform does not call to Web page JS during these events. 

I'm less confident about chromebug since I have less experience. But there we turn on jsd during profile-after-change, so I expect that at worst other profile-after-change handlers will be not be debugged.

Finally, we need to test with this patch because currently some 50+ Firebug tests fail because the jsd is not active: we can not really answer the broader question about JS debug with the new JS engine until this patch is in and we are able to run our tests.
Comment 40 Andreas Gal :gal 2010-10-22 15:09:43 PDT
We can take out scanning for an empty stack in Pop and send the notification in Push onto an empty stack only since both happen consecutively.
Comment 41 Damon Sicore (:damons) 2010-10-26 10:46:25 PDT
Andreas, have a patch yet?
Comment 42 Damon Sicore (:damons) 2010-10-26 10:48:03 PDT
Sorry, hard to tell what's next on this bug.  Please advise.
Comment 43 Andreas Gal :gal 2010-10-26 14:13:18 PDT
JSD was crashing in other bugs that block b7 as well. I will retest and see if we are ready to land this patch.
Comment 44 Damon Sicore (:damons) 2010-10-29 09:46:33 PDT
Can we land this please?
Comment 45 Robert Sayre 2010-10-29 11:23:37 PDT
Created attachment 486965 [details] [diff] [review]
rebased
Comment 46 Andreas Gal :gal 2010-10-29 15:04:50 PDT
Comment on attachment 486965 [details] [diff] [review]
rebased

God is this ugly.
Comment 47 John J. Barton 2010-10-29 15:48:45 PDT
Just a suggestion without any idea of what problems you face: we could probably deal with a jsd-activation solution that was "you get called when the first call stack of this nsIDOMWindow begins and you have to say yes or no on debugging then". Synchronous, but no calls have happened.
Comment 48 Johnny Stenback (:jst, jst@mozilla.com) 2010-10-29 18:25:45 PDT
For the record, the patch for jsd in this bug (attachment "rebased") landed on mozilla-central. The remaining parts here is to fix up Firebug.
Comment 49 Jeff Walden [:Waldo] (remove +bmo to email) 2010-10-29 23:04:58 PDT
I backed the changes here out of TM, because they were turning mochitest-plain-4 orange.
Comment 50 Robert Sayre 2010-10-30 09:00:13 PDT
(In reply to comment #49)
> I backed the changes here out of TM, because they were turning
> mochitest-plain-4 orange.

There was a test using the old API, which now throws (good). Fixing up.
Comment 51 Robert Sayre 2010-10-30 09:19:15 PDT
fixed up the test:

http://hg.mozilla.org/tracemonkey/rev/05e76ef439fd
Comment 52 John J. Barton 2010-10-30 09:49:13 PDT
(In reply to comment #48)
> For the record, the patch for jsd in this bug (attachment "rebased") landed on
> mozilla-central. The remaining parts here is to fix up Firebug.

r8215 on https://fbug.googlecode.com/svn/branches/firebug1.7 for 1.7a5
Comment 53 Robert Sayre 2010-10-30 11:53:44 PDT
Created attachment 487182 [details] [diff] [review]
only compile main thread scripts for debug mode, and only do it from the main thread
Comment 55 Andreas Gal :gal 2010-11-01 02:21:00 PDT
sayrer, sounds like firebug is having problems with the new API.

https://bugzilla.mozilla.org/show_bug.cgi?id=595743#c37
Comment 56 Jan Honza Odvarko [:Honza] 2010-11-01 08:08:47 PDT
(In reply to comment #52)
> (In reply to comment #48)
> > For the record, the patch for jsd in this bug (attachment "rebased") landed on
> > mozilla-central. The remaining parts here is to fix up Firebug.
> 
> r8215 on https://fbug.googlecode.com/svn/branches/firebug1.7 for 1.7a5

I did a small modification at R8232 (Firebug SVN) but, the context.jsDebuggerActive is never set to true. It looks like Debugger.supportsGlobal is never called. Does that mean that fbs can't find the registered Debugger?

Honza
Comment 57 John J. Barton 2010-11-01 08:31:55 PDT
Last night's build has a broken jsd:

Error: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [jsdIDebuggerService.flags]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///C:/jjb/eclipse/fbugWorkspace/fbug/chromebug/branches/chromebug1.7/components/chromebug-startup-observer.js :: anonymous :: line 101"  data: no]
Source File: file:///C:/jjb/eclipse/fbugWorkspace/fbug/chromebug/branches/chromebug1.7/components/chromebug-startup-observer.js
Line: 101
Comment 58 John J. Barton 2010-11-01 09:01:30 PDT
Ok I see that comment 56 was to fix the issue in comment 57. I made more changes, but we still don't know what the API is, so we're just guessing.
Comment 59 John J. Barton 2010-11-01 09:41:54 PDT
Our initial tests show that jsd is not obeying setBreakpoint(0). We are creating a test case.

As far as this particular bug is concerned, the activation of jsd with the FF 4.0 JIT, we can call asyncOn and see onScriptCreated calls. We don't know yet if we can turn jsd off and back on again.
Comment 60 Jan Honza Odvarko [:Honza] 2010-11-01 10:41:55 PDT
I have created Bug 608763 with STR and a test case for the setBreakpoint(0) problem.

Honza
Comment 61 Robert Sayre 2010-11-01 12:02:32 PDT
(In reply to comment #58)
> Ok I see that comment 56 was to fix the issue in comment 57. I made more
> changes, but we still don't know what the API is, so we're just guessing.

I don't understand what's missing. There is an IDL file with an interface description and comments alongside it. I also wrote a patch for Firebug in attachment 484984 [details] [diff] [review], and verified that I could turn JSD on and off. What additional information is needed?

I will help get Firebug up this week. We can start in bug 608763, if that's ok.
Comment 62 John J. Barton 2010-11-01 14:06:16 PDT
As I was trying to explain in comment 39, the async API needs additional information so we can reason about the application we write to start jsd.

The IDL says:
     * The debugger will be activated asynchronously, because there can be no JS
     * on the stack when code is to be re-compiled for debug mode.
But we have no means to use the API correctly. All the comment says is that we call asyncOn() and any amount of time later jsd is active. Ok we can infer that the comment means:
      * The debugger will be activated at the earliest time that no JS
      * on the stack.
However, we have no direct control over the JS stack. So the effective API is:
      * The debugger will be activated sometime after this call. If some
      * important JS things happen before onDebuggerActivate you won't 
      * know about it. Just run a lot of test cases and try to figure 
      * out if things are working.
To be fair that is already pretty much the case for other parts of the jsd API, but I'm hoping we can improve things over time.

So under what circumstances can we call jsd.asyncOn() and know that the call stack will become empty (and thus jsd will be on) before any Javascript activity of interest occurs?
Comment 63 Robert Sayre 2010-11-01 18:50:07 PDT
(In reply to comment #62)
> 
> So under what circumstances can we call jsd.asyncOn() and know that the call
> stack will become empty (and thus jsd will be on) before any Javascript
> activity of interest occurs?

I see where the misunderstanding lies. In Firefox 3.6, jsd.on() is called from JS, which means that the JSRuntime immediately starts adding debugging hooks for all new scripts, including those that comprise Firebug. This is actually not very valuable.

As a demonstration, let's suppose jsd.on() was called like this:

jsd.on = true; // <-- debugger activated immediately
eval("var foo = 42");

that means that the new JSScript from eval (with the JSD-activating code on the stack) would be hooked here:

[new script and hook]
> eval() //preceeded by jsd.on
> firebug.enableDebugger
> firbugservice...

but that isn't particularly valuable. that's all firebug code. With new way, it would go like this:

> jsd.asyncOn()
> firebug.enableDebugger
> firbugservice...

and wouldn't get enabled until the JS stack unwinds, back through enableDebugger and the rest of the Firebug service. But as soon as that stack unwinds, the onDebuggerActivated callback will get executed before any other JS.
Comment 64 Brendan Eich [:brendan] 2010-11-01 20:40:00 PDT
(In reply to comment #62)
> The IDL says:
>      * The debugger will be activated asynchronously, because there can be no
> JS
>      * on the stack when code is to be re-compiled for debug mode.

This comment could be more specific, for sure. It's fair to criticize on that basis. But it should be clear from the comments, if not the code in the patch, that the current event's stack has to go empty, so your worries in comment 39 about not calling other JS on the same stack before it goes empty are not valid.

> But we have no means to use the API correctly. All the comment says is that we
> call asyncOn() and any amount of time later jsd is active.

The comment is trying to be more precise about when, although it is a bit hard to understand. It is *not* saying "any amount of time later". The debugger can run after the current event's stack empties and control returns to the event loop.

> Ok we can infer that the comment means:
>       * The debugger will be activated at the earliest time that no JS
>       * on the stack.
> However, we have no direct control over the JS stack. So the effective API is:
>       * The debugger will be activated sometime after this call. If some
>       * important JS things happen before onDebuggerActivate you won't 
>       * know about it. Just run a lot of test cases and try to figure 
>       * out if things are working.

Were you being sarcastic or did you really think this was what we proposed? Hard to believe the last, but moving on:

> To be fair that is already pretty much the case for other parts of the jsd API,
> but I'm hoping we can improve things over time.

John, this is simply not accurate if (as proposed), onDebuggerActivated is called on the next event loop turn to let the debugger know that the it is safe to run then. Is that callback not happening, or did you not wire it up?

/be
Comment 65 John J. Barton 2010-11-01 21:52:01 PDT
(In reply to comment #64)
> (In reply to comment #62)
> > The IDL says:
> >      * The debugger will be activated asynchronously, because there can be no
> > JS
> >      * on the stack when code is to be re-compiled for debug mode.
> 
> This comment could be more specific, for sure. It's fair to criticize on that
> basis. But it should be clear from the comments, if not the code in the patch,
> that the current event's stack has to go empty, so your worries in comment 39
> about not calling other JS on the same stack before it goes empty are not
> valid.

That is what I wanted someone to say.
...
> 
> > Ok we can infer that the comment means:
> >       * The debugger will be activated at the earliest time that no JS
> >       * on the stack.
> > However, we have no direct control over the JS stack. So the effective API is:
> >       * The debugger will be activated sometime after this call. If some
> >       * important JS things happen before onDebuggerActivate you won't 
> >       * know about it. Just run a lot of test cases and try to figure 
> >       * out if things are working.
> 
> Were you being sarcastic or did you really think this was what we proposed?

Sorry, I have to say it does sound sarcastic. I was exaggerating to try to get you to see things from my point of view.  I'm completely fed up with having to reverse engineer to get things to work.  It's time to stop writing crap code because we can't communicate about the system. 

> Hard to believe the last, but moving on:
> 
> > To be fair that is already pretty much the case for other parts of the jsd API,
> > but I'm hoping we can improve things over time.
> 
> John, this is simply not accurate if (as proposed),

I guess we should avoid a discussion my assessment of jsd ;-)

This would be a great way to say it in the IDL:
> onDebuggerActivated is
> called on the next event loop turn to let the debugger know that the it is safe
> to run then. 

or the earlier one:
> The debugger can
> run after the current event's stack empties and control returns to the event
> loop.

> Is that callback not happening, or did you not wire it up?

Yes the callback happens. 

Thanks for your patience and explanation.
Comment 66 Brendan Eich [:brendan] 2010-11-01 22:30:39 PDT
(In reply to comment #65)
> That is what I wanted someone to say.

Ok. I figured it out from the patch description and comments. It took a little leap of faith, but not much. YMMV.

> Sorry, I have to say it does sound sarcastic. I was exaggerating to try to get
> you to see things from my point of view.  I'm completely fed up with having to
> reverse engineer to get things to work.  It's time to stop writing crap code
> because we can't communicate about the system. 

I don't recall writing any crap code here. Let's try to stick to fixable problems instead of generalized negatives, while we are still here trying to do something better.

On the general rotting state of jsd, it's our fault for not putting someone on the case earlier, relying on the kindness of timeless. We aimed to fix that with jimb but he was pulled into ES5 work. He's coming back and I think he'll do a great job, so give him some support, ok? In no way was he to blame for any of the troubles.

> This would be a great way to say it in the IDL:
> > onDebuggerActivated is
> > called on the next event loop turn to let the debugger know that the it is safe
> > to run then. 

Good point. Rob, can you amend the comments?

> > Is that callback not happening, or did you not wire it up?
> 
> Yes the callback happens. 

Cool -- then if something later prevents a breakpoint from being set, that should be filed separately. Do any debugger features work after the callback fires?

> Thanks for your patience and explanation.

np.

/be
Comment 67 Andreas Gal :gal 2010-11-01 22:36:07 PDT
We have some pretty concrete plans for the shortly-after-FF4 time frame to implement JSD2. jimb already has a pretty solid design. I have argued that we can ship JSD2 with a point release since it won't regress anyone's experience (we could keep old jsd around for a little while to make sure we got everything right), so firebug won't be stuck with old-JSD. We just don't have the resources right now to do the work until FF4 is done.
Comment 68 John J. Barton 2010-11-01 22:43:52 PDT
(In reply to comment #66)
> (In reply to comment #65)
> > That is what I wanted someone to say.
> 
> Ok. I figured it out from the patch description and comments. It took a little
> leap of faith, but not much. YMMV.

I guess if you read Firebug code then YMMV as well ;-)

> 
> > Sorry, I have to say it does sound sarcastic. I was exaggerating to try to get
> > you to see things from my point of view.  I'm completely fed up with having to
> > reverse engineer to get things to work.  It's time to stop writing crap code
> > because we can't communicate about the system. 
> 
> I don't recall writing any crap code here. Let's try to stick to fixable
> problems instead of generalized negatives, while we are still here trying to do
> something better.

I should have written my last sentence: 

   I'm sick and tired of writing crap code 
   because we can't communicate about the system. 

I did not intend to make any comment about anyone else's code, sorry for miscommunicating about communication.

> 
> On the general rotting state of jsd, it's our fault for not putting someone on
> the case earlier, relying on the kindness of timeless. We aimed to fix that
> with jimb but he was pulled into ES5 work. He's coming back and I think he'll
> do a great job, so give him some support, ok? In no way was he to blame for any
> of the troubles.

I've had many good conversations with jimb and I look forward to the something good happening.

> > > Is that callback not happening, or did you not wire it up?
> > 
> > Yes the callback happens. 
> 
> Cool -- then if something later prevents a breakpoint from being set, that
> should be filed separately. Do any debugger features work after the callback
> fires?

jsd features yes, we get calls to onScriptCreated. However nothing of note happens in Firebug because the first thing we do in onScriptCreated is call setBreakpoint(0) and we never hit. That is the subject of Bug 608763
Comment 69 John J. Barton 2010-11-01 22:49:40 PDT
(In reply to comment #67)
> We have some pretty concrete plans for the shortly-after-FF4 time frame to
> implement JSD2.

Firebug 1.7 is solely dedicated to reimplementing our JS debug code to an API similar to what we expect in JSD2; that work is underway and we are aiming to beat FF4 to its finish line. (1.6 will support FF4 until 1.7.0).
Comment 70 Robert Sayre 2010-11-02 08:43:31 PDT
(In reply to comment #66)
> 
> > This would be a great way to say it in the IDL:
> > > onDebuggerActivated is
> > > called on the next event loop turn to let the debugger know that the it is safe
> > > to run then. 

I'll try to clarify the comment. However, phrasing it in terms of the event loop wouldn't be accurate, because I tried that and it didn't work. :)

The problem is that there are some edge cases like sync XHR and XBL loading that can spin the event loop with JS on the stack, so scheduling an event doesn't actually work. Instead, I had to set up a cheap check at the bottom of the XPConnect context stack.

So, how about "onDebuggerActivated will be called after the script which called asyncOn has exited."
Comment 71 John J. Barton 2010-11-02 09:00:26 PDT
(In reply to comment #70)
> The problem is that there are some edge cases like sync XHR and XBL loading
> that can spin the event loop with JS on the stack, so scheduling an event
> doesn't actually work. Instead, I had to set up a cheap check at the bottom of
> the XPConnect context stack.

What about nsIObserve handlers, specifically profile-after-change? That is where I call jsd.asyncOn for chromebug.  All I really need to know is what JS will be missed so I don't go looking for it.

> 
> So, how about "onDebuggerActivated will be called after the script which called
> asyncOn has exited."

Maybe

onDebuggerActivated will be called after the call stack which called asyncOn has exited and the XPConnect context stack has zero frames.
Comment 72 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-11-02 09:08:04 PDT
(In reply to comment #71)
> What about nsIObserve handlers, specifically profile-after-change? That is
> where I call jsd.asyncOn for chromebug.  All I really need to know is what JS
> will be missed so I don't go looking for it.

Do you mean "missed" in the sense that it has already executed in the past so you don't get events for it, or do you mean "missed" in the sense that it was *compiled* in the past?  In the latter case I think we should just be recompiling scripts that predate a debug-state-transition.

For the former case, honestly, I think we should probably just support an environment variable or command-line flag to start in debugMode for the chrome case.  Other embedders who bootstrap heavily on JS will want something like that too, but it's a separate bug.  Let's keep this bug focused on what we need for content-script debugging to be effective and robust.  (This is how node and v8 work together, for example.)

> onDebuggerActivated will be called after the call stack which called asyncOn
> has exited and the XPConnect context stack has zero frames.

I think that might be in the "true but useless" category, in that anyone who wants to make a decision based on the information needs to first map it to the "so what?" version: no script is running.
Comment 73 Robert Sayre 2010-11-02 09:27:44 PDT
(In reply to comment #72)
> (In reply to comment #71)
> > What about nsIObserve handlers, specifically profile-after-change? That is
> > where I call jsd.asyncOn for chromebug.  All I really need to know is what JS
> > will be missed so I don't go looking for it.
> 
> Do you mean "missed" in the sense that it has already executed in the past so
> you don't get events for it, or do you mean "missed" in the sense that it was
> *compiled* in the past?  In the latter case I think we should just be
> recompiling scripts that predate a debug-state-transition.

The patch does this--it recompiles all scripts in the runtime for debug mode, unless they are workers.

> 
> For the former case, honestly, I think we should probably just support an
> environment variable or command-line flag to start in debugMode for the chrome
> case.

If you leave Firebug's JS debugger enabled, and then restart the browser, you will enter debugMode almost immediately on startup. Additionally, there's a class called jsdASObserver at the bottom of jsd_xpc.cpp. It's an app-startup observer that turns on debug mode.
Comment 74 John J. Barton 2010-11-02 09:32:49 PDT
(In reply to comment #72)
> (In reply to comment #71)
> > What about nsIObserve handlers, specifically profile-after-change? That is
> > where I call jsd.asyncOn for chromebug.  All I really need to know is what JS
> > will be missed so I don't go looking for it.
> 
> Do you mean "missed" in the sense that it has already executed in the past so
> you don't get events for it, or do you mean "missed" in the sense that it was
> *compiled* in the past?  In the latter case I think we should just be
> recompiling scripts that predate a debug-state-transition.

Compiled: just what JS compilations will be missed. Recompile won't help because I need the call stack at the compile point to determine the kind of compilation unit. 

> 
> For the former case, honestly, I think we should probably just support an
> environment variable or command-line flag to start in debugMode for the chrome
> case.  Other embedders who bootstrap heavily on JS will want something like
> that too, but it's a separate bug.  Let's keep this bug focused on what we need
> for content-script debugging to be effective and robust.  

We need chromebug to debug Firebug. But I guess I don't really need an answer to the 'what's missing question', we can just muddle along. I've never known before.

(This is how node and
> v8 work together, for example.)

v8 wires a lot of stuff in to C++, making the barrier to innovation high.

> 
> > onDebuggerActivated will be called after the call stack which called asyncOn
> > has exited and the XPConnect context stack has zero frames.
> 
> I think that might be in the "true but useless" category, in that anyone who
> wants to make a decision based on the information needs to first map it to the
> "so what?" version: no script is running.

I was imagining that the XPConnect phrase would help remind us where Rob put the test. Anyway I think Rob should just pick something.
Comment 75 John J. Barton 2010-11-02 09:37:33 PDT
(In reply to comment #73)
> (In reply to comment #72)
> > (In reply to comment #71)
> > > What about nsIObserve handlers, specifically profile-after-change? That is
> > > where I call jsd.asyncOn for chromebug.  All I really need to know is what JS
> > > will be missed so I don't go looking for it.
> > 
> > Do you mean "missed" in the sense that it has already executed in the past so
> > you don't get events for it, or do you mean "missed" in the sense that it was
> > *compiled* in the past?  In the latter case I think we should just be
> > recompiling scripts that predate a debug-state-transition.
> 
> The patch does this--it recompiles all scripts in the runtime for debug mode,
> unless they are workers.

As I mentioned above, Firebug can't use these because we don't have a way to correct the misinformation jsd produces unless we see the call stack at compile time.

> 
> > 
> > For the former case, honestly, I think we should probably just support an
> > environment variable or command-line flag to start in debugMode for the chrome
> > case.
> 
> If you leave Firebug's JS debugger enabled, and then restart the browser, you

I guess you mean "exit with jsd.on === true"?

> will enter debugMode almost immediately on startup. Additionally, there's a
> class called jsdASObserver at the bottom of jsd_xpc.cpp. It's an app-startup
> observer that turns on debug mode.
Comment 76 Brendan Eich [:brendan] 2010-11-02 09:48:40 PDT
Sorry, sayrer -- my memory held only your first design, the event loop turn one. Now that you mention it, a nested event loop could be ignored, couldn't it? But perhaps the XPCThreadContextStack is better. Harder to describe in a comment!

/be
Comment 77 Robert Sayre 2010-11-02 10:25:23 PDT
(In reply to comment #75)
> 
> As I mentioned above, Firebug can't use these because we don't have a way to
> correct the misinformation jsd produces unless we see the call stack at compile
> time.

Is there a bug on the jsd misinformation in question? It would probably be better to fix it than hack around it forever. We should file a new bugs to fix stuff like that, not turn this one into a venue for griping about every JSD bug that currently exists.
Comment 78 John J. Barton 2010-11-02 11:14:42 PDT
Bug 449452 has a slightly out of date summary, some evalInSandbox issues are missing; Bug 449464 is the most closely related to the recompilation issue.
Comment 79 Andrew Drake [:adrake] 2010-12-13 15:27:39 PST
*** Bug 585233 has been marked as a duplicate of this bug. ***
Comment 80 John J. Barton 2011-01-18 15:18:56 PST
I am not able to get this feature to work correctly, see Bug 626830 jsd.asyncOn fails
Comment 81 :Gijs Kruitbosch 2011-02-13 06:07:10 PST
Is there any reason bz's suggestion from comment 37 was not taken onboard? This is making using this API harder. In general, it'd be nice to add "function" to all these JSD callbacks...

I'd appreciate if someone from the JS team can chime in - I'll be happy to file the bug myself, but I'd like to confirm that there are no obscure reasons why we can't do that. :-)
Comment 82 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-02-13 09:00:31 PST
There are no good reasons for it.  File a bug, please.
Comment 83 :Gijs Kruitbosch 2011-02-13 09:11:36 PST
(In reply to comment #82)
> There are no good reasons for it.  File a bug, please.

Thanks for the quick reply. Bug 633833 filed.

Note You need to log in before you can comment on or make changes to this bug.