Wait for full thread resume in debugger tests
Categories
(DevTools :: Debugger, enhancement)
Tracking
(Fission Milestone:M7, firefox88 fixed)
| Tracking | Status | |
|---|---|---|
| firefox88 | --- | fixed |
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
(Whiteboard: dt-fission-m3-mvp)
Attachments
(5 files)
For now, the resume test helper only waits for the action to complete:
https://searchfox.org/mozilla-central/source/devtools/client/debugger/test/mochitest/helpers.js#797-801
But this doesn't wait for the THREAD_STATE's resume resource.
Not waiting for this means that we may have some pending requests when the toolbox closes. It also mean that the test may start running another action, while the thread isn't fully paused yet.
I ended up having test failure in bug 1631451 because of that.
Then, once I started looking at pause reducers, I saw a lot of unused methods.
And also some discrepancies between threadcx.isPaused and getIsPaused.
While the first (threadcx.isPaused) is manually updated, mostly following THREAD_STATE resources. "Mostly", because on thread selection, we will follow the very different behavior of getIsPaused...
And so the second (getIsPaused) will report the thread as being paused if its frames array is defined. This looks a bit brittle and very implicit. This is very different from the first threadcx.isPaused behavior, while it has the same goal.
So I took the opportunity to clean this up and set the thread as paused or running based on THREAD_STATE resources.
This will:
- simplify the codebase,
- ensure we always have a unified way to consider that a thread is paused or not,
- and, last but not least, if we wait for getIsPaused() to be false from the resume test helper, we can avoid pending requests at teardown!
|
Assignee | |
Comment 1•4 years ago
|
||
Hopefully try won't be mad with such fixes:
https://treeherder.mozilla.org/jobs?repo=try&revision=8b5cd9ae07ea5a3fdcc76cc3da0c48f128c2c416
|
|
Assignee | |
Comment 2•4 years ago
|
||
|
|
Assignee | |
Comment 3•4 years ago
|
||
|
|
Assignee | |
Comment 4•4 years ago
|
||
This is mostly a prep work for the next changeset.
So that we can pull the isPaused information from the already existing per-thread state object.
|
|
Assignee | |
Comment 5•4 years ago
|
||
This help get rid of the redundant threadcx.isPaused attribute,
which isn't trivial to maintain as it duplicates a per thread data.
The SELECT_THREAD codepath was especially special as we bound isPaused
to perThreadData.frames being defined or not.
But "frames" is having a very different behavior from isPaused.
Following changeset is going to revisit how we define that a thread is paused.
|
|
Assignee | |
Comment 6•4 years ago
|
||
The previous algorithm was ignoring THREAD_STATE resources.
We should consider thread as being paused or running based on these events,
which will fire RESUME and PAUSED actions.
|
|
Assignee | |
Comment 7•4 years ago
|
||
|
||
Updated•4 years ago
|
|
|
||
Updated•4 years ago
|
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/53511941f932 [devtools] Wait for thread to be running against after resume in all debugger tests. r=nchevobbe,bomsy https://hg.mozilla.org/integration/autoland/rev/fdc149b35d9b [devtools] Remove unused attributes and functions from pause reducer. r=nchevobbe,bomsy https://hg.mozilla.org/integration/autoland/rev/65b2289ab034 [devtools] Stop using isPaused from thread context. r=nchevobbe,bomsy https://hg.mozilla.org/integration/autoland/rev/6e95e23c0096 [devtools] Pull isPaused state from per-thread data. r=nchevobbe,bomsy https://hg.mozilla.org/integration/autoland/rev/9e5f5462892f [devtools] Stop considering that thread with no frames are running. r=nchevobbe,bomsy
|
||
Comment 9•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/53511941f932
https://hg.mozilla.org/mozilla-central/rev/fdc149b35d9b
https://hg.mozilla.org/mozilla-central/rev/65b2289ab034
https://hg.mozilla.org/mozilla-central/rev/6e95e23c0096
https://hg.mozilla.org/mozilla-central/rev/9e5f5462892f
|
||
Updated•3 years ago
|
Description
•