956087 - Debugger statements should automatically fire up the debugger

Status: RESOLVED DUPLICATE of bug 1132501

(DevTools :: Debugger, defect, P3)

Description

[Brian Grinstead [:bgrins]]

STR:
* Open a page with a debugger statement, like: http://htmlpad.org/debugger/
* Open devtools using any panel besides the debugger (console, inspector, etc)
* Click the button on the page to trigger the breakpoint

Expected behavior:
DevTools switches to the debugger panel paused at the breakpoint, just like it would if you had opened the Debugger panel first.

Actual behavior:
Nothing happens.  The toolbox has not loaded the debugger tool yet, so it doesn't react to the statement.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

IIRC, we do this because just adding a |onDebuggerStatement| hook forces the whole page to go into debug mode, which has a not-insignificant performance impact.

Comment 2

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #1)
> IIRC, we do this because just adding a |onDebuggerStatement| hook forces the
> whole page to go into debug mode, which has a not-insignificant performance
> impact.

It seems that if you are opening up developer tools, it would be OK for things to slow down a bit (especially if it is going to happen once you open the debugger panel anyway).  Missed debugger statements can cause a lot of confusion, especially if it only happens some of the time.  That said, I don't know exactly how much it does slow things down.  Would it slow down the devtools startup time, or just the execution of scripts on the page?

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

I think Panos knows more about the process and thinking that led to this decision than I do.

Comment 4

[Dave Camp (:dcamp)]

TBH I don't know if much process went in to that decision, other than I think it just evolved that way.

Brian's right, the behavior is confusing.  Nick's right, debug mode for the whole page has some perf impact if all you want to do is tweak styles.

It's probably OK to slow down a bit while tweaking styles, but I don't think the same holds for profiling.

Comment 5

[Panos Astithas (he/him) [:past] (please ni?)]

Also note that Firebug thinks it's worthwhile to provide a separate UI toggle to enable the script panel, precisely to avoid the performance degradation from debug mode. I think our tradeoff (and UX) is better in this case (opening the debugger panel once enables debug mode), but I can't think of any solution that would satisfy everyone at the same time. In my experience regular web developers rarely know of or have used debugger statements in their code.

Comment 6

[Brandon Benvie [:benvie]]

I agree with what Panos said and think the current behavior is fine. Maybe we can use some UI to indicate whether the debugger is enabled? Like say the Debugger tab is grayed or has some indicator.

Comment 7

[(not reading bugmail) Nick Desaulniers [:\n]]

bump; at first I thought the debugger statement was broken for remote debugging.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Nick Desaulniers [:\n] from comment #7)
> bump; at first I thought the debugger statement was broken for remote
> debugging.

Nothing has changed since comment 5, and I don't foresee any changes to the existing behavior. Would be worth exploring the ideas raised in comment 6, though.

Comment 9

[Panos Astithas (he/him) [:past] (please ni?)]

I just had an idea, but I'm not sure how feasible it is.

Gecko has various code paths that call certain callbacks registered by Gecko or SpiderMonkey if present, or just continue if no callback is set. The slow script dialog uses such a callback (the interrupt callback), and my execution reasons patch adds another one. If SpiderMonkey could check for the presence of such a callback when encountering a debugger statement, we could easily adapt the slow script dialog mechanism to open the debugger frontend and pause execution. Desktop Firefox will register the callback when initializing gDevTools (i.e. on browser startup, so pretty much always), and other embeddings (like Firefox OS and Firefox for Android) will set the callback when a remote debugger is connected and reset it after the connection is terminated.

I'm not sure if that is feasible though, given the various compiler and interpreter backends involved here and the performance implications of an extra check. In debug builds we dump a stack trace in the terminal, so it seems like the idea is not entirely crazy.

Paging the experts for advice.

Comment 10

[Jason Orendorff [:jorendorff]]

Doing an extra check each time a debugger statement runs is fine. No performance concerns there.

Forwarding the question to shu. If we can turn on "debug mode" while there's code on the stack, then this should be very easy. The main issue is API design: it's as though we want a mostly-disabled Debugger to still be able to receive onDebuggerStatement callbacks. Or something.

Comment 11

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> Doing an extra check each time a debugger statement runs is fine. No
> performance concerns there.
> 
> Forwarding the question to shu. If we can turn on "debug mode" while there's
> code on the stack, then this should be very easy. The main issue is API
> design: it's as though we want a mostly-disabled Debugger to still be able
> to receive onDebuggerStatement callbacks. Or something.

Yeah, I think having |debugger;| hook into the XPC service that the slow script button uses will work fine for this.

As an aside, the presence of |debugger;| affects frontend optimization, like marking all variables as aliased. It possibly disables the entire scope coord optimization, I don't remember. So the very presence of |debugger;| slows scripts down even with debug mode off, but nowhere near as much.

Comment 12

[Shu-yu Guo [:shu]]

I'd be happy to mentor whoever wants to work on this, I won't be able to get to it for a bit.

One thing I don't know how to work around off the top of my head is that the actual checking and calling for the XPC service callback would need QI and all that chrome jazz. So we can't check for this directly in the JS engine.

We could introduce a new callback in addition to the interrupt callback. We could also appropriate the interrupt callback here and make |debugger;| behave like an interrupt point. Jason, any opinions?

Comment 13

[Shu-yu Guo [:shu]]

To wit, the way the debugger frontend hooks into the JS engine is:

[Chrome JS]    gDevTools sets a callback on
[XPC Service]  SlowScriptDebug, which is checked by
[DOM]          GlobalWindow's slow script dialog code, which is called by
[JS Engine]    JS engine's interrupt callback

For the |debugger;| use case here, what's missing are the [DOM] and [JS Engine] steps above. The browser needs to be able to register a C++ callback with the engine to be called on |debugger;| statements. After that we can reuse the existing SlowScriptDebug machinery (with some renaming).

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to Shu-yu Guo [:shu] from comment #12)
> I'd be happy to mentor whoever wants to work on this, I won't be able to get
> to it for a bit.
> 
> One thing I don't know how to work around off the top of my head is that the
> actual checking and calling for the XPC service callback would need QI and
> all that chrome jazz. So we can't check for this directly in the JS engine.
> 
> We could introduce a new callback in addition to the interrupt callback. We
> could also appropriate the interrupt callback here and make |debugger;|
> behave like an interrupt point. Jason, any opinions?

I'd prefer for the Debugger object to be the access point for whatever we do here.

We already have an onDebuggerStatement hook; maybe we just change the rules for that. But I defer to jimb for Debugger API questions.

Comment 15

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #14)
> (In reply to Shu-yu Guo [:shu] from comment #12)
> > I'd be happy to mentor whoever wants to work on this, I won't be able to get
> > to it for a bit.
> > 
> > One thing I don't know how to work around off the top of my head is that the
> > actual checking and calling for the XPC service callback would need QI and
> > all that chrome jazz. So we can't check for this directly in the JS engine.
> > 
> > We could introduce a new callback in addition to the interrupt callback. We
> > could also appropriate the interrupt callback here and make |debugger;|
> > behave like an interrupt point. Jason, any opinions?
> 
> I'd prefer for the Debugger object to be the access point for whatever we do
> here.
> 
> We already have an onDebuggerStatement hook; maybe we just change the rules
> for that. But I defer to jimb for Debugger API questions.

I have some half-baked plans for "Debugger-go-faster" patches that selectively disable JITs depending on what hooks are set. What you're proposing sounds pretty similar. Even if a compartment is in debug mode, having onDebuggerStatement as the only hook set shouldn't disable Ion.

Comment 16

[Jim Blandy :jimb]

Shu's general plan, in which a Debugger with nothing but an onDebuggerStatement hook doesn't cause any deoptimization, is obviously the ideal approach. But he's busy for the moment, so we have to think through the other approaches.

We want 'debugger;' statements to be no-ops in tabs that don't have the devtools are open. That means that the debugger statement hooks need to be localized to those tabs that do, in some fashion or another. 

If Debugger is to be the access point, but we can't make these tabs debuggees, because that would be slow... then we've got to give Debugger *two* sets of globals, one of which only permits onDebuggerStatement hooks? What a kludge!

However, SpiderMonkey is already providing all the public API we need for this. Even my patch in bug 1031881 leaves JSDebugHooks::debuggerHandler in place, because it's used by XPCOM, outside JSD. 

https://hg.mozilla.org/mozilla-central/file/f93c0ef45597/js/xpconnect/src/XPCJSRuntime.cpp#l3281

That's runtime-wide, but we can filter hits in JS. Firefox just needs a way to wrest control of that hook away from xpc_DebuggerKeywordHandler.

Comment 17

[Shu-yu Guo [:shu]]

Maybe Nicolas will make speedy progress on bug 1032869? :)

Comment 18

[Brian Grinstead [:bgrins]]

Sounds like it may become possible to listen for the debugger statement without major performance issues, so we could then fix this as expected in the original description (allow a debugger statement to work even without having opened the debugger).

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: debugger-open-WIP.patch

WIP.  Nick, as discussed, this simply opens the full panel but just doesn't focus it on toolbox startup.  I added a comment in panel.js at a place where we could maybe detect a 'lightweight' mode and do something different than fully opening up the view and/or setting all the options with the debugger server.  Any ideas how this might work?

You can test this out by opening this page: http://bgrins.github.io/devtools-demos/debugger/debugger.html and opening the toolbox to the webconsole or something then pressing the button.  The debugger panel should automatically focus and work as expected when the debugger; statement is triggered.

Comment 20

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8526959 [details] [diff] [review]
debugger-open-WIP.patch

Review of attachment 8526959 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/framework/toolbox.js
@@ +318,5 @@
>            }
>  
> +          // Load the debugger so that we can detect a debugger; statement
> +          // even if the user hasn't clicked on the tab yet.
> +          let debuggerPromise = this.loadTool("jsdebugger");

Do we really want to block the toolbox being ready on opening the debugger in the background?

Comment 21

[Brian Grinstead [:bgrins]]

Unfortunately this blows up the tests: https://tbpl.mozilla.org/?tree=Try&rev=ef5cb3088e18.  I haven't had a chance to take a closer look at the errors.  Although I do wonder if we will need the new getPanelWhenReady API from Bug 1093730 to better handle situations where the debugger panel is already opened but the test is assuming that it hasn't yet when switching to that tab.

Comment 22

[Eddy Bruel [:ejpbruel]]

Brian, are you still actively working on this? The bug isn't currently assigned to you.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Eddy Bruel [:ejpbruel] from comment #22)
> Brian, are you still actively working on this? The bug isn't currently
> assigned to you.

No, not right now.  It's on my wishlist though.

Comment 26

[James Long (:jlongster)]

This is going to happen automatically once bug 1132501 lands