Open Bug 1528976 Opened 5 years ago Updated 2 years ago

Cleanup toolbox's destruction codepath

Categories

(DevTools :: Framework, enhancement, P3)

Product:
DevTools
Component:
Framework
Type:
enhancement

Tracking

(Not tracked)

People

(Reporter: ochameau, Unassigned)

References

(Depends on 3 open bugs, Blocks 1 open bug)

Details

When the toolbox closes, various codebases start firing cleanup methods. Unfortunately, theses cleanup methods do not necessarily run in a coordinated way. By that I mean that there is no clear expectation in the order in which these methods are being called.

Main entry point

The main entry point is Toolbox.destroy:
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/devtools/client/framework/toolbox.js#2866

Duplicated calls to destroy

Like many of the other cleanup methods, we have to guard against infinite loops and ensure destroying only once, while waiting for full destruction before resolving the non-first calls:

    // If several things call destroy then we give them all the same
    // destruction promise so we're sure to destroy only once
    if (this._destroyer)
      return this._destroyer;
    return this._destroyer = this._destroyToolbox();

There is two reasons behind this defensive code:

  • Cleanup methods are asynchronous.
    There is various reasons and we should investigate all the async ones and ensure they are really justified. (Some are probably not justified)
  • We have some kind of spaghetti code.
    To be extra safe, we trigger the destruction methods out of many events and often end up calling each other cleanup methods. We should probably how the various classes are organized between each others (Toolbox vs Target vs Client) and may be organize them as a Tree rather than a Graph. (One class should probably be owning/managing the others, thus limiting the cross calls to cleanup methods)

Races with initialization

But it get even worse, when we have to ensure waiting for full initialization before proceeding with the destruction:

    // allow the console to finish attaching if it started.
    if (this.isAttached) {
      await this.isAttached;
    }

      // Ensure that the inspector isn't still being initiated, otherwise race conditions
      // in the initialization process can throw errors.
      await this._initInspector;

We wait for full initialization before cleaning up to ensure that we correctly unregister the listeners that are set during the initialization.

Toolbox and panel iframes removed from the DOM

Another very important detail is about one of the typical destruction codepath:
What happens when a user closes a tab with an opened toolbox?
The iframe used for loading the toolbox is a child of <xul:notificationbox>, which includes the tab and the toolbox's iframe.
When the tab is closed, the notificationbox is removed from the DOM.
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/browser/base/content/tabbrowser.js#3189
Nothing is called from the Firefox frontend codebase to cleanup the DevTools before removing the iframe. What happen is that there is a tab closing animation, and when the animation ends, the <xul:notificationbox> is removed from the DOM. Up to that point, no code in DevTools is ran.
It is only when the toolbox's iframe is removed from the DOM that it's document get the "unload" event fired and then we call Toolbox.destroy():
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/devtools/client/framework/toolbox.js#960

  this.win.addEventListener("unload", this.destroy);

Luckily, this code, in toolbox.js is loaded via the DevTools module loader and so can run code indefinitely. But if it was a <script> tag of the toolbox document, it would only be able to run synchronous cleanup call. Like what a web page can execute on "unload".

Zombie promises

Unfortunately, it is even more complicated because of the Zombie Promises.
While toolbox.js is loaded using the DevTools module loader, some other panel scripts aren't.
Like inspector.js:
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/devtools/client/inspector/index.xhtml#42

<script type="application/javascript" src="resource://devtools/client/inspector/inspector.js" defer="true"></script>

The issue is that once the panel iframe is removed from the DOM, the "unload" event will fire and one last even loop will fire. Then any script from the panel's document will be released. A side effect of that is that, privileged code, like modules from the DevTools loader may keep some objects of the panel document alive, like promises. But these promises will be frozen. They will be the "Zombie promises". You can still access them, but even if you resolve them via some privileged code, they won't resolve. Their resolve/reject handlers won't be called. That's because their host document has been released.

In order to mitigate that, we are using special promises, that aren't the panel's document's ones:
https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/devtools/client/inspector/inspector.js#20-22

// Use privileged promise in panel documents to prevent having them to freeze
// during toolbox destruction. See bug 1402779.
const Promise = require("Promise");

And we have to be careful to never involve async functions in the destruction codepath. But that, only from JS files that are loaded as <script> tag from a panel/toolbox document.

In order to mitigate that, we planned to standardize all panels in order to use the BrowserLoader and ban any usage of <script> tag. (bug 1455421) But unfortunately, we hit a regression when moving from <script> tag to BrowserLoader. We ended up pausing this project, but this regression was related to compartments. So we may be able to resume this work once bug 1517210 lands!

Typical actions done in cleanup methods

      this.webconsolePanel.removeEventListener("resize",
        this._saveSplitConsoleHeight);

    this.off("select", this._onToolSelected);
    this.off("host-changed", this._refreshHostTitle);

    gDevTools.off("tool-registered", this._toolRegistered);
    gDevTools.off("tool-unregistered", this._toolUnregistered);

    Services.prefs.removeObserver("devtools.cache.disabled", this._applyCacheSettings);
    Services.prefs.removeObserver("devtools.serviceWorkers.testing.enabled",
                                  this._applyServiceWorkersTestingSettings);

    this.webConsoleClient.clearNetworkRequests();

https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/devtools/client/webconsole/webconsole.js#327

          await this.target.focus();

    this.telemetry.toolClosed(this.currentToolId, this.sessionId, this);

  this._saveSplitConsoleHeight();
  _saveSplitConsoleHeight: function() {
    Services.prefs.setIntPref(SPLITCONSOLE_HEIGHT_PREF,
      this.webconsolePanel.height);
  },

win.location.replace("about:blank");

            win.windowUtils.garbageCollect();

    this._markupBox.style.visibility = "hidden";

https://searchfox.org/mozilla-central/rev/05d4b6962a571585bd679d2bbb0df0a5fb4e4eff/devtools/client/webconsole/components/JSTerm.js#1524-1526

      // We do this because it's much faster than letting React handle the ConsoleOutput
      // unmounting.
      this.webConsoleUI.outputNode.innerHTML = "";
  • I may have missed some other action, feel free to tell me what I missed in order to contribute to this list!

Call for action

  • Remove all asynchronous destruction code

    • First, we still do some request to actors when the toolbox closes, but there is absolutely to guarantee that the request will succeed. Instead, the actor should execute this cleanup/action in its destroy method.
    • Then, trying to execute cleanup code on destruction is something web pages can't do. The only thing a Web page can do is execute one synchronous piece of code on "unload" event. I think we should stick to that and not involve any Promise, nor any async function in panel's destruction.

  • In parallel, we should remove any non-useful destruction

    • Removing DOM listeners shouldn't be useful as the document is going to be removed and all DOM listeners should be unregistered anyway
    • Removing front listeners is useless as they are all removed when the front is destroyed, which should happen as we destroy the Target, which ends up destroying all the target-scoped actors (inspector, console, ...), themselves should destroy all the other child actors as well.

  • Review carefully the typical sources of leaks

    • The listeners made to gDevTools, Services.obs, Services.prefs, XPCOM, ... are the typical sources of leaks. And also the listeners of DebuggerClient, which is being kept alive in WebIDE. (This is very different from Front and listeners put on actors clients (threadClient.addListener() versus DebuggerClient.addListener()). These listeners are the one we have to be extremelly cautious and ensure removing them. That's because they are not part of the toolbox or panels. These objects are always alive and will keep your listeners as well as their scoped (and so many objects) alive.
    • We may identify that we are only register listener to a couple of chrome API and have a wrapper in-between which would ensure releasing everything on toolbox destruction?
Severity: normal → enhancement
Priority: -- → P3
Depends on: 1529621
Depends on: 1530299
Depends on: 1565977
Depends on: 1568150
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.