1566421 - Make webconsole destroy codepath synchronous

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

See bug 1529621. This bug is dedicated to the Web Console and aims at getting rid of async on this particular method and all its dependencies:
https://searchfox.org/mozilla-central/rev/07f7390618692fa4f2a674a96b9b677df3a13450/devtools/client/webconsole/panel.js#103-116

Comment 1

[Alexandre Poirot [:ochameau]]

It doesn't look too bad.
But it is interesting to see that removing the one RDP request is enough to trigger all the shutdown races we have a tests.
All failures are about pending requests from tests which are not waited correctly.

Here is the last try push I had:
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=256808656&revision=3e2ae22341ea5b81f6e8df80098bd231b1c07e65

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1566421 - Do focus the content window from the actor when the toolbox closes.

No RDP request should be done when the toolbox closes as there no guarantee that
the request will complete. Instead, such cleanup should be done by the actors.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1566421 - Make webconsole panel destroy codepath synchronous.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1566421 - Make WebConsoleUI.destroy synchronous.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 9078671 [details]
Bug 1566421 - Do focus the content window from the actor when the toolbox closes.

Well. Removing this focus request isn't trivial.

I tried two things:

• Automatically do it from the webconsole actor, on destroy, only for tabs (i.e. ignore the browser toolbox, workers, ...)
  This makes one test to fail devtools/shared/webconsole/test/test_console_serviceworker_cached.html in a very unexpected way!
  We open a first tab, instantiate a console actor, close the connection, open a new tab and reinstantiate a new console actor for the new tab.
  It happens that when we try to instantiate the second console actor, the first tab is being selected because of the focus call.
  This is a race where the call to focus is asynchronous and happen after we requested to open a second tab.
  https://phabricator.services.mozilla.com/D38309?vs=on&id=133130#toc
  (I get failure on this service worker test)

We could somehow address that by ensuring that we try to reset the focus only when a toolbox is involved.
(There is no toolbox involved in this test)
https://phabricator.services.mozilla.com/D38309?vs=on&id=133829#toc
(I'm waiting for try results for this one)

• Use Target.reconfigure to very precisely tell when to restore the focus.
  The current behavior isn't 100% perfect. We force focusing the content page no matter what if the console actor is created (i.e. always).
  We could listen for focus/blur on JSTerm to know if we have the focus in the console and only try to restore it on the content if we were focused.
  But this solution creates various pending requests in tests, as we quite often focus the console input and aren't waiting for its now async completion...
  https://phabricator.services.mozilla.com/D38309?vs=on&id=133369#toc
  (I get random failure because of pending reconfigure request)

Comment 6

[Alexandre Poirot [:ochameau]]

Getting closer:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7eb6bb82a55691d7be13fce1347f4c4234887af

There is a surprising impact on DOM panel tests. It looks like one change of this queue make the DOM panel initialization faster, leading to race in the panel's tests!
Then, it looks like there is at least one additional failure in a console test testing network request inspection.

Comment 7

[Alexandre Poirot [:ochameau]]

I might have a green try this time:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b8240272caeb24080471b82ebd92f705ff23ea9

This is really not trivial test failures, but it helped highlighting tests not waiting correctly for many async operations!

Comment 8

[Alexandre Poirot [:ochameau]]

Hum. It looks like when I fix tests, some new start failing. So it isn't clear when I will be done here... The new failure are still around the netmonitor.

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1566421 - Ensure that actions.updateRequest resolves only once the action is processed.

The code callng action.updateRequest, in FirefoxDataProvider, expects the updateRequest action
to be processed once the returned promise is resolved. Otherwise it may spawn
duplicated requestData requests.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a6c37905df7
Make WebConsoleUI.destroy synchronous. r=nchevobbe

Comment 11

[Alexandru Michis [:malexandru]]

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/532828ce7e3e
Ensure that actions.updateRequest resolves only once the action is processed. r=nchevobbe

Comment 13

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/532828ce7e3e

Comment 14

[Phabricator Automation]

Comment on attachment 9078671 [details]
Bug 1566421 - Do focus the content window from the actor when the toolbox closes.

Revision D38309 was moved to bug 1510690. Setting attachment 9078671 [details] to obsolete.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/150bf969aea0
Make webconsole panel destroy codepath synchronous. r=nchevobbe

Comment 16

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/150bf969aea0