Last Comment Bug 676590 - Properly handle location changes in the script debugger
: Properly handle location changes in the script debugger
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Panos Astithas [:past]
:
Mentors:
Depends on:
Blocks: minotaur
  Show dependency treegraph
 
Reported: 2011-08-04 11:04 PDT by Panos Astithas [:past]
Modified: 2011-10-18 08:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (14.47 KB, patch)
2011-09-29 10:30 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
WIP 2 (22.43 KB, patch)
2011-10-05 09:20 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch (27.88 KB, patch)
2011-10-06 10:42 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v2 (27.94 KB, patch)
2011-10-12 12:31 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v3 (27.91 KB, patch)
2011-10-14 01:55 PDT, Panos Astithas [:past]
no flags Details | Diff | Splinter Review
Working patch v4 (30.40 KB, patch)
2011-10-14 08:54 PDT, Panos Astithas [:past]
dcamp: review+
Details | Diff | Splinter Review

Description Panos Astithas [:past] 2011-08-04 11:04:03 PDT
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 [:past] 2011-09-29 10:30:56 PDT
Created attachment 563454 [details] [diff] [review]
WIP

Storing my WIP for safe-keeping.
Comment 2 Panos Astithas [:past] 2011-10-05 09:20:13 PDT
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.
Comment 3 Panos Astithas [:past] 2011-10-06 10:42:26 PDT
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.
Comment 4 Panos Astithas [:past] 2011-10-12 12:31:05 PDT
Created attachment 566609 [details] [diff] [review]
Working patch v2

Rebased.
Comment 5 Panos Astithas [:past] 2011-10-14 01:55:42 PDT
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.
Comment 6 Panos Astithas [:past] 2011-10-14 08:54:19 PDT
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.
Comment 7 Dave Camp (:dcamp) 2011-10-17 10:36:15 PDT
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] 2011-10-17 13:06:39 PDT
> >    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) 2011-10-17 15:31:15 PDT
(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 [:past] 2011-10-18 01:26:00 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.