Closed Bug 1566421 Opened 1 year ago Closed 1 year ago

Make webconsole destroy codepath synchronous

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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

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

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 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)

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.

Status: NEW → ASSIGNED
Priority: -- → P1

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!

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.

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.

Keywords: leave-open
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a6c37905df7
Make WebConsoleUI.destroy synchronous. r=nchevobbe
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/532828ce7e3e
Ensure that actions.updateRequest resolves only once the action is processed. r=nchevobbe

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.

Attachment #9078671 - Attachment is obsolete: true
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/150bf969aea0
Make webconsole panel destroy codepath synchronous. r=nchevobbe
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.