1156531 - "attached" and "resumed" thread state names are confusing

Status: NEW

(DevTools :: Debugger, task, P5)

Description

[James Long (:jlongster)]

I've seen both "attached" and "resumed" used to indicate that the thread is currently running like normal. Both of this mean the same state, but we use these 2 different terms and it's very confusing.

After looking into it a bit, it looks like the `thread.state` property will always be "attached" in this state, but the thread will always fire an event named "resumed" when it switches to this state. The `thread.state` property is `attached` because we map the event name to it here: https://github.com/mozilla/gecko-dev/blob/master/toolkit/devtools/client/dbg-client.jsm#L206

I think this is unnecessary and creates more confusion. For example, here's a test which calls `ensureThreadClientState(aPanel, "resumed")`:

https://github.com/mozilla/gecko-dev/blob/master/browser/devtools/debugger/test/browser_dbg_break-on-dom-04.js#L61

`ensureThreadClientState` will check the `thread.state` property to see if it matches the supplied state. If it doesn't, it waits for an event to be fired of the same name as the supplied argument. Because the state and event name differ this call is faulty and prone to race conditions; if the thread has already been resumed the state will be `attached` and `ensureThreadClientState` will wait for an event named "resumed" but it will never come.

Not sure what the action item is here yet, but we should clarify this.

Comment 1

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

The most confusing part is that ThreadStateTypes in the client doesn't include all the possible thread states in the actor, which are: detached, attached, paused, running and exited. See this (old and slightly inaccurate) state diagram:

https://wiki.mozilla.org/Remote_Debugging_Protocol#Interacting_with_Thread-Like_Actors

Comment 2

[James Long (:jlongster)]

Additionally, the `setBreakpoint` request doesn't make sure the `resume` request is done before calling the callback, which causes race conditions (see bug 896139). We need to fix that but can only do that once this is fixed because our tests depend on this brittle naming.