Stop pausing the thread on "attach"
Categories
(DevTools :: Debugger, enhancement, P3)
Tracking
(Fission Milestone:M7, firefox86 fixed)
Tracking | Status | |
---|---|---|
firefox86 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
(Blocks 1 open bug)
Details
(Whiteboard: dt-fission-m3-mvp )
Attachments
(5 files)
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
Bug 1682780 - [devtools] Adapt tests and doc now that ThreadActor.attach no longer pause the thread.
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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!
Updated•4 years ago
|
Assignee | ||
Comment 1•4 years ago
|
||
Assignee | ||
Comment 2•4 years ago
|
||
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.
Updated•4 years ago
|
Assignee | ||
Comment 3•4 years ago
|
||
Assignee | ||
Comment 4•4 years ago
|
||
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
Assignee | ||
Comment 5•4 years ago
|
||
(Julian, Nicolas, have a look at my previous comment)
Assignee | ||
Comment 6•4 years ago
|
||
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•4 years ago
|
||
Tracking dt-fission-m3-mvp bugs for Fission MVP.
Assignee | ||
Comment 8•4 years ago
|
||
Assignee | ||
Comment 9•4 years ago
|
||
Comment 10•4 years ago
|
||
Comment 11•4 years ago
|
||
bugherder |
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•4 years ago
|
||
This completed dt-fission-m3-mvp
bug should have Fission Milestone M7 Beta.
Description
•