1354679 - Automatically display the PausedDebuggerOverlay when the debugger is paused

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

The goal is to use the new PausedDebuggerOverlay added in bug 1338582 so it will be shown as soon as script execution is paused for any reason, and hidden otherwise.

I have started to look into this, and my current approach is to have the ThreadActor do this.

ThreadActor._pauseAndRespond gets executed when we pause, so we can show it there.
ThreadActor.onResume gets executed when we resume, so we can hide it there.

There are multiple reasons that can cause script execution to pause:

debuggerStatement: we hit a `debugger;` statement in the code
breakpoint: we're paused at a breakpoint
exception: the "pause on exceptions" setting is ON and an exception was thrown
pauseOnDOMEvents: the "pause on DOM events" setting is ON and a DOM event occurred. Although I'm not sure if this actually exists in the new debugger
breakpointConditionThrown: a conditional breakpoint exists but the condition code actually threw an exception
resumeLimit: I have no idea what this one is, but it is part of the list of potential reasons.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: Bug 1354679 - Show the paused debugger overlay when script gets paused;

A previous bug had introduced a new overlay (highlighter) that was aimed
at being shown when the debugger would pause.
This commit adds the necessary code to the ThreadActor for the overlay
to be shown when script execution is paused, and hidden when script
execution resumes.
This commit also adds a test to cover this feature.

Review commit: https://reviewboard.mozilla.org/r/132866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132866/

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #0)
Some clarifications about a couple of paused reasons:
> pauseOnDOMEvents: the "pause on DOM events" setting is ON and a DOM event
> occurred. Although I'm not sure if this actually exists in the new debugger
The old debugger has a panel for this. The new debugger has a WIP for this too, but it's not yet been released.
> resumeLimit: I have no idea what this one is, but it is part of the list of
> potential reasons.
This happens when stepping through the code.

I tested the patch with all of the existing paused reasons, and verified that the overlay was shown every time.

Comment 3

[Patrick Brosset <:pbro>]

Green try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=313d270c1a18e5977c5e72a076895fde89822156&group_state=expanded

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8860863 [details]
Bug 1354679 - Show the paused debugger overlay when script gets paused;

https://reviewboard.mozilla.org/r/132866/#review135748

Note that it doesn't work against: `data:text/html,<script>debugger;</script>`

I'm concerned about increasing the complexity of this already hairy code.
Pause overlay have something to do with thread-actor, but what do you think about that:
Make ThreadActor be an EventEmitter, dispatch new events like "paused" or "pausing" based on when you dispatch it. Same for "resumed" or "resuming". And then have the overlay code contained in one function listening for these events, you may even be able to inline some of the existings.

Thanks for test, it is great to see this being tested!

::: devtools/server/actors/highlighters/paused-debugger.js:100
(Diff revision 1)
> +   * - {String} reason The text that will be shown in the toolbar. If not given, the
> +   *   toolbar will not be displayed.
> +   * - {Boolean} onlyToolbar Set to true to only show the toolbar, and not the overlay.
> +   * @return {Boolean}
> +   */
> +  showOverlay(options = {}) {

It looks like you don't use this function anywhere, so I miss the value of creating it? And even if you end up using it from script.js, it has a very low value.

::: devtools/server/actors/script.js:742
(Diff revision 1)
>                message: error.message + "\n" + error.stack
>              };
>            })
>            .then(packet => {
>              this.conn.send(packet);
> +            showPausedStateOverlay(this._parent);

Any particular reason to show it right after send()?
You are calling it from both pauseAndRespond and onExceptionUnwin. Both these methods are calling _paused(), so I'm wondering if we could get it called from _paused instead?

::: devtools/server/actors/script.js:1018
(Diff revision 1)
>      } else {
>        this._clearSteppingHooks(this.youngestFrame);
>        resumeLimitHandled = resolve(true);
>      }
>  
> +    hidePausedStateOverlay(this._parent);

Any particular reason to do it before than after the event loop release? (this._popThreadPause -> eventLoop.resolve())

I would rather do that after, when we dispatch devtools-thread-resumed.

::: devtools/server/actors/script.js:2406
(Diff revision 1)
> +  const principal = tabActor.window.document.nodePrincipal;
> +  if (Services.scriptSecurityManager.isSystemPrincipal(principal)) {
> +    return null;
> +  }
> +
> +  require("devtools/server/actors/inspector");

It would be helpful to comment why you have to instanciate the inspector actor here.

::: devtools/server/actors/script.js:2430
(Diff revision 1)
> +function showPausedStateOverlay(tabActor) {
> +  let overlay = getPausedStateOverlay(tabActor);
> +  if (overlay) {
> +    // TODO: for now, the same reason is always shown. In the future, we would like to
> +    // show the same reason that is also shown inside the debugger panel UI (using the
> +    // code in aReason.type). See the following issue for why we didn't do this already:

What is aReason.type? It doesn't make sense to me from script.js perspective.

::: devtools/server/tests/unit/test_pausedOverlay.js:32
(Diff revision 1)
> +      isOverlayVisible = false;
> +    }
> +  }
> +};
> +
> +function run_test() {

You can use add_task, like in mochitests:
http://searchfox.org/mozilla-central/source/browser/components/migration/tests/unit/test_Chrome_bookmarks.js#5

Comment 5

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Show the paused debugger overlay when script gets paused

Review commit: https://reviewboard.mozilla.org/r/190554/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190554/

Comment 6

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Show the paused debugger overlay when script gets paused;

A previous bug had introduced a new overlay (highlighter) that was aimed
at being shown when the debugger would pause.
This commit adds the necessary code to the ThreadActor for the overlay
to be shown when script execution is paused, and hidden when script
execution resumes.
This commit also adds a test to cover this feature.

Review commit: https://reviewboard.mozilla.org/r/190556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190556/

Comment 7

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Show the paused debugger overlay when script gets paused;

Review commit: https://reviewboard.mozilla.org/r/190558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/190558/

Comment 8

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8919660 [details]
Bug 1354679 - Show the paused debugger overlay when script gets paused

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190554/diff/1-2/

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8919660 [details]
Bug 1354679 - Show the paused debugger overlay when script gets paused

I'm not convinced that the client should tell when the debugger paused. This actor is fully in control of that. And it looks like it is only because we don't fully understand it that we do that.

My suggestion to control pause overlay from "_paused" was just wrong.
I thought that this method was actually pausing.
But it is not!

Its main goal seems to be the creation of a "paused" packet.
Then there is a couple of things being done:
* Clear onEnterFrame, onExceptionUnwind, onStep, onPop
* Clear DOM event breakpoints
* Create the PausedActor (which seem to be unused)
* Create the _pausePool (into which we add all the "grips" of objects being inspected by the debugger when being paused, like scope variable, return values, exception values, ...)

But nothing seem to actually pause anything. It cleans up state for getting into pause mode, but not actually pausing.

I think this method would benefit from some doc/comments and may be a better name?
And also, we should strip all what is unused! (poppedFrames is only used in test, PauseActor seems to not be used anywhere, ...)

Comment 10

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Use highlighter actor from client instead of from the server

Review commit: https://reviewboard.mozilla.org/r/191514/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191514/

Comment 11

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Show the paused debugger overlay when script gets paused;

A previous bug had introduced a new overlay (highlighter) that was aimed
at being shown when the debugger would pause.
This commit adds the necessary code to the ThreadActor for the overlay
to be shown when script execution is paused, and hidden when script
execution resumes.
This commit also adds a test to cover this feature.

Review commit: https://reviewboard.mozilla.org/r/191516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191516/

Comment 12

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Show the paused debugger overlay when script gets paused;

Review commit: https://reviewboard.mozilla.org/r/191518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/191518/

Comment 13

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8919660 [details]
Bug 1354679 - Show the paused debugger overlay when script gets paused

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190554/diff/2-3/

Comment 14

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8919660 [details]
Bug 1354679 - Show the paused debugger overlay when script gets paused

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/190554/diff/3-4/

Comment 15

[Yulia Startsev [:yulia] OOO until July 2026]

Ok, so far here is what I have gathered from conversation with various parties:

Just to be completely clear, i am using the following words to refer to different things: client = the client in MC, frontend = debugger-html on github.

1) the debugger server pauses when it needs to make any changes. That means that the _paused packet is sent, for example, when a breakpoint is added or a setting changes. 
2) The locations where patrick added his original interventions were well placed, however it means that the thread actor has too much responsibility. Its now no longer only taking care of the thread, but also responsible for other things: including showing / hiding the overlay. Using the event emitter is a nicer pattern, though perhaps I didnt do it in the best way
3) after talking with Jim Blandy, I realized that the client needed to inform the server of user significant pauses. This would be cleaner and require less intervention inside of the threadActor. An extra bonus here was that we could directly reuse the translations from the debugger client, making it much easier to implement the next step: showing the pause reason. I initially implemented this in the debugger frontend, making the message in the packet useful. Jason asked me to move it back to MC, he said that he would rather duplicate the logic. I prefer it in the frontend, because then we do not have to duplicate the code on the server and the frontend, but I am flexible with this.

Im writing this summary because I think I am the only one who knows everthing that has been going on.

My suggestiong is that the best solution is the keep the threadActor and the pauseOverlay separate. threadActor doesn't care about the pauseOverlay and the PauseOverlay only cares if it is shown.

My recommendation: remove everything inside of threadActor that triggers the pause overlay. then: either listen to the packets as they are being emitted, and respond in a similar way as the highlight tool, or just have the debugger client take care of triggering it. this reflects what we are doing with the inspector (ruler etc) and the architecture that chrome uses as well (i dont have a sample for that but I can find one)

A better suggestion (coming from alex) "get a front for the pause debugger overlay via: let overlay = await toolbox.highlighterUtils.getHighlighterByType("PausedDebuggerOverlay");"

I am attaching all of you so we can discuss it here centrally :) I am new to this code, so if I got something wrong feel free to correct me. 

pinging :ochameau, :jimb, :pbro and :jlast

Comment 16

[Patrick Brosset <:pbro>]

At a high level, I agree that the thread actor should not have to deal with the overlay. This is purely a user-visible thing that the UI should be in charge of.
It's true that in other panels such as the inspector, the front-end is the one doing the toggling of the various highlighters.

Using the getHighlighterByType API is the right way to instantiate the overlay and get a front for it.

We just need to find the right place in the code where these are true:
- we know whenever script execution is halted, whatever the reason may be,
- we know the actual reason,
- we have access to the same translated string than the debugger frontend for that reason.

My main motivation when I wrote the first patch was to get something going, and the server was the only place I knew of that satisfied the first condition. I was able to hook some code in like 2 or 3 places to know for sure when script execution was paused.
If we can do this client-side, then better!

Comment 17

[Alexandre Poirot [:ochameau]]

(In reply to Yulia Startsev [:yulia] from comment #15)
> Ok, so far here is what I have gathered from conversation with various
> parties:
> 
> Just to be completely clear, i am using the following words to refer to
> different things: client = the client in MC, frontend = debugger-html on
> github.
> 
> 1) the debugger server pauses when it needs to make any changes. That means
> that the _paused packet is sent, for example, when a breakpoint is added or
> a setting changes. 

This is unclear to me. Yes, `ThreadActor._paused` in called in many cases,
but it is not pausing anything. It mostly creates a "paused" packet.
I'm convinced that it would be clearer if we had a better comprehension of script.js
and figure out where it really pauses execution.

> 2) The locations where patrick added his original interventions were well
> placed, however it means that the thread actor has too much responsibility.
> Its now no longer only taking care of the thread, but also responsible for
> other things: including showing / hiding the overlay. Using the event
> emitter is a nicer pattern, though perhaps I didnt do it in the best way

My suggestion with paused/resumed events was to uncouple pause overlay from ThreadActor.
Today script.js contains various actors/classes, ThreadActor could be moved to its own module
and script.js be a more global script. The main issue relates to my previous comment,
it isn't obvious by just reading at the code where the execution is really paused.
I think Patrick ended up identifying all the precise spot where we really pause.

> 3) after talking with Jim Blandy, I realized that the client needed to
> inform the server of user significant pauses. This would be cleaner and
> require less intervention inside of the threadActor. An extra bonus here was
> that we could directly reuse the translations from the debugger client,
> making it much easier to implement the next step: showing the pause reason.

That's a good point. We may be able to know the reasons from ThreadActor.
But it would be harder to fetch translations from the actor or would require duplicated strings hosted in m-c instead of github. (There is a couple of strings translated in actors, but this is rare)

> I initially implemented this in the debugger frontend, making the message in
> the packet useful. Jason asked me to move it back to MC, he said that he
> would rather duplicate the logic.

Why? I don't see any particular reason mentioned here?

> Im writing this summary because I think I am the only one who knows
> everthing that has been going on.
 
Thanks for doing such summary of the discussions!

> My suggestion is that the best solution is the keep the threadActor and the
> pauseOverlay separate. threadActor doesn't care about the pauseOverlay and
> the PauseOverlay only cares if it is shown.

+1 PauseOverlay only care about the paused/resumed events, no matter where it lives (client vs server)

> My recommendation: remove everything inside of threadActor that triggers the
> pause overlay. then: either listen to the packets as they are being emitted,
> and respond in a similar way as the highlight tool, or just have the
> debugger client take care of triggering it. this reflects what we are doing
> with the inspector (ruler etc) and the architecture that chrome uses as well
> (i dont have a sample for that but I can find one)

Having said all that, making it pure client sounds fine to me if you manage to completely uncouple it from actors/script.js. In your last patch with "a more client oriented solution":
https://reviewboard.mozilla.org/r/190554/diff/4#index_header
ThreadActor was still managing the pause overlay.

With getHighlighterByType you should be able to completely manage it from client,
without any tweak to be made to script.js.

Still, I have some concern that should be investigated:
* Is debugger frontend/panel always up and running when we pause?
The overlay should be shown no matter what happens on the client side if we pause.
Debugger Panel is not loaded until we select/open it.
I imagine we may force loading it if we hit a `debugger;` statement?
(or may be it is ignored if debugger panel isn't opened?)
If we force load it, we should ensure correctly displaying the overlay on load.
A mitigation would be to put the overlay management into toolbox.js so that we do not depend on having the debugger panel loaded. But it may be harder to share strings from github?

* We should ensure hiding the overlay in some remote debugger cases
if you are debugging Firefox for Android page and have your usb cable to be brittle and disconnect,
the ThreadActor should resume execution. But if the overlay is client side, client won't have time to instruct the backend to hide it. Normally, all actors are destroyed when you close the connection. So their `destroy` method should be called. We would just need to ensure that the overlay is correctly hidden in such scenario.

* Is it fine constructing/displaying the overlay *after* it is paused?
Many things are known to be not working once we are paused (bug 1177346). But I think this is mostly events not firing. So may be we are just fine. I was wondering if some painting/reflows may be broken. But I imagine you would have noticed that already.


So. Sounds fine to me, but please look into "Is debugger frontend/panel always up and running when we pause?".

Comment 18

[Yulia Startsev [:yulia] OOO until July 2026]

Attachment: Bug 1354679 - Use highlighter actor from client instead of from the server

Review commit: https://reviewboard.mozilla.org/r/200862/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/200862/

Comment 19

[Yulia Startsev [:yulia] OOO until July 2026]

Comment on attachment 8920511 [details]
Bug 1354679 - Use highlighter actor from client instead of from the server

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191514/diff/1-2/

Comment 20

[Jim Blandy :jimb]

I don't have much to add to what Alex and Patrick said, other than to observe that whether a page is paused or not is of great significance to the developer, as all further JavaScript execution is (well, ought to be) forbidden. The page is frozen until the devtools thaw it. So if it is really that hard to tell when the page is paused, that's an architectural problem. It means the debugger is liable not to show the "play" button when necessary, for example.

If there is no Debugger instance of which the page is a debuggee that has onDebuggerStatement or onEnterFrame handlers or breakpoints set, then the page will never pause. We should not be creating Debugger instances with handlers or breakpoints unless they're going to carry out some useful task: either carry out some quick work and return (resuming the debuggee), or contacting the client. So I'm curious how we would end up paused without the client being made aware of it.

Comment 21

[Patrick Brosset <:pbro>]

This was picked up by Yulia last year. So adjusting the assignee field accordingly. It hasn't landed though, so maybe Yulia also wants to change the assignee field to reflect the current situation.

Comment 23

[Jason Laster [:jlast]]

Attachment: Bug 1354679 - Automatically display the PausedDebuggerOverlay when the debugger is paused. r=gl

Comment 24

[Gabriel Luong [:gl] (ΦωΦ)]

Comment on attachment 9072477 [details]
Bug 1354679 - Automatically display the PausedDebuggerOverlay when the debugger is paused. r=gl

I looked thru the current draft. I don't think I have any comments to add other than the fact that we can probably just add a class if we want pointer-events: auto.

Comment 25

[Jason Laster [:jlast]]

Breaking this down into a couple parts:

1. show an initial overlay with the top-line summary
2. add stepping buttons
3. re-visit why paused UI and the breakpoint paused UI

---

the inspector should hide the overlay when the picker is activated

Comment 26

[Marco Mucci [:MarcoM]]

Informed by Honza that work on this bug has been paused.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6363111ee8ce
Automatically display the PausedDebuggerOverlay when the debugger is paused. r=gl

Comment 28

[Jason Laster [:jlast]]

Attachment: Bug 1354679 - tweak pause overlay. r=gl

Comment 29

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6363111ee8ce

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa44f47160b2
tweak pause overlay. r=gl

Comment 31

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/fa44f47160b2

Comment 32

[Andrei Ciure[:aciure]]

Backed out changeset fa44f47160b2 (bug 1354679)for causing high frequency browser_aboutdebugging_devtoolstoolbox_shortcuts.js and browser_aboutdebugging_devtoolstoolbox_reload.js failures

push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=fa44f47160b2371069652b9ae763345d811bce32&selectedJob=256033654

failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&fromchange=835ac3ae20ba6c1f55c4e0bd7adeb133c45e0904&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Ctest-linux64%2Fdebug-mochitest-devtools-chrome-e10s-7%2Cm%28dt7%29&selectedJob=256252793

backout: https://hg.mozilla.org/integration/autoland/rev/cbac7fc3b4472c9f0f1a0d5c9be5c0f55f7419dd

Comment 33

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 6363111ee8ce (bug 1354679) for causing bug 1565833

Backout: https://hg.mozilla.org/integration/autoland/rev/7d9b5d8c9b9b36b135237292785537fc13f40226

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7d9b5d8c9b9b

Comment 35

[Jason Laster [:jlast]]

Cosmin, I'm not seeing any failures when i re-revert it looks like it safe to re-land. What do you think?

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=256822279&revision=d9f71c1608374aa9437a3492f8075695ff426d34

Comment 36

[Cosmin Sabou [:CosminS]]

Looks good to me.

Comment 37

[Jason Laster [:jlast]]

Attachment: Bug 1354679 - re-apply pause overlay. r=davidwalsh

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63ef8fdfff70
re-apply pause overlay. r=davidwalsh

Comment 39

[Cristian Brindusan [:cbrindusan]]

Backed out changeset 63ef8fdfff70 (Bug 1354679) for xpcshell failures at test_xpcshell_debugging.js.

Backout: https://hg.mozilla.org/integration/autoland/rev/5fff2a9bf0785afbdb774c178135cc9e9ad18211

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=63ef8fdfff70215c2f9194128c51c4b926db99bb&selectedJob=257285618

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=257285618&repo=autoland&lineNumber=2648

Comment 40

[Jason Laster [:jlast]]

Attachment: Revert "Backed out changeset 63ef8fdfff70 (Bug 1354679) for xpcshell failures at test_xpcshell_debugging.js."

This reverts commit e2023eef255628a3b10040b4b6b35c482efd152b.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63e23c9ed252
Revert "Backed out changeset 63ef8fdfff70 for xpcshell failures at test_xpcshell_debugging.js."

Comment 42

[Alexandre Poirot [:ochameau]]

(In reply to Pulsebot from comment #41)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63e23c9ed252
Revert "Backed out changeset 63ef8fdfff70 for xpcshell failures at
test_xpcshell_debugging.js."

Jason, could you please avoid landing revert of reverts?
This practice makes it very hard to read the changelog.
The best practice when you are re-landing a backed out changeset is to re-land as you first attempt of landing.
i.e. use the typical commit message:
Bug 1354679 - Your commit message r=reviewer

Comment 43

[Jason Laster [:jlast]]

Yep. Sorry about that.