Closed Bug 1682780 Opened 4 years ago Closed 4 years ago

Stop pausing the thread on "attach"

Categories

(DevTools :: Debugger, enhancement, P3)

enhancement

Tracking

(Fission Milestone:M7, firefox86 fixed)

RESOLVED FIXED
86 Branch
Fission Milestone M7
Tracking Status
firefox86 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

(Whiteboard: dt-fission-m3-mvp )

Attachments

(5 files)

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:

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!

Severity: -- → S3
Priority: -- → P3
Whiteboard: dt-fission-m3-mvp

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.
Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

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

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

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.

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

Fission Milestone: --- → MVP
Pushed by apoirot@mozilla.com: 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
See Also: → 1657059

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

Fission Milestone: MVP → M7
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: