1682780 - Stop pausing the thread on "attach"

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P3)

Description

[Alexandre Poirot [:ochameau]]

Bug 697762 originaly introduced most of the ThreadActor implementation.
When this landed, we were already having the pattern, where, when we call ThreadActor.attach, we:

• initialize the Debugger API (dbg.enabled = true;)
• pause the current thread (call to this._nest(), which spawns a nested event loop)
  https://searchfox.org/mozilla-central/rev/854933e0fb842877a6665c45fc69386665ddb9e2/toolkit/devtools/debugger/server/dbg-script-actors.js#141-157
  And this pattern never changed.

In bug 1573327 I was having an issue with this pattern as I need to initialize the Debugger API and register breakpoints. But I don't want to pause the current thread.
This makes me wonder if we really need this pattern or if this is pure legacy.

Nowadays, the ThreadActor.attach method is here:
https://searchfox.org/mozilla-central/source/devtools/server/actors/thread.js#376-429

Could we possibly stop pausing the thread on attach ?
This would simplify a bit the code on the frontend side, where, today, we have to call threadActor.resume just after the call to threadActor.attach:
https://searchfox.org/mozilla-central/source/devtools/client/fronts/targets/target-mixin.js#489-495
We could then only call threadFront.attach(options) and be done with thread actor initialization.
And then, ultimately, via bug 1681698, the thread actor would be automatically initialized from the server codebase and the frontend wouldn't have to attach/initialize the thread actor anymore!

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1682780 - [devtools] Remove unused callback arguments from xpshell debugger test helpers.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1682780 - [devtools] Avoid pausing on thread actor's attach request.

We always used to pause when "attaching" the thread actor.
We ought to call ThreadActor's attach method first before using it.
And this method do various things:

• Initialize the Debugger API, enable it, register various listeners, so that breakpoint can work,
• Pause the current thread by starting a nested event loop
  So that we also ought to resume the thread, by calling ThreadActor's resume right after attach.
  Otherwise the page would be paused as soon as we open the DevTools.
  Which sounds like something we might have wanted a very long time ago.
  But sounds like pure legacy behavior from today's perspective.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1682780 - [devtools] Adapt tests and doc now that ThreadActor.attach no longer pause the thread.

Comment 4

[Alexandre Poirot [:ochameau]]

Nicolas, Julian, I got a chat with Jim and he confirmed me that we could change that.

Jim's words about this:

"Could we possibly stop pausing the thread on attach?" Yes, absolutely we could.
The concept of 'pause' makes sense for the user, because it's important not to be processing events while stopped at a breakpoint.
But it shouldn't ever have been treated as essential for internal reasons.

In parallel of that, I was already playing with these patchs and saw that it was mostly about changing the tests assuming you had to resume on attach, and also about handling backward compat. Where it is important to keep resuming, still.

This chat ended up in a long discussion about Event loops, Tasks and Micro tasks, which I should debrief to the whole team. This is super interesting topic to cover in order to better understand the debugger and the executable Web :o

Comment 5

[Alexandre Poirot [:ochameau]]

(Julian, Nicolas, have a look at my previous comment)

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1682780 - [devtools] Remove special hack around ThreadActor.attach from protocol.js internals.

This is no longer necessary as attach is no longer entering in a nested event loop.
And so we can have attach to complete and return its value as any other request.

Comment 7

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m3-mvp bugs for Fission MVP.

Comment 8

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1682780 - [devtools] Used contants to define ThreadActor `state` attribute.

Comment 9

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60f837a9856a9d95d2c1a76130401dd15ce10976

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d78ba6281178
[devtools] Remove unused callback arguments from xpshell debugger test helpers. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/817b052be3c9
[devtools] Avoid pausing on thread actor's attach request. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/4052bf56a359
[devtools] Adapt tests and doc now that ThreadActor.attach no longer pause the thread. r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/bca052c8fe2d
[devtools] Remove special hack around ThreadActor.attach from protocol.js internals. r=nchevobbe,jdescottes
https://hg.mozilla.org/integration/autoland/rev/c052cd78f9fa
[devtools] Used contants to define ThreadActor `state` attribute. r=jdescottes,nchevobbe

Comment 11

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d78ba6281178
https://hg.mozilla.org/mozilla-central/rev/817b052be3c9
https://hg.mozilla.org/mozilla-central/rev/4052bf56a359
https://hg.mozilla.org/mozilla-central/rev/bca052c8fe2d
https://hg.mozilla.org/mozilla-central/rev/c052cd78f9fa

Comment 13

[Chris Peterson [:cpeterson]]

This completed dt-fission-m3-mvp bug should have Fission Milestone M7 Beta.