Closed Bug 1619622 Opened 5 years ago Closed 5 years ago

Use the TargetList for listening to and retrieving process targets

Categories

(DevTools :: Debugger, enhancement, P1)

Product:
DevTools
Component:
Debugger
Type:
enhancement

Tracking

(Fission Milestone:M6c, firefox78 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 78
Project Flags:
Fission Milestone M6c
Tracking Flags:
Tracking Status
firefox78 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(5 files, 5 obsolete files)

Bug 1619622 - Use the TargetList to fetch content process targets in the Debugger.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619622 - Update target/thread individually from the TargetList listeners.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619622 - Stop fetching the sources twice.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619622 - Assert the context in various additional places.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1619622 - Various fixes after switching processes to the TargetList
47 bytes, text/x-phabricator-request
Details | Review

This is blocking many of my ongoing work related to early targets and ressource API.
We have to make the Debugger use the TargetList for all kind of targets.
For now, the Debugger uses the TargetList only for the top-level target.
One first iteration would be to start using the TargetList just for the processes.
The TargetList exposes all what is needed already, which isn't quite the case for workers.
Bug 1594754 is aiming at improving the TargetList around workers.

Tracking Fission DevTools bugs for Fission Nightly (M6) milestone

Fission Milestone: --- → M6

We weren't actually returning any value.
The only case when we return something is when the RDP method throws and return
an object response with an "error" attribute.
I imagine this has been refactored incorectly from old style actor to protocol.js.
We should throw in case of errors if we want to transfer a RDP message with "error" attribute.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a2b75a25794 Remove unecessary attributes set on Target classes related to thread front. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/778c166fd33f Return only the thread front from Target.attachThread. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/ceeaf109d8b6 Stop returning any value from ThreadActor.attach. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/f2ac72165a3a Use the TargetList to fetch content process targets in the Debugger. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/d447fd6fa18d Use the TargetList to retrieve the current top level target in the debugger. r=jdescottes,jlast

Backed out 5 changesets for causing devtool failures in browser_dbg-browser-content-toolbox.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/68404668656d5c63f38a8f1775dbead60de828d4

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&tochange=fe6752ffe1b833abdafe07d447f8e3705dc4cd83&fromchange=5e32bdf73dc213e9944205411baa0d51df681140&test_paths=devtools%2Fclient%2Fdebugger%2Ftest%2Fmochitest&selectedJob=292720495

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=292720495&repo=autoland&lineNumber=4412

[task 2020-03-11T18:33:39.606Z] 18:33:39     INFO - Close the browser toolbox window
[task 2020-03-11T18:33:39.606Z] 18:33:39     INFO - Buffered messages logged at 18:33:39
[task 2020-03-11T18:33:39.606Z] 18:33:39     INFO - Toolbox is destroyed
[task 2020-03-11T18:33:39.607Z] 18:33:39     INFO - Buffered messages finished
[task 2020-03-11T18:33:39.630Z] 18:33:39     INFO - TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg-browser-content-toolbox.js | A promise chain failed to handle a rejection: Resource "server0.conn20.content-process2/source153" already exists, cannot insert - stack: insertResources@resource://devtools/client/debugger/src/utils/resource/core.js:42:13
[task 2020-03-11T18:33:39.630Z] 18:33:39     INFO - update@resource://devtools/client/debugger/src/reducers/source-actors.js:33:47
[task 2020-03-11T18:33:39.630Z] 18:33:39     INFO - combination@resource://devtools/client/shared/vendor/redux.js:502:36
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - dispatch@resource://devtools/client/shared/vendor/redux.js:256:22
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - log/</<@resource://devtools/client/debugger/src/actions/utils/middleware/log.js:114:9
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - waitUntilService/</<@resource://devtools/client/debugger/src/actions/utils/middleware/wait-service.js:71:24
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - promiseMiddleware/</<@resource://devtools/client/debugger/src/actions/utils/middleware/promise.js:46:14
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - context/</<@resource://devtools/client/debugger/src/actions/utils/middleware/context.js:35:12
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - thunk/</</<@resource://devtools/client/debugger/src/actions/utils/middleware/thunk.js:29:100
[task 2020-03-11T18:33:39.631Z] 18:33:39     INFO - dispatch@resource://devtools/client/shared/vendor/redux.js:681:28
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - insertSourceActors/<@resource://devtools/client/debugger/src/actions/source-actors.js:26:13
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - thunk/</</<@resource://devtools/client/debugger/src/actions/utils/middleware/thunk.js:29:45
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - dispatch@resource://devtools/client/shared/vendor/redux.js:681:28
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - newGeneratedSources/<@resource://devtools/client/debugger/src/actions/sources/newSources.js:363:13
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - thunk/</</<@resource://devtools/client/debugger/src/actions/utils/middleware/thunk.js:29:45
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - dispatch@resource://devtools/client/shared/vendor/redux.js:681:28
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - newQueuedSources/<@resource://devtools/client/debugger/src/actions/sources/newSources.js:234:13
[task 2020-03-11T18:33:39.632Z] 18:33:39     INFO - thunk/</</<@resource://devtools/client/debugger/src/actions/utils/middleware/thunk.js:29:45
[task 2020-03-11T18:33:39.633Z] 18:33:39     INFO - bindActionCreator/<@resource://devtools/client/shared/vendor/redux.js:520:12
[task 2020-03-11T18:33:39.633Z] 18:33:39     INFO - dispatchNewSources@resource://devtools/client/debugger/src/utils/source-queue.js:20:23
[task 2020-03-11T18:33:39.633Z] 18:33:39     INFO - invokeFunc@resource://devtools/client/shared/vendor/lodash.js:10333:23
[task 2020-03-11T18:33:39.633Z] 18:33:39     INFO - trailingEdge@resource://devtools/client/shared/vendor/lodash.js:10382:18
[task 2020-03-11T18:33:39.633Z] 18:33:39     INFO - timerExpired@resource://devtools/client/shared/vendor/lodash.js:10370:18
[task 2020-03-11T18:33:39.633Z] 18:33:39     INFO - setTimeout handler*leadingEdge@resource://devtools/client/shared/vendor/lodash.js:10341:19
[task 2020-03-11T18:33:39.634Z] 18:33:39     INFO - debounced@resource://devtools/client/shared/vendor/lodash.js:10410:20
[task 2020-03-11T18:33:39.634Z] 18:33:39     INFO - queue@resource://devtools/client/debugger/src/utils/source-queue.js:31:5
[task 2020-03-11T18:33:39.635Z] 18:33:39     INFO - newSource@resource://devtools/client/debugger/src/client/firefox/events.js:134:24
[task 2020-03-11T18:33:39.635Z] 18:33:39     INFO - _emit@resource://devtools/shared/event-emitter.js:226:34
[task 2020-03-11T18:33:39.636Z] 18:33:39     INFO - emit@resource://devtools/shared/event-emitter.js:172:18
[task 2020-03-11T18:33:39.636Z] 18:33:39     INFO - emit@resource://devtools/shared/event-emitter.js:324:18
[task 2020-03-11T18:33:39.636Z] 18:33:39     INFO - onPacket@resource://devtools/shared/protocol/Front.js:270:13
[task 2020-03-11T18:33:39.636Z] 18:33:39     INFO - onPacket@resource://devtools/shared/client/devtools-client.js:498:13
[task 2020-03-11T18:33:39.637Z] 18:33:39     INFO - send/<@resource://devtools/shared/transport/local-transport.js:68:25
[task 2020-03-11T18:33:39.637Z] 18:33:39     INFO - exports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:111:22
[task 2020-03-11T18:33:39.641Z] 18:33:39     INFO - DevToolsUtils.executeSoon*exports.executeSoon@resource://devtools/shared/DevToolsUtils.js:62:21
[task 2020-03-11T18:33:39.642Z] 18:33:39     INFO - send@resource://devtools/shared/transport/local-transport.js:56:21
[task 2020-03-11T18:33:39.642Z] 18:33:39     INFO - send@resource://devtools/server/devtools-server-connection.js:91:20
[task 2020-03-11T18:33:39.642Z] 18:33:39     INFO - receiveMessage@resource://devtools/shared/transport/child-transport.js:66:16
[task 2020-03-11T18:33:39.643Z] 18:33:39     INFO - MessageListener.receiveMessage*_addListener@resource://devtools/shared/transport/child-transport.js:40:14
[task 2020-03-11T18:33:39.643Z] 18:33:39     INFO - ready@resource://devtools/shared/transport/child-transport.js:57:10
[task 2020-03-11T18:33:39.644Z] 18:33:39     INFO - listener@resource://devtools/server/connectors/content-process-connector.js:43:22
[task 2020-03-11T18:33:39.644Z] 18:33:39     INFO - MessageListener.receiveMessage*connectToContentProcess/<@resource://devtools/server/connectors/content-process-connector.js:32:8
Flags: needinfo?(poirot.alex)

Comment on attachment 9130455 [details]
Bug 1619622 - Use the TargetList to fetch content process targets in the Debugger.

This patch is introducing the intermittent.
I'll probably try to land all the others, which seem to be green:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=292846156&revision=2f896184d77719f1f413dbe6d30ae8503bd7c0e1&test_paths=devtools%2Fclient%2Fdebugger%2Ftest%2Fmochitest%2F

Flags: needinfo?(poirot.alex)

Comment on attachment 9130453 [details]
Bug 1619622 - Remove unecessary attributes set on Target classes related to thread front.

Revision D65123 was moved to bug 1622219. Setting attachment 9130453 [details] to obsolete.

Attachment #9130453 - Attachment is obsolete: true

Comment on attachment 9130454 [details]
Bug 1619622 - Return only the thread front from Target.attachThread.

Revision D65124 was moved to bug 1622219. Setting attachment 9130454 [details] to obsolete.

Attachment #9130454 - Attachment is obsolete: true

Comment on attachment 9132099 [details]
Bug 1619622 - Stop returning any value from ThreadActor.attach.

Revision D66132 was moved to bug 1622219. Setting attachment 9132099 [details] to obsolete.

Attachment #9132099 - Attachment is obsolete: true

Comment on attachment 9132100 [details]
Bug 1619622 - Use the TargetList to retrieve the current top level target in the debugger.

Revision D66133 was moved to bug 1622222. Setting attachment 9132100 [details] to obsolete.

Attachment #9132100 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ee78eb11695d Use the TargetList to fetch content process targets in the Debugger. r=jdescottes

Oops, sorry, I should not have landed this bug's patch, but bug 1622222 instead. I've been confused with them both ending with 22 :/

Flags: needinfo?(poirot.alex)

No problem, thanks for looking!

Tentatively moving P1 Fission M6 bugs to M6a.

Fission Milestone: M6 → M6a

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:ochameau, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(poirot.alex)

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcaae0a991347ab043f565771777a6ae3197a675
This patch still fails:

TEST-UNEXPECTED-FAIL | devtools/client/debugger/test/mochitest/browser_dbg-browser-content-toolbox.js | A promise chain failed to handle a rejection: Resource "server0.conn21.content-process2/source156" already exists, cannot insert - stack: insertResources@resource://devtools/client/debugger/src/utils/resource/core.js:42:13

I'm wondering if we should get rid of this assertion.
The debugger frontend does fetch the sources from multiple places without and with this patch:

  1. the server dispatches the sources more than once because of bug 1523947. As it requires server side modification and possibly backward compat, It is worth waiting for bug 1620280 and its "ResourceWatcher server side" companion bug in order to prevent having to handle backward compat twice.
  2. the frontend call ThreadActor.sources, requesting all the sources, from multiple concurrent places:
    https://searchfox.org/mozilla-central/search?q=.fetchSources()&case=false&regexp=false&path=
    https://searchfox.org/mozilla-central/search?q=.fetchThreadSources(&case=false&regexp=false&path=

This patch hits a bit of all of this.
This defensive code doesn't seem to be enough to workaround the fact that we fetch sources multiple time.

Flags: needinfo?(poirot.alex)

We already do that from actions, once per added thread.
Only the timing may be different.

Blocks: 1634197
Attachment #9143683 - Attachment is obsolete: true
Attachment #9146469 - Attachment description: Bug 1619622 - Various fixes → Bug 1619622 - Various fixes after switching processes to the TargetList

Weird, I pushed to autoland and there was no notice of it being landed, nor it being backed out:
https://hg.mozilla.org/integration/autoland/rev/68404668656d
Sheriffs reports browser_dbg-browser-content-toolbox.js as still failing.
I rebased and pushed to try again:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a75beeb674dd2d998be69619213ca6630f07cd7

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c)

Fission Milestone: M6a → M6c
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dbb558861681 Use the TargetList to fetch content process targets in the Debugger. r=jdescottes,jlast https://hg.mozilla.org/integration/autoland/rev/d6a9cab6867e Update target/thread individually from the TargetList listeners. r=jdescottes,jlast https://hg.mozilla.org/integration/autoland/rev/954226e988d5 Stop fetching the sources twice. r=jdescottes,jlast https://hg.mozilla.org/integration/autoland/rev/de9b7ce3fd5a Assert the context in various additional places. r=loganfsmyth https://hg.mozilla.org/integration/autoland/rev/d3281755124e Various fixes after switching processes to the TargetList r=loganfsmyth
Regressions: 1641561
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: