717749 - Slow script dialog should offer a Debug button if debugger is installed

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

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

This used to happen. Now it doesn't.

Comment 1

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

Rob, did you mean to increase or decrease the priority of this? I'm getting mixed signals from the changes in Priority and Target Milestone.

Comment 3

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

Jim made an insightful comment in bug 891134:

We should be able to put a button on the slow script dialog that says "Debug This Page" that pops up the debugger showing the running code. However, bug 716647 makes this impossible.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

I want this!

Comment 5

[:Gijs (he/him)]

http://mxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindow.cpp#10482 is the code that used to do this. I suspect at the very least its detection code will need updating...

Comment 6

[Michael Bebenita [:mbx]]

I really want this as well. It's currently almost impossible to debug / find infinite loops without this feature. Having an option "break on slow script warnings" would be sufficient.

Comment 7

[Jim Blandy :jimb]

Yeah, this is a super-big deal. Unfortunately, the interactions between the Debugger and the JITs make it difficult. Shu is working on it, in the bug marked as a blocker. That bug also blocks a bunch of other really fun stuff, so I hope it gets traction.

Comment 8

[Samuel Bronson]

... which debugger would the button start?  It would be nice if it were possible to invoke Firebug, too, not only the seemingly nameless debugger.

Comment 9

[Jim Blandy :jimb]

(In reply to Samuel Bronson from comment #8)
> ... which debugger would the button start?  It would be nice if it were
> possible to invoke Firebug, too, not only the seemingly nameless debugger.

It seems like Firebug could simply replace the default handler for that button with its own, so this should be no problem. If that's not a good idea for some reason, I'd be happy to work with the Firebug folks on an appropriate hook.

Comment 10

[Jim Blandy :jimb]

I see now that this is handled by custom C++ code in nsGlobalWindow::ShowSlowScriptDialog, not by an event handler on a nice XUL document or something.

It seems like the best approach here would be to create an XPCOM service that maintains a handler hook, which can be null. Then, ShowSlowScriptDialog can find that service, ask it if a hook has been registered, and invoke it --- all with a nice C++ API.

The hook's interface type would have the [function] attribute, so the devtools or Firebug could simply drop in their own JS function in as the handler.

Comment 11

[Shu-yu Guo [:shu]]

I have some WIP patches at https://github.com/syg/gecko-dev/tree/debug-mode-osr which tries to add this functionality back in. That branch includes all the patches from bug 716647.

The problem I have now is that the all the devtools APIs seem async, and pausing a slow script is a synchronous thing. Here's what I have in mind the "onclick" of the "debug script" button on the slow script dialog should do:

  1) Open the debugger tab if not already open.
  2) Synchronously set a one-time breakpoint on the immediate next JS bytecode.
  3) Resume execution of the slow script, which should break immediately.

The current state is that with the help of Victor and Nick I have 1) figured out. If I call |interrupt| on the thread client of the debugger, it even adds a little pause tick to the right line of the code. The problem is that the slow script just *keeps going* (and keeps spawning more slow script dialogues). I'm not sure how the debugger client currently achieves pausing, but calling |interrupt| doesn't actually pause the script execution.

Setting NI for Nick and Jim for ideas.

Comment 12

[Shu-yu Guo [:shu]]

I should note that 1) and 2) above need to be synchronous. We shouldn't return from the slow script dialogue until we're sure we will pause immediately after returning.

Comment 13

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

We use a horrible hack with nested event loops to achieve synchronicity where it would otherwise be impossible on the server[0]. I hesitate to suggest the same technique, but if that is the only option...

Regarding interrupt not causing a pause: did you check the RDP response? Was it reporting everything as fine or did it have an error? To enable extensive RDP debugging, see[1].

Back to the sync setting of breakpoints: come to think of it, the slow script dialog is happening on the server (the debugee) so you shouldn't need to actually use the RDP for setting your BP. You should be able to just ensure that the |DebuggerServer| is up and running and then somehow grab/instantiate the right |ThreadActor| and call its |setBreakpoint| method. Problem is, AFAIK, we don't have a way for a server to tell the (not yet existing/connected) client to open the panel and start debugging. Obviously this wouldn't work at all if your b2g phone isn't already connected to a desktop machine, so I guess this feature is only for when we are already connected and when the dbg server and client are on the same machine.

This might be a bit more work than previously expected...

[0] http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/script.js#1154
[1] https://wiki.mozilla.org/DevTools/Hacking#Enabling_DevTools_Logging

Comment 14

[Shu-yu Guo [:shu]]

Attachment: WIP: Make the button pop up the debugger and pause the debuggee

Asking for f? from smaug for the adding the SlowScriptDebugButton XPCOM service to register an onClick callback.

Asking for f? from past for the devtools parts.

Here's what this patch does:

 - Add an XPCOM service to attach an onClick handler the slow script dialog's "Debug Script" button. If set, the callback gets called when the "Debug Script" button is pressed, and takes priority over the existing JSD1 hook code. (We should be removing JSD1 soon.)

 - Install a handler which pops up the debugger. This is tricky, as we are doing this from inside the interrupt handler from JS. To make things synchronous, I enter modal state and spin the event loop until the Debugger panel is all set up. A special one-time onStep and onPop are installed on the topmost frame to pause, so that upon returning from the interrupt handler, we immediately pause in the debugger.

All of this depends on the patches in bug 716647 to be applied.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8404388 [details] [diff] [review]
WIP: Make the button pop up the debugger and pause the debuggee


>+[scriptable, uuid(f75d4164-3aa7-4395-ba44-a5f95b2e8427)]
>+interface nsISlowScriptDebugButton : nsISupports
>+{
>+  attribute nsISlowScriptDebugCallback onClick;
>+};


Naming is odd here. Maybe
s/nsISlowScriptDebugButton/nsISlowScriptDebug/
and s/onClick/activationHandler/

Comment 16

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

Comment on attachment 8404388 [details] [diff] [review]
WIP: Make the button pop up the debugger and pause the debuggee

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

Does this work with e10s? I haven't tried it as the dependencies need rebasing, but perhaps you have?

The protocol part is similar to my break-on-next patch in bug 789430, but the use case is slightly different as we will definitely have frames on the stack in this case.

::: browser/devtools/framework/gDevTools.jsm
@@ +962,5 @@
>    gDevToolsBrowser._addToolToWindows(toolDefinition);
> +
> +  if (toolId === "jsdebugger") {
> +    gDevToolsBrowser.setSlowScriptDebugButtonHandler();
> +  }

We do tool customization in _addToolToWindows, so better move this there as well.

@@ +973,5 @@
>    gDevToolsBrowser._removeToolFromWindows(toolId);
> +
> +  if (toolId === "jsdebugger") {
> +    gDevToolsBrowser.unsetSlowScriptDebugButtonHandler();
> +  }

Same here, _removeToolFromWindows is a better fit.

::: toolkit/devtools/client/dbg-client.jsm
@@ +1250,5 @@
> +   *
> +   * @param function aOnResponse
> +   *        Called with the response packet.
> +   */
> +  breakInPlace: function (aOnResponse) {

Does breakImmediately or breakOnNext sound any better?

Comment 17

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

Comment on attachment 8404388 [details] [diff] [review]
WIP: Make the button pop up the debugger and pause the debuggee

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

Oops.

Comment 18

[Jim Blandy :jimb]

Clearing needinfo, as it seems like Shu's questions have been answered.

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 1: Add a slow script debug handler service

Renamed stuff per feedback comment.

Comment 20

[Shu-yu Guo [:shu]]

Attachment: slow script test page

You can use this to test the dialog button.

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 2: Hook up the debugger to the slow script debug service

Panos, you can use the attached test page to test the button. There's a UI
quirk I don't know how to fix, but hopefully you do. Asking you for f? since
I'd like the UI quirk to be fixed first.

When the script is being paused via the "Debug script" button, the little arrow
signaling what line is currently executing moves once before pausing. This is
easiest to see if the debugger panel is already open when the slow script
dialog pops up.

That is, in the example page, when the panel is first brought up:

     ...
     if ((i % 1000000) === 0) dump(i + "\n");
==>  i++;
     ...

And when it's finally paused:

     ...
==>  if ((i % 1000000) === 0) dump(i + "\n");
     i++;
     ...

How do I make it so that the arrow doesn't jump?

Comment 22

[Shu-yu Guo [:shu]]

Well I guess it's pretty obvious why the arrow is moving. To pause, we have to actually resume the debuggee code and break again after it takes a step, since we cannot do a real pause inside the interrupt handler.

So now the question is: how do I suppress displaying the little green arrow until the debugger is fully paused, in this special case?

Comment 23

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

Comment on attachment 8416831 [details] [diff] [review]
Part 2: Hook up the debugger to the slow script debug service

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

What I see is not actually a jump, but a flicker, as both pauses occur on the same line for me. It's not too jarring, but I think it's easy to fix.

The debugger frontend currently considers all pauses worthy of updating its state, but clearly a pause caused by an interrupt packet is not interesting to a developer. Interrupt packets are an implementation detail for sending some pause-only requests when the debuggee is running. Therefore we should make the frontend ignore state changes completely in those cases. The debugger client will remain the only entity that will handle them and eventually return with the expected result or produce the final interesting state change.

The frontend adds 2 listeners for "paused" packets in debugger-controller.js: ThreadState._update and StackFrames._onPaused. They should return early if packet.why.type == "interrupted".

The only case where the frontend doesn't send an additional packet after an "interrupt" request is when clicking on the pause button to simply cease execution immediately. This case is rarely useful as it is, because by definition when that request happens the debuggee has returned to the event loop without any useful stack to inspect. This is why we plan to change the behavior of this button in bug 789430 to use something like the breakOnNext() request introduced here. The good news is that since there are no frames or variables to display, ignoring the "paused" packet in this case is fine. The complication is that we should at least update the pause/resume button's state, so that the user receives the expected feedback. That means that ThreadState._update should always toggle the button state (until bug 789430 is fixed), but probably not emit the "thread-paused" event in that case.

Comment 24

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #23)
> Comment on attachment 8416831 [details] [diff] [review]

FWIW, Victor, James and Nick, all agreed (on IRC) on removing the flickering of buttons for interrupt packet while James was working on the patch for 812172. Although it did not happen.

Comment 25

[Shu-yu Guo [:shu]]

Attachment: Part 2: Hook up the debugger to the slow script debug service.

Panos, thanks for the detailed steps for what I needed to change!

Comment 26

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

Comment on attachment 8417761 [details] [diff] [review]
Part 2: Hook up the debugger to the slow script debug service.

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

::: browser/devtools/debugger/debugger-controller.js
@@ +461,5 @@
>     * Update the UI after a thread state change.
>     */
>    _update: function(aEvent) {
> +    // Ignore "interrupted" events, which are generated by the slow script
> +    // dialog, to avoid UI flicker.

Besides the slow script dialog, "interrupted" events can be generated by the debugger client, e.g. while setting a breakpoint, setting the pause-on-exceptions flag, etc. Perhaps the comment should include those "internal" cases as well.

::: browser/devtools/framework/gDevTools.jsm
@@ +595,5 @@
> +
> +        // Break in place, which means resuming the debuggee thread and pausing
> +        // right before the next step happens.
> +        switch (threadClient.state) {
> +        case "paused":

Style nit: we indent case clauses as well.

Comment 27

[Shu-yu Guo [:shu]]

So my patch definitely does not work in e10s. I get an error that says:

------------------------------------------------------------------------
Hit JavaScript "debugger" keyword. JS call stack...
0 <TOP LEVEL> ["file:///home/shu/slowscript.html":6]
    this = [object Window]

------------------------------------------------------------------------

There is no |debugger| keyword, btw. Is our debugger supposed to work in e10s at all?

Comment 28

[Shu-yu Guo [:shu]]

That message is printed by a JSD1 hook that XPCJSRuntime installs. Why is XPCJSRuntime using JSD1?!

Comment 29

[Shu-yu Guo [:shu]]

While testing this patch I noticed the following behavior. When we close a tab that's paused in the debugger, the debuggee script runs to completion. I suppose this isn't so much of a problem for terminating scripts, but for an infinite loop script, this gets into a very strange situation where we try to pop up the slow script dialog for a window that no longer exists.

So it turns out on tab closure (not panel!), we need to force termination of the debuggee script. Marking dependency on implementation of D.F.pop.

Implementation ideas over in bug 736733.

Comment 30

[Shu-yu Guo [:shu]]

Oops, accidentally removed dependency.

Comment 31

[Shu-yu Guo [:shu]]

Attachment: Hack to distinguish tab closures

So I came up with a much, much easier solution to the problem I described in
comment 29. Simply return |null| from the onStep/onPop handlers when the tab is
closed.

Panos, could you tell me how to pass the tab closed event to the script actor?
This patch is a pretty hacky proof of concept. I got pretty lost trying to
trace all the actors and transports and connections. :)

Comment 32

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

Comment on attachment 8420760 [details] [diff] [review]
Hack to distinguish tab closures

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

In the desktop browser case, the parent of the ThreadActor is the BrowserTabActor, which already listens for TabClose events:

http://dxr.mozilla.org/mozilla-central/source/toolkit/devtools/server/actors/webbrowser.js#399

This handler basically ends up calling browserTabActor.exit(), which in turn calls threadACtor.exit(), which will eventually resume execution. BrowserTabActor.prototype.threadActor is a reference to the child actor. It seems to me that BrowserTabActor could set a flag on ThreadActor for onStep/onPop to inspect when returning, which would be all you need here.

Comment 33

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The debugger is supposed to work in e10s. If you need help debugging it, please let me know.

Comment 34

[Shu-yu Guo [:shu]]

Bill and I talked about the e10s problem on IRC. Summary is that this will need to be rewritten for e10s support, so I'm going to push the current implementation for now. This only means that this functionality won't be available in e10s -- there shouldn't be any regressions for e10s.

Comment 35

[Shu-yu Guo [:shu]]

Attachment: Part 3: Terminate instead of resume debuggee script on tab closure.

Comment 36

[Shu-yu Guo [:shu]]

Attachment: Part 3: Terminate instead of resume debuggee script on tab closure.

Oops, forgot to add tests.

Comment 37

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea744b43a24
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae9ea46cbe26
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/94e7e63ae385

Comment 38

[[e-mail]]

Comment on attachment 8417761 [details] [diff] [review]
Part 2: Hook up the debugger to the slow script debug service.

>+      let target = devtools.TargetFactory.forTab(chromeWindow.gBrowser.selectedTab);
What happens if you get a slow script dialog for chrome code? (Admittedly rare, but I've had it happen in unoptimised or debug builds.)

>+    debugService.activationHandler = undefined;
[XPConnect will change this to null.]

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ea744b43a24
https://hg.mozilla.org/mozilla-central/rev/ae9ea46cbe26
https://hg.mozilla.org/mozilla-central/rev/94e7e63ae385

Comment 40

[Shu-yu Guo [:shu]]

(In reply to [e-mail] from comment #38)
> Comment on attachment 8417761 [details] [diff] [review]
> Part 2: Hook up the debugger to the slow script debug service.
> 
> >+      let target = devtools.TargetFactory.forTab(chromeWindow.gBrowser.selectedTab);
> What happens if you get a slow script dialog for chrome code? (Admittedly
> rare, but I've had it happen in unoptimised or debug builds.)
> 

Good question. How do I find out if the nsGlobalWindow that triggered the slow script is already a chrome window?

Comment 41

[[e-mail]]

Well, aWindow.top should either be equal to chromeWindow.content (if aWindow.top is the current tab) or chromeWindow (if aWindow is not a tab). I'm not sure what you would do about background tabs, but given that you've just clicked on Debug I assume that's not relevant here.

Comment 42

[[e-mail]]

Does this only works for in-page debugging rather than remote debugging?

Comment 43

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to [e-mail] from comment #42)
> Does this only works for in-page debugging rather than remote debugging?

It doesn't work in e10s currently, and I'd guess the remote case has similar issues to that path (see bug 1034213).

Comment 44

[Adam Moore]

It isn't clear to me looking at the patch...  How do I remove/disable this debug button from this dialog from within an extension?

Comment 45

[Adam Moore]

(In reply to Adam Moore from comment #44)
> It isn't clear to me looking at the patch...  How do I remove/disable this
> debug button from this dialog from within an extension?

Nevermind, it isn't pretty, but I found a way.