Closed Bug 1494796 Opened 1 year ago Closed 1 year ago

Convert ThreadClient to protocol.js front

Categories

(DevTools :: Framework, task, P1)

task

Tracking

(Fission Milestone:M3, firefox69 fixed)

RESOLVED FIXED
Firefox 69
Fission Milestone M3
Tracking Status
firefox69 --- fixed

People

(Reporter: ochameau, Assigned: yulia)

References

(Blocks 4 open bugs)

Details

(Whiteboard: dt-fission [devtools-backward-compat] dt-fission-m1)

Attachments

(22 files, 5 obsolete files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
ThreadClient is the client class to connect to the thread actor.
We should convert that client class to a protocol.js front.

It may help us later in fission ongoing work to more easily manage multiple thread fronts. We could use target.getFront instead of having to come up with another mechanism specific to it.

Note that this can be done before converting the actor to protocol.js. Both refactoring are independant.

Also note that it must be the hardest client to refactor to protocol.js.
That's because low level transport layer contains various code that are specific to the thread client. We don't do that for any other client/actor!

https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/devtools/shared/client/debugger-client.js#845-846

https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/devtools/shared/client/debugger-client.js#856-861

https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/devtools/shared/client/constants.js#55-67
https://searchfox.org/mozilla-central/rev/6d1ab84b4b39fbfb9505d4399857239bc15202ef/devtools/shared/client/constants.js#7-16

These specifics might be necessary? But hopefully they could also be historical cruft that could be replace with regular protocol.js requests/events...
Whiteboard: dt-fission
Severity: normal → enhancement
Priority: -- → P2
Blocks: dbg-fission
Depends on: 1529247
Blocks: 1529163
Depends on: 1532567
Fission Milestone: --- → M2
Fission Milestone: M2 → M3
Depends on: 1544694
Depends on: 1544697
Depends on: 1545461
Depends on: 1450284
Depends on: 1042642

This is preparation for moving the threadClient to a front. Fronts do not support the
callback style that clients have, they only support promises. As such, this patch migrates all
instances of the threadClient using callback style methods to promises. I have cc'd the debugger
team so that they are aware of this change and it doesn't take them by surprise.

try run : https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=242251058&revision=dbd6cca9619ca0880e920d5b86fc6d6e0c728113

Depends on: 1225492
Depends on: 1492830
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/92678c01ead7
Use promise pattern rather than callback pattern for threadClient methods; r=ochameau
Keywords: leave-open
Type: enhancement → task
Component: Debugger → Framework

This is the first part of the threadClient refactor. It only moves the methods to the new
front. and does some basic fixes.

This is part one of removing threadClient specifics out of the debuggerClient. We were
managing messages from the thread client in a special way -- this was the "Unsolicited Pauses"
object that we had before. This patch updates the threadClient to use Front style events. This
required updating the spec for the threadClient, and several of the methods. What has not been fully
migrated here is the "resumed" event, as this is much more complex. This is taken care of in the
next patch.

The resume case is much more complex than the other events, because we do an
unsafeSynchronize to send an unsolicited pause. In the old system, the resume response would have
been ignored, but that is no longer the case. With the new system, we do not want to send a response
to a resume action if it did not come from the UI. This also update the debugger panel code to
accept a resume.

In order for a front to be available to getFront on a given target, it must be first --
registered on the target scope, and second -- set on the target's targetForm. This makes that update
for both browsing context and worker targets. This works as part of a work around until we can get
the server into better shape.

The webConsoleFront and the threadClient front both used to depend on the debugger-client
to destroy them via registered clients. This is no longer the case, and this code can be deleted

Attachment #9067760 - Attachment description: Bug 1494796 - Removing threadClient specifics from DebuggerClient p2: Special case resume; r=ochameau → Bug 1494796 - Removing threadClient specifics from DebuggerClient [part 2] Special case resume; r=ochameau
Attachment #9067749 - Attachment description: Bug 1494796 - Convert ThreadClient into a Front part 1: transform into class; r=ochameau,jdescottes → Bug 1494796 - Convert ThreadClient into a Front [part 1] transform into class; r=ochameau,jdescottes
Attachment #9067761 - Attachment description: Bug 1494796 - use getFront to retrieve threadClient; r=ochameau → Bug 1494796 - Convert ThreadClient into a Front [part 2] use getFront to retrieve threadClient; r=ochameau

The front destroy method is async, but this is not respected if the client suddenly
disconnects. This causes issues with tests. This change introduces a mechanism for waiting on asyn
fronts to close.

This was a difficult situation. We are not waiting for a server response when setting or
removing breakpoints. The result is that the server has not completed those actions by the time the
test closed, leaving setBreakpoint or removeBreakpoint requests hanging, causing a number of tests
to fail. In order to get around this, I made the panel action "SET_BREAKPOINT" and
"REMOVE_BREAKPOINT" aware of the server response. This involved rewriting the helper methods
clientSetBreakpoint and clientRemoveBreakpoint so that they no longer relied on the dispatch.

It was not possible to use the dispatches to wait, as they had no event exposed to the tests,
especially in cases when we triggered these requests via button presses.

There were a few miscellaneous situations in which the test would fail due to a hanging
request. These tests passed in the past because the old way of using the threadActor did not
identify which requests had been responded to.

this patch fixes a few tests that had miscellanous timeouts

the test browser_net_params_sorted may not have been functioning for some time. While
investigating this, I discovered that we were getting an empty array for actualKeys. Iterating
over this array returned immediately. As a result the test always passed, even though the shape of
the data changed. I updated this code so that it waits for the dom to be populated with the number
of keys expected. I also updated the test to reflect the data the front end has today.

There was an issue where this test was timing out, and due to the way it was written it was
very hard to identify where -- there were many nested promises. I rewrote the test in order to
identify the time out.

this test was failing because the threadClient was posting messages due to being unable to
find the source map. After investigating, I found that the sourcemap was formatted incorrectly. I do
not know if this was intentional. If it wasn't this fix works. If it was, then we need to find a way
to wait on source map failures, and the test name might need to be updated

This test was failing due to the tab closing before a response from the tab was recieved.
It is due to the threadClient closing more quickly. This waits for the last message before closing

this test was failing because it did not wait for the debugger to finish reloading.

we shouldn't have this code, but we do

Alternative fix here is to only update telemetry after the response. I do not know if there
was a product decision here.

this is the weird one. we have a Debugger.executeSoon call for the destruction of the host.
However, before this happened immediately after the test closed. Now, it happens a little later, and
this messes up the data set up. The comment says that we cannot remove this.

Attachment #9067752 - Attachment description: Bug 1494796 - get rid of ThreadClient specifics in DebuggerClient [part 1]; r=ochameau → Bug 1494796 - get rid of ThreadClient specifics in DebuggerClient ; r=ochameau
Attachment #9067760 - Attachment description: Bug 1494796 - Removing threadClient specifics from DebuggerClient [part 2] Special case resume; r=ochameau → Bug 1494796 - Removing threadClient specifics from DebuggerClient Special case resume; r=ochameau
Assignee: nobody → ystartsev
Attachment #9067761 - Attachment description: Bug 1494796 - Convert ThreadClient into a Front [part 2] use getFront to retrieve threadClient; r=ochameau → Bug 1494796 - use getFront to retrieve threadClient; r=ochameau
Attachment #9067749 - Attachment description: Bug 1494796 - Convert ThreadClient into a Front [part 1] transform into class; r=ochameau,jdescottes → Bug 1494796 - Convert ThreadClient into a Front; r=ochameau,jdescottes

The ChromeDebuggerActor extends the methods from threadActor but didn't have an event
emitter. I updated it to function the same as other actors

make sure we have a copy of thread client for old servers

Attachment #9068066 - Attachment is obsolete: true

Introduce a flag to maintain backwards compatibility with old servers

Attachment #9069357 - Attachment is obsolete: true
Attachment #9069357 - Attachment is obsolete: false
Attachment #9067810 - Attachment description: Bug 1494796 - use a polyfill for paintflash button telemetry test; r=jdescottes → Bug 1494796 - wait for paintflash requests to settle
Attachment #9067806 - Attachment is obsolete: true
Whiteboard: dt-fission → dt-fission [devtools-backward-compat]
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c5408cf27728
fix timing issues in debugger tests due to threadClient refactor; r=jlast
https://hg.mozilla.org/integration/autoland/rev/b77955eb4a71
fix timing issues in general tests due to threadClient refactor; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3a9e09d5a4d7
fix netmonitor test to ensure elements exist on the dom before testing; r=Honza
https://hg.mozilla.org/integration/autoland/rev/3c5a0b786ffb
fix timing issue and rewrite object grip test; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/986402a59722
fix blob stylesheet test to not fail retrieving sourcemap; r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d2b8576bf67c
wait in aboutdebugging addons nobg test for tabs before closing; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/818e2521980c
wait for paintflash requests to settle r=jdescottes
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2a034e34fa6
fork threadclient for backwards compatibility r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/b900d41c8ae8
Convert ThreadClient into a Front; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/25e69c21dc2e
get rid of ThreadClient specifics in DebuggerClient ; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/33eec873a43e
get rid of ThreadClient specifics in DebuggerClient, attach/detach methods and events; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1d21a55cae15
get rid of ThreadClient specifics in DebuggerClient, interrupt method; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/35d967de4223
Removing threadClient specifics from DebuggerClient Special case resume; r=jdescottes,jlast
https://hg.mozilla.org/integration/autoland/rev/fe1559f5f1d4
use getFront to retrieve threadClient; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1a4ab8b35a85
maintain backwards compatibility r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0e214d450b35
Delete registerClient functionality; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a14e820311bc
Fix host resize test caused by timing issue; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/591453b88e8b
change test code for reaching into the server; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/c48f00f0df72
update setBreakpoint action on the debugger to be aware of server response; r=jdescottes,jlast
https://hg.mozilla.org/integration/autoland/rev/5db908b26d50
exit gracefully if docShell has been destroyed; r=jdescottes
Keywords: leave-open
Attachment #9067766 - Attachment is obsolete: true
Keywords: leave-open
Pushed by ystartsev@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5ea8582b48c0
fork threadclient for backwards compatibility r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9d24fd51deb4
Convert ThreadClient into a Front; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/eacdf422f586
get rid of ThreadClient specifics in DebuggerClient ; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2a4f4639567a
get rid of ThreadClient specifics in DebuggerClient, attach/detach methods and events; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/3c1c8393c6e8
get rid of ThreadClient specifics in DebuggerClient, interrupt method; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9732099eeb56
Removing threadClient specifics from DebuggerClient Special case resume; r=jdescottes,jlast
https://hg.mozilla.org/integration/autoland/rev/2494fafdeae7
use getFront to retrieve threadClient; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1d3845e33eb9
maintain backwards compatibility r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/01195404b5ea
Delete registerClient functionality; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/caefc0b61c81
Fix host resize test caused by timing issue; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7408af62d6bb
change test code for reaching into the server; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/0013565e760d
update setBreakpoint action on the debugger to be aware of server response; r=jdescottes,jlast
https://hg.mozilla.org/integration/autoland/rev/67445d8e4d05
exit gracefully if docShell has been destroyed; r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/ec5d586ced75
fix verify test r=jdescottes
Regressions: 1559547
Fission Milestone: M3 → M4
Flags: needinfo?(ystartsev)
Keywords: leave-open
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Fission Milestone: M4 → M3

Comment on attachment 9069357 [details]
Bug 1494796 - update chromeDebugger to use event emitter

Revision D33498 was moved to bug 1239008. Setting attachment 9069357 [details] to obsolete.

Attachment #9069357 - Attachment is obsolete: true

Comment on attachment 9068068 [details]
Bug 1494796 - rename context to thread; r=jdescottes

Revision D32849 was moved to bug 1559819. Setting attachment 9068068 [details] to obsolete.

Attachment #9068068 - Attachment is obsolete: true
No longer depends on: 1450284
Regressions: 1484818
Target Milestone: --- → Firefox 69
Blocks: 1566851
Regressions: 1567210
Priority: P2 → P1
Whiteboard: dt-fission [devtools-backward-compat] → dt-fission [devtools-backward-compat] dt-fission-m1
You need to log in before you can comment on or make changes to this bug.