1485664 - Convert WebConsoleClient to protocol.js front

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

WebConsoleClient is the client class to connect to the console 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 console fronts.

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

Comment 1

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - migrate client to the shape of a front;

Comment 2

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - convert old style webconsole front to new class front; r=ochameau

Depends on D14392

Comment 3

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - migrate evaluationResult event; r=ochameau

Depends on D14393

Comment 4

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - use networkEventFront; r=ochameau

Depends on D14394

Comment 5

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - remove echoed events; r=ochameau

Depends on D14395

Comment 6

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - remove unused methods; r=ochameau

Depends on D14396

Comment 7

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - make evaluateJS calls use promise pattern rather than callbacks; r=ochameau

Depends on D14397

Comment 8

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - use promise pattern for getCachedMessage rather than callbacks; r=ochameau

Depends on D14398

Comment 9

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - use promise pattern for other webconsole methods rather than callbacks; r=ochameau

Depends on D14399

Comment 10

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - use networkEvent front to manage networkEvent methods instead of console; r=ochameau

Depends on D14400

Comment 11

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - use protocol js methods instead of hard coding it in the console front; r=ochameau

Depends on D14401

Comment 12

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - convert webconsoleClient to front; r=ochameau

initial move of the webconsoleClient to a front. Events do not work yet, but methods do.

Comment 13

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - remove echoed events and unused events; r=ochameau

Comment 14

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - adjust events to front events; r=ochameau

this patch introduces events for the webconsoleFront -- not all of them are being listened
to by their subscribers yet, but this is the bulk of the effort to move events to Protocol.js

Comment 15

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - update har collector to use webconsole events; r=ochameau

Comment 16

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - adjust webconsole-connection-proxy to listen to webconsole front events, and fix tests; r=ochameau

this migrates the proxy to the new webconsole front events. This resulted in a number of
test failures once the migration was finished, and those were fixed

Comment 17

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - use protocol js methods instead of hard coding it in the console front; r=ochameau

Just updating the stubs for tests

Comment 18

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - remove from field from jsterm tests; r=ochameau

Comment 19

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - Change "attachConsole" in the debuggerClient so that it is compatible with a front; r=ochameau

Comment 20

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - move webconsole client to front directory; r=ochameau

Comment 21

[Yulia Startsev [:yulia]]

We have an unusual intermittent: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8533e14e032856ab5308f17b4a0fe2e5297fa07e&selectedJob=226504889

The issue is in d6, devtools/client/webconsole/test/mochitest/browser_webconsole_eval_in_debugger_stackframe.js

it can be reproduced by applying this patch queue, and then running the above test in a debug build with the following

./mach mochitest devtools/client/webconsole/test/mochitest/browser_webconsole_eval_in_debugger_stackframe.js --verify --headless

I haven't tried it with other machines, but it is reproducible on mac in a debug build. It looks like it happens frequently on windows debug builds as well.

I have traced the issue to here: https://searchfox.org/mozilla-central/source/devtools/server/actors/webconsole/eval-with-debugger.js#132

The string we pass in is : foo = 'abba'; foo3 = 'bug783499'; foo + foo3
The expected result is : “abbabug783499”
Instead we are seeing: "fooFirstCallfoo3FirstCall"

Comment 22

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - remove unused inspectObjectProperties; r=ochameau

Comment 23

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - update console stub generators; r=ochameau

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ab1ab7f3bd6
remove unused inspectObjectProperties; r=ochameau

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/1ab1ab7f3bd6

Comment 26

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - Fix failing test due to race; r=ochameau

Comment 27

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - remove attachConsole from the debugger client; r=ochameau

Comment 28

[Yulia Startsev [:yulia]]

Attachment: Bug 1485664 - guard webconsole destroy method from being called when there is no client; r=ochameau

Comment 29

[Yulia Startsev [:yulia]]

Some more information about the destroy path problem:

before, we were running the code to remove clients (webconsole client and threadclient) first on the debugger client
https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/devtools/shared/client/debugger-client.js#234-252

then we would do the cleanup if there was a problem or a sudden disconnection
https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/devtools/shared/client/debugger-client.js#812-831

now, the destroy path is a little different
we inform the target to start destroying, but we don’t have a mechanism for waiting for it to destroy. The webconsole front is now treated as any other front.

We don't wait for the webconsole to close, instead we inform the targets that the client has closed: https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/devtools/shared/client/debugger-client.js#808
via this listener: https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/devtools/shared/fronts/targets/target-mixin.js#391

and the target starts destroying fronts, asynchronously (since the fronts are loaded async) : https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/devtools/shared/fronts/targets/target-mixin.js#475-478

so, simultaneously, we start destroying things in the pool, because there is no way to wait for the fronts: https://searchfox.org/mozilla-central/rev/ee40541496d3ad738097eebadaf4965ca1343b7a/devtools/shared/client/debugger-client.js#830

Comment 30

[Yulia Startsev [:yulia]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=229182780&revision=74ff87da53e0702158db6a2c1957edc2c298a7f4

Comment 31

[Yulia Startsev [:yulia]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ef5355b1e4d97dc58a4babf9edfe122b4b2d3e8&selectedJob=229401685

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b1392e0b33f7
convert webconsoleClient to front; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/feb85ff28604
remove echoed events and unused events; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/c1b5af58756f
adjust events to front events; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/b7535075b575
update har collector to use webconsole events; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/f3acf0651a2c
adjust webconsole-connection-proxy to listen to webconsole front events, and fix r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a6be93699bf6
use protocol js methods instead of hard coding it in the console front; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a895bb6fe591
update console stub generators; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/fac93d4a57ba
remove from field from jsterm tests; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/4ce9378a9784
Change "attachConsole" in the debuggerClient so that it is compatible with a front; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/1cbd54c219ca
move webconsole client to front directory; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/9544d90c6e2d
Fix failing test due to race; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/fa0e2e873b16
remove attachConsole from the debugger client; r=ochameau
https://hg.mozilla.org/integration/autoland/rev/6fc5d53dd6d3
guard webconsole destroy method from being called when there is no client; r=ochameau

Comment 33

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/b1392e0b33f7
https://hg.mozilla.org/mozilla-central/rev/feb85ff28604
https://hg.mozilla.org/mozilla-central/rev/c1b5af58756f
https://hg.mozilla.org/mozilla-central/rev/b7535075b575
https://hg.mozilla.org/mozilla-central/rev/f3acf0651a2c
https://hg.mozilla.org/mozilla-central/rev/a6be93699bf6
https://hg.mozilla.org/mozilla-central/rev/a895bb6fe591
https://hg.mozilla.org/mozilla-central/rev/fac93d4a57ba
https://hg.mozilla.org/mozilla-central/rev/4ce9378a9784
https://hg.mozilla.org/mozilla-central/rev/1cbd54c219ca
https://hg.mozilla.org/mozilla-central/rev/9544d90c6e2d
https://hg.mozilla.org/mozilla-central/rev/fa0e2e873b16
https://hg.mozilla.org/mozilla-central/rev/6fc5d53dd6d3

Comment 34

[Brian Hackett [Laid off!]]

Attachment: mochitest patch

Hi, I think these patches broke the ability to evaluate expressions in workers. The debugger uses the console's evaluateJSAsync request for this. Before these patches, each worker was associated with its own console actor which could evaluate expressions in that worker's thread, but after the patch in https://phabricator.services.mozilla.com/D20150 each worker is associated with the same console actor which the main thread uses.

Unfortunately there was no test for this functionality (mea culpa, windowless workers are pretty new and the tests are in poor shape). I've attached a patch which updates a mochitest to exercise this. This test passes before these patches, and fails afterwards.

I'm not sure how to go about fixing this. Is there still a separate console actor associated with each thread? From the changes in D20150 it seems like there isn't, but it's hard for me to follow the changes that are happening in this bug.

Comment 35

[Brian Hackett [Laid off!]]

Sorry for the clutter here, I just filed a separate bug for the worker evaluation problem.