Closed Bug 1354679 Opened 7 years ago Closed 5 years ago

Automatically display the PausedDebuggerOverlay when the debugger is paused

Categories

(DevTools :: Debugger, enhancement, P2)

Product:
DevTools
Component:
Debugger
Type:
enhancement

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: pbro, Assigned: jlast)

References

(Blocks 1 open bug, Regressed 1 open bug)

Details

(Keywords: parity-chrome, Whiteboard: [debugger-mvp])

Attachments

(7 files, 5 obsolete files)

Bug 1354679 - Show the paused debugger overlay when script gets paused;
59 bytes, text/x-review-board-request
Details
Bug 1354679 - Show the paused debugger overlay when script gets paused
59 bytes, text/x-review-board-request
Details
Bug 1354679 - Use highlighter actor from client instead of from the server
59 bytes, text/x-review-board-request
Details
Bug 1354679 - Show the paused debugger overlay when script gets paused;
59 bytes, text/x-review-board-request
Details
Bug 1354679 - Show the paused debugger overlay when script gets paused;
59 bytes, text/x-review-board-request
Details
Bug 1354679 - Use highlighter actor from client instead of from the server
59 bytes, text/x-review-board-request
Details
Revert "Backed out changeset 63ef8fdfff70 (Bug 1354679) for xpcshell failures at test_xpcshell_debugging.js."
47 bytes, text/x-phabricator-request
Details | Review 
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.
Assignee: nobody → pbrosset
Status: NEW → ASSIGNED
(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 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
Attachment #8860863 - Flags: review?(poirot.alex)
Attachment #8919661 - Attachment is obsolete: true
Attachment #8919661 - Flags: review?(poirot.alex)
Attachment #8919662 - Attachment is obsolete: true
Attachment #8919662 - Flags: review?(poirot.alex)
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, ...)
Attachment #8919660 - Flags: review?(poirot.alex)
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
Flags: needinfo?(poirot.alex)
Flags: needinfo?(pbrosset)
Flags: needinfo?(jlaster)
Flags: needinfo?(jimb)
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!
Flags: needinfo?(pbrosset)
(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?".
Flags: needinfo?(poirot.alex)
Attachment #8919660 - Flags: review?(poirot.alex)
Attachment #8919660 - Flags: review?(pbrosset)
Flags: needinfo?(jlaster)
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.
Flags: needinfo?(jimb)
Product: Firefox → DevTools

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.

Assignee: pbrosset → ystartsev
Assignee: ystartsev → nobody
Blocks: dbg-frontend
Status: ASSIGNED → NEW
Priority: -- → P3
Blocks: 1353564
Priority: P3 → P2
Blocks: dbg-70
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Whiteboard: [debugger-mvp]
Attachment #9072477 - Flags: feedback?(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.

Attachment #9072477 - Flags: feedback?(gl)

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

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

Assignee: jlaster → nobody
Status: ASSIGNED → NEW
Whiteboard: [debugger-mvp] → [debugger-reserve]
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6363111ee8ce
Automatically display the PausedDebuggerOverlay when the debugger is paused. r=gl
Assignee: nobody → jlaster
Status: NEW → ASSIGNED
Whiteboard: [debugger-reserve] → [debugger-mvp]

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

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70
Regressions: 1544828
Depends on: 1565375
Blocks: 1565711
Blocks: 1565713
See Also: → 1565833

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

Flags: needinfo?(jlaster)
Flags: needinfo?(csabou)
Flags: needinfo?(aryx.bugmail)

Looks good to me.

Flags: needinfo?(csabou)
Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63ef8fdfff70
re-apply pause overlay. r=davidwalsh

This reverts commit e2023eef255628a3b10040b4b6b35c482efd152b.

Pushed by jlaster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63e23c9ed252
Revert "Backed out changeset 63ef8fdfff70 for xpcshell failures at test_xpcshell_debugging.js."

(In reply to Pulsebot from comment #41)

Pushed by jlaster@mozilla.com:
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

Flags: needinfo?(jlaster)

Yep. Sorry about that.

Flags: needinfo?(jlaster)
Attachment #9076957 - Attachment is obsolete: true
Attachment #9079170 - Attachment is obsolete: true
Attachment #9072477 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 5 years ago → 5 years ago
Resolution: --- → FIXED
Depends on: 1568693
Depends on: 1568694
No longer blocks: 1565711
No longer blocks: 1565713
Blocks: dbg-overlay
No longer blocks: dbg-frontend, 1353564
No longer depends on: 1565375, 1568693, 1568694
Regressions: 1590932
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: