690615 - Debugger ThreadClient should prevent packets from being sent during invalid states

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Dave Camp (:dcamp)]

In particular, we shouldn't allow most requests while the debuggee is running.

Comment 1

[Dave Camp (:dcamp)]

Attachment: v1

This adds an assertion to most ThreadClient requests.  The only one I avoided was detach, which is specifically called out in the protocol spec as executable from a running state.

Some of these requests are things I wouldn't immediately assume would need to be paused to use (for example, setting breakpoints or listing scripts).  setBreakpoint *is* explicitly specced as needing the paused state.  The scripts request isn't specced at all (that should probably be a separate bug).

Requiring a paused state isn't particularly bad for these things, but it does mean we need a "pause" request that can pause us at the event queue.

Thoughts?

Comment 2

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

Comment on attachment 563614 [details] [diff] [review]
v1

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

(In reply to Dave Camp (:dcamp) from comment #1)
> Created attachment 563614 [details] [diff] [review] [diff] [details] [review]
> v1
> 
> This adds an assertion to most ThreadClient requests.  The only one I
> avoided was detach, which is specifically called out in the protocol spec as
> executable from a running state.
> 
> Some of these requests are things I wouldn't immediately assume would need
> to be paused to use (for example, setting breakpoints or listing scripts). 
> setBreakpoint *is* explicitly specced as needing the paused state.  The
> scripts request isn't specced at all (that should probably be a separate
> bug).
> 
> Requiring a paused state isn't particularly bad for these things, but it
> does mean we need a "pause" request that can pause us at the event queue.
> 
> Thoughts?

My only observation is that requiring a paused state for the scripts request, essentially leads the location change implementation in bug 676590 towards the unsolicited notification solution. Fetching the script list at DOMContentLoaded (or similar) would happen in a running state, unless we could pause the debuggee  somehow (but would that allow the loading to proceed and the event to fire?). So if you haven't made your mind up yet about that, you might want to avoid the assertion in that case.

Should we file a bug to get a mechanism for pausing the debuggee or should we make sure that the protocol states are properly thought out to never require such a knob?

Also, should I file a bug for adding the scripts request to the spec or is the email I had sent about a month ago enough to put it in Jim's queue?

Both of these questions (pausing the debuggee and getting the scripts) I think are returning to the core issue of how does the protocol model the server-to-client notifications, and of course could influence the direction we take.

::: toolkit/devtools/debugger/dbg-client.jsm
@@ -526,3 +555,3 @@
> >        || !this._frameCache[this._frameCache.length - 1].oldest;
> >    },
> >  

I think you have to use parens here if you want to hide the button when the debugger is not paused. && has higher precedence than ||, so:

false && true || true || true === true

Comment 3

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

(In reply to Panos Astithas [:past] from comment #2)
> ::: toolkit/devtools/debugger/dbg-client.jsm
> @@ -526,3 +555,3 @@
> > >        || !this._frameCache[this._frameCache.length - 1].oldest;
> > >    },
> > >  
> 
> I think you have to use parens here if you want to hide the button when the
> debugger is not paused. && has higher precedence than ||, so:
> 
> false && true || true || true === true

So splinter uses only three lines of context, huh?
To be clear, I meant adding parens right after the &&:

return this.state === "paused" &&
  (!this._frameCache || this._frameCache.length == 0
  || !this._frameCache[this._frameCache.length - 1].oldest);

Comment 4

[Dave Camp (:dcamp)]

(In reply to Panos Astithas [:past] from comment #3)
> (In reply to Panos Astithas [:past] from comment #2)
> So splinter uses only three lines of context, huh?
> To be clear, I meant adding parens right after the &&:
> 
> return this.state === "paused" &&
>   (!this._frameCache || this._frameCache.length == 0
>   || !this._frameCache[this._frameCache.length - 1].oldest);


What am I, a toddler?  Fixed in remote-debug.

Comment 5

[Dave Camp (:dcamp)]

(In reply to Panos Astithas [:past] from comment #2)

> 
> My only observation is that requiring a paused state for the scripts
> request, essentially leads the location change implementation in bug 676590
> towards the unsolicited notification solution. Fetching the script list at
> DOMContentLoaded (or similar) would happen in a running state, unless we
> could pause the debuggee  somehow (but would that allow the loading to
> proceed and the event to fire?). So if you haven't made your mind up yet
> about that, you might want to avoid the assertion in that case.

I took out the assertion for scripts right now, pending a way to pause the debugger.
 
> Should we file a bug to get a mechanism for pausing the debuggee or should
> we make sure that the protocol states are properly thought out to never
> require such a knob?

Yeah, should probably file that bug.  We need to pause one way or another anyway.  I'll file.

> 
> Also, should I file a bug for adding the scripts request to the spec or is
> the email I had sent about a month ago enough to put it in Jim's queue?

Jim's been managing the protocol change queue himself, but I bet we could convince him to move that to bugzilla.
 
> Both of these questions (pausing the debuggee and getting the scripts) I
> think are returning to the core issue of how does the protocol model the
> server-to-client notifications, and of course could influence the direction
> we take.

Yep.

Comment 6

[Dave Camp (:dcamp)]

http://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/91bc9b4b2ac7
http://hg.mozilla.org/users/dcamp_campd.org/remote-debug/rev/2fd4e08577bc