595243 - Expose debugMode to JSD

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Robert Sayre]

+++ This bug was initially created as a clone of Bug #594054 +++

This will help Firebug not crash on JM.

Comment 1

[Robert Sayre]

Attachment: expose it

Comment 2

[Robert Sayre]

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]

Attachment: use public API everywhere

same patch, just using JS_* functions in both places.

Comment 4

[John J. Barton]

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] (always need-info? me)]

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]

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]]

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]

Firebug worked yesterday (and chromebug for the first time yay!). Today it crashes on bug 595743

Comment 9

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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] (always need-info? me)]

(In reply to comment #6)
> Is onScriptCreated called for scripts in a jsdIConext with debugMode===false?
Yes

Honza

Comment 11

[John J. Barton]

(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] (always need-info? me)]

> 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]

(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]

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] (always need-info? me)]

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]]

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] (always need-info? me)]

(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]]

OK, then it needs an ETA for completion.

Comment 19

[Robert Sayre]

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] (always need-info? me)]

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]]

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]

Attachment: 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]

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]

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]

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]

(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]]

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]

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]

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]

I'll have a new patch up for this shortly

Comment 31

[Tomas]

Any news?

Comment 32

[erpman1]

(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]

Attachment: revised patch

this seems to work, but the Firebug UI isn't fully ready for the asynchronous interface.

Comment 34

[Robert Sayre]

Attachment: 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]

(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]

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 [:bzbarsky]]

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]]

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]

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]

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)]

Andreas, have a patch yet?

Comment 42

[Damon Sicore (:damons)]

Sorry, hard to tell what's next on this bug.  Please advise.

Comment 43

[Andreas Gal :gal]

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)]

Can we land this please?

Comment 45

[Robert Sayre]

Attachment: rebased

Comment 46

[Andreas Gal :gal]

Comment on attachment 486965 [details] [diff] [review]
rebased

God is this ugly.

Comment 47

[John J. Barton]

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)]

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]]

I backed the changes here out of TM, because they were turning mochitest-plain-4 orange.

Comment 50

[Robert Sayre]

(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]

fixed up the test:

http://hg.mozilla.org/tracemonkey/rev/05e76ef439fd

Comment 52

[John J. Barton]

(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]

Attachment: only compile main thread scripts for debug mode, and only do it from the main thread

Comment 54

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

FTR:

Landed: <http://hg.mozilla.org/mozilla-central/rev/85feecf161fe> (comment 48)
Backed out: <http://hg.mozilla.org/mozilla-central/rev/a57aba6992e3> (comment 49)
Landed: <http://hg.mozilla.org/mozilla-central/rev/05e76ef439fd> (comment 51)
Landed followup: <http://hg.mozilla.org/mozilla-central/rev/7ab3445d162e>

Comment 55

[Andreas Gal :gal]

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] (always need-info? me)]

(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]

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]

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]

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] (always need-info? me)]

I have created Bug 608763 with STR and a test case for the setBreakpoint(0) problem.

Honza

Comment 61

[Robert Sayre]

(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]

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]

(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]]

(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]

(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]]

(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]

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]

(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]

(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]

(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]

(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)]

(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]

(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]

(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]

(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]]

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]

(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]

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 80

[John J. Barton]

I am not able to get this feature to work correctly, see Bug 626830 jsd.asyncOn fails

Comment 81

[:Gijs (he/him)]

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 [:bzbarsky]]

There are no good reasons for it.  File a bug, please.

Comment 83

[:Gijs (he/him)]

(In reply to comment #82)
> There are no good reasons for it.  File a bug, please.

Thanks for the quick reply. Bug 633833 filed.