1529621 - Panel destroy() should be synchronous

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Julian Descottes [:jdescottes]]

One of the items for the toolbox destruction cleanup will be to make sure that all panels destroy methods are synchronous.

To do:

• review which panels have really async destroy methods (today we know about webconsole and old performance panel)
• make those destroy synchronous
• update toolbox.destroy() to call panels' destroy synchronously (and maybe throw/log an exception when we detect that a panel destroy returns a promise)

Comment 1

[Alexandre Poirot [:ochameau]]

We discussed about that with Yulia and it appears that in a couple of places, the destroy is async because of the initialization being async.

The web console destroy is async, ultimately because this disconnect method being async here:
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/webconsole/webconsole-connection-proxy.js#408-411
And it is async because we have to wait for the initialization of the proxy:
https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/webconsole/webconsole-connection-proxy.js#130

Regarding the perf panel, we may be using async everywhere in the panel for no reason. A careful review will confirm.
But at the front level, we had to deal with the async instantiation of its front.
We worked around this in a particular way:
https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/devtools/client/framework/toolbox.js#590-595
By waiting for the front to initialize only in tests and block the toolbox opening on this.

Note that there is also similar pattern in the toolbox, for the inspector front:
https://searchfox.org/mozilla-central/rev/4587d146681b16ff9878a6fdcba53b04f76abe1d/devtools/client/framework/toolbox.js#2847-2849
But instead we wait for initialization from the destruction.

Comment 2

[Julian Descottes [:jdescottes]]

The performance panel is in the same situation, all the real async destroy are waiting for the initialization of "graph" objects to be completed. Typical example: https://searchfox.org/mozilla-central/rev/b36e97fc776635655e84f2048ff59f38fa8a4626/devtools/client/shared/widgets/FlameGraph.js#236-237

Re the pattern used for performanceFrontConnection, this seems like a very dangerous approach to me. We are no longer testing the initialization code we are shipping to users. We had a similar if(testing) { await markupView.init } in the inspector a few month ago to "improve performances". I didn't think this was ok at the time and I share the same feeling about performanceFrontConnection

Comment 3

[Alexandre Poirot [:ochameau]]

Now that the old perf panel has been removed and inspector has been migrated to ResourceCommand,
we might no longer have any pending RDP request, nor any async panel destroy method.

Panels might still emit one-way RDP request against the watcher actor, to call unwatchResources.
But these requests are meant to be synchronous from the frontend point of view.
They might still introduce some noise or cause some race conditions, but we should be able to make all panel destroy method become synchronous.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1529621 - [devtools] Make all panel's destroy method be synchronous.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2861aa332722
[devtools] Make all panel's destroy method be synchronous. r=jdescottes

Comment 6

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/2861aa332722