1717005 - Cleanup code handling stack of event loops

Status: RESOLVED FIXED

(DevTools :: Debugger, task)

Description

[Alexandre Poirot [:ochameau]]

Various code in devtools seems to assume that we can pause a given thread actor multiple times.
For example, the toolbox, by having a Set of _pausedThread implicitely allows to handle nicely the fact that a given thread actor can pause multiple times and resume on the first resumed event.
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/client/framework/toolbox.js#680-688
Another example is the EventLoopStack:
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/server/actors/utils/event-loop.js#10-16
which very explicitely suppose we can spawn a stack of nested event loop, so that the thread actor can pause more than once at a given time.
And this is being made very explicit from the thread actor point of view:
https://searchfox.org/mozilla-central/source/devtools/server/actors/thread.js#321-340

But since day one, the thread actor never supported pausing while being already paused:
https://searchfox.org/mozilla-central/rev/854933e0fb842877a6665c45fc69386665ddb9e2/toolkit/devtools/debugger/server/dbg-script-actors.js#363-373
This comment has been kept and is still visible here:
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/server/actors/thread.js#1619-1628

    // We don't handle nested pauses correctly.  Don't try - if we're
    // paused, just continue running whatever code triggered the pause.
    // We don't want to actually have nested pauses (although we
    // have nested event loops).  If code runs in the debuggee during
    // a pause, it should cause the actor to resume (dropping
    // pause-lifetime actors etc) and then repause when complete.

    if (this.state === STATES.PAUSED) {
      return undefined;
    }

On top of that, in all other places where we call _pushThreadPause, we do bail out if the thread actor is already paused.
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/server/actors/thread.js#1564,1569,1607
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/server/actors/thread.js#1970-1972,2032

Now bug 901712 introduced some confusion as unsafeSynchronize uses EventLoopStacks and can do spawn nested event loops while the debugger is paused.
But:

1. unsafeSynchronize is used only in one place and can probably be dropped
2. this is completely independant of thread pausing on any kind of breakpoint, this is an helper to have some async code to block the execution of the server codebase. This isn't really meant to block page's execution.

On top of that, if you are testing Firefox and Chrome, we could not find any STR where we would pause while already being paused. We can do pause in some other thread (workers, remote iframes,...) but never from the same thread that is currently paused.
For example, calling some code where a breakpoint is set from the console won't trigger the breakpoint and the code will run to full completion. Similar thing happens with debugger statements.

Having said all that bug 901712 also highlighted a valid case where a given thread (not a thread actor, but a given execution thread) can be paused by distinct thread actor instances. Typically if two DevToolsClient spawn two ThreadActors and both could pause the thread on top of each others.
The following test covers exactly that:
https://searchfox.org/mozilla-central/source/devtools/server/tests/xpcshell/test_nesting-03.js
This should be kept working. But this particular behavior isn't handled by EventLoopStack. The two thread actors won't share a single EventLoopStack instance. Instead they will use the same xpcInspector c++ component, which will be the contact point.
EventLoopStack includes some code really specific to multiple thread actor coordination, but we ever push only one EventLoop instance for a given thread actor:
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/server/actors/utils/event-loop.js#51-58

So I think we can drastically simplement ThreadActor and EventLoopStack in order to better acknowledge that a thread actor can pause only once and spawn a nested event loop only once at a given time. All that while still handling the fact that two thread actors may pause the same thread.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1717005 - [devtools] Stop using unsafeSynchronize.

I don't think it was any useful to block synchronously on the call to doResume.
In the past it used to send an event to the client synchronously,
it may have been important back then.

test_nesting-03.js is still covering nested event loop by using two clients.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1717005 - [devtools] Simplify nested event loop management.

Comment 3

[Alexandre Poirot [:ochameau]]

try run:
https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=JFLnSH9DSt6GsJ0HaSCtAA.0&revision=2591de80dc526abc2b161cc7925771a5ca99e782

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/efab6db45952
[devtools] Stop using unsafeSynchronize. r=jdescottes,bomsy
https://hg.mozilla.org/integration/autoland/rev/7bb80026b599
[devtools] Simplify nested event loop management. r=jdescottes,bomsy

Comment 5

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/efab6db45952
https://hg.mozilla.org/mozilla-central/rev/7bb80026b599