676590 - Properly handle location changes in the script debugger

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

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

The script debugger is not handling location changes correctly at the moment. Dave Camp has suggested the following course of action for tackling this:

"There's no protocol operation speced yet, but we expect the
BrowserTabActor to create a new ThreadActor and put the old one in an
exited[1] state.  The new ThreadActor should be attached and paused.
A notification should sent to the client that there's a newly
attached/paused ThreadActor, giving the client time to set new
breakpoints[2] or whatever.  Then the client can resume the load.

Implementing that pause behavior is really the tricky part.  I don't
think we need to implement it immediately, though.

[1] Down the road we might want to expose bfcache behavior, and put
the ThreadActor in a 'bfcached' state, but we can ignore that for now.
[2] This needs to happen before we have any scripts, so it will need
to include some sort of breakpoint storage waiting for scripts to
arrive."

Comment 1

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

Attachment: WIP

Storing my WIP for safe-keeping.

Comment 2

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

Attachment: WIP 2

Updated, but still WIP. Now I can refresh any number of times and the debugger works properly. Test still not working and a number of cleanups are still required.

Comment 3

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

Attachment: Working patch

This is a working version with a test. It contains various unrelated bug fixes and cleanups that I made while debugging this. I could split them into a separate patch (and bug), but it seems unworthy of the effort at this point. 

The thing that I wanted in this bug was (a) to properly hook into the new window object and (b) to get all the scripts in the page visible in the debugger UI. Extra stuff like breakpoint setting, will have to wait until we get breakpoint support in the UI.

The approach I followed was getting the debugger server to emit events when new scripts appear, instead of having the client poll for the list at page load, because that seemed more efficient and simple. I also removed from the server the responsibility to react on tab navigation events, because besides the redundancy of having both server and client handling the event separately, there were synchronization issues that popped up.

Comment 4

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

Attachment: Working patch v2

Rebased.

Comment 5

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

Attachment: Working patch v3

Also removed a redundant initialization of client.hooks, since it's not defined or used anywhere else.

Comment 6

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

Attachment: Working patch v4

Cleaned up the client shutdown sequence, in order to properly detach from tabs and threads in the process. This simplifies the client close() method and renders it sufficient for debugger shutdown.

Comment 7

[Dave Camp (:dcamp)]

Comment on attachment 567109 [details] [diff] [review]
Working patch v4

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

Looks good.

Do we have a bug filed for pause-on-navigation?  We should file one if not.

::: browser/devtools/debugger/content/debugger.js
@@ +121,5 @@
>     * Update the UI after a thread state change.
>     */
>    update: function TS_update(aEvent) {
>      DebuggerView.Stackframes.updateState(this.activeThread.state);
> +    if (aEvent == "detached") {

Should probably === here.

Comment 8

[Dão Gottwald [:dao]]

> >    update: function TS_update(aEvent) {
> >      DebuggerView.Stackframes.updateState(this.activeThread.state);
> > +    if (aEvent == "detached") {
> 
> Should probably === here.

There's no case where aEvent == "detached" would be different from aEvent === "detached", is there?

Comment 9

[Dave Camp (:dcamp)]

(In reply to Dão Gottwald [:dao] from comment #8)
> There's no case where aEvent == "detached" would be different from aEvent
> === "detached", is there?

Nah, not in practice.  Whatever you prefer, panos.

Comment 10

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

(In reply to Dave Camp (:dcamp) from comment #7)
> Comment on attachment 567109 [details] [diff] [review] [diff] [details] [review]
> Working patch v4
> 
> Review of attachment 567109 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Looks good.
> 
> Do we have a bug filed for pause-on-navigation?  We should file one if not.

We have bug 690771, but I assume you mean something more specific to tab navigation. I'll file a new one, but can you clarify what is the exact requirement here? Perhaps that the server should pause the debuggee when sending the tabNavigated event in order to give the client a chance to set breakpoints and then resume?

> ::: browser/devtools/debugger/content/debugger.js
> @@ +121,5 @@
> >     * Update the UI after a thread state change.
> >     */
> >    update: function TS_update(aEvent) {
> >      DebuggerView.Stackframes.updateState(this.activeThread.state);
> > +    if (aEvent == "detached") {
> 
> Should probably === here.

I went with == in part to conform to the guideline here:

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide#Operators

Comment 11

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

https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/3964a5ecf3e9