The default bug view has changed. See this FAQ.

Properly handle location changes in the script debugger

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: past, Assigned: past)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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."
Assignee: nobody → past
Blocks: 676586
Status: NEW → ASSIGNED
Created attachment 563454 [details] [diff] [review]
WIP

Storing my WIP for safe-keeping.
Created attachment 564878 [details] [diff] [review]
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.
Attachment #563454 - Attachment is obsolete: true
Created attachment 565280 [details] [diff] [review]
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.
Attachment #564878 - Attachment is obsolete: true
Attachment #565280 - Flags: review?(dcamp)
Created attachment 566609 [details] [diff] [review]
Working patch v2

Rebased.
Attachment #565280 - Attachment is obsolete: true
Attachment #565280 - Flags: review?(dcamp)
Attachment #566609 - Flags: review?(dcamp)
Created attachment 567030 [details] [diff] [review]
Working patch v3

Also removed a redundant initialization of client.hooks, since it's not defined or used anywhere else.
Attachment #566609 - Attachment is obsolete: true
Attachment #566609 - Flags: review?(dcamp)
Attachment #567030 - Flags: review?(dcamp)
Created attachment 567109 [details] [diff] [review]
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.
Attachment #567030 - Attachment is obsolete: true
Attachment #567030 - Flags: review?(dcamp)
Attachment #567109 - Flags: review?(dcamp)

Comment 7

6 years ago
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.
Attachment #567109 - Flags: review?(dcamp) → review+
> >    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

6 years ago
(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.
(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
https://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/3964a5ecf3e9
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.