1602727 - target-mixin destroy can be called several times synchronously

Status: RESOLVED FIXED

(DevTools :: General, task, P3)

Description

[Julian Descottes [:jdescottes]]

In target-mixin.js we use the following pattern to prevent destroy from running twice:

destroy() {
  if (this._destroyer) {
    return this._destroyer;
  }

  this._destroyer = (async() => {
    doSomething();
    if (someBool) {
      await doSomethingAsync();
    }
  })();
}

In theory this._destroyer 's value should be set to a promise. But to do that the right handside of the assignment needs to yield first. So if we call destroy() in doSomething(), then this._destroyer will still be undefined, and we will call destroy twice.

I thought using a Promise could help, but no, same issue.

this._destroyer = new Promise(async r => {
  doSomething();
  if (someBool) {
    await doSomethingAsync();
  }
  r();
});

So our pattern is faulty and I don't have a solution yet. I'll attach a snippet to test this in the console.

Comment 1

[Julian Descottes [:jdescottes]]

var Test = class {
  callDestroyOnce() {
    // this is just to avoid too much recursion, the point is just to call destroy once, synchronously
    if (!this._alreadyCalledDestroy) {
      this._alreadyCalledDestroy = true;
      this.destroy();
    }
  }

  async doSomethingAsync() {
    await 0;
  }

  destroy() {
    if (this._destroyer) {
      return this._destroyer;
    }

    console.log("SHOULD ONLY LOG ONCE");

    this._destroyer = (async() => {
      this.callDestroyOnce();
      await this.doSomethingAsync();
    })();
  }
}

var test = new Test();
test.destroy();

This is a basic test case you can run from the console.

Comment 2

[Julian Descottes [:jdescottes]]

A hack that works is to add anything async (eg await 0;) as the first thing done in the _destroyer

    this._destroyer = (async() => {
      await 0;
      this.callDestroyOnce();
      await this.doSomethingAsync();
    })();

Comment 3

[Julian Descottes [:jdescottes]]

Looks hacky, but I think conceptually that's what we need here.

    let destroyerResolve;
    this._destroyer = new Promise(r => destroyerResolve = r);

    (async () => {
      this.callDestroyOnce();
      await this.doSomethingAsync();
      destroyerResolve();
    })();

Comment 4

[Julian Descottes [:jdescottes]]

Quickly reviewed our destroy methods, I think only the toolbox is using a similar pattern, and therefore exposed to the same issue: https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/devtools/client/framework/toolbox.js#3554-3564

Note that for the sake of "not calling the destroy" twice, we could just use boolean flags and bail out.
But here we satisfy the extra requirement that all callers should "resolve" at the same time. If destroy is async all callers should resolve only when destroy has completed. With boolean flags, typically only the first destroy caller could wait for the destroy completion, all subsequent callers would resolve immediatly.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1602727 - Fix DevTools destroyer patterns

Just to illustrate a potential fix

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d21f0bb74a11
Fix DevTools destroyer patterns r=nchevobbe

Comment 7

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/d21f0bb74a11