1402779 - Use always-alive Promise from panel scripts.

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Alexandre Poirot [:ochameau]]

Work on bug 1387128 highlighted possible races in toolbox shutdown.

During toolbox close, panel documents are destroyed (their iframe is removed from browser.xul). And all the references to DOM Promises from documents are "suspended". (i.e. they never resolve) It happens at some point during document unload.
We can still have references to then, but they won't resolve anymore.

Here is an example on canvas panel.

http://searchfox.org/mozilla-central/source/devtools/client/canvasdebugger/panel.js#70
    return this._destroyer = this.panelWin.shutdownCanvasDebugger().then(() => {
      // Destroy front to ensure packet handler is removed from client
      this.panelWin.gFront.destroy();
      this.emit("destroyed");
    });
`this.panelWin.shutdownCanvasDebugger()` never resolves

http://searchfox.org/mozilla-central/source/devtools/client/canvasdebugger/canvasdebugger.js#114-120
  return Promise.all([
    EventsHandler.destroy(),
    SnapshotsListView.destroy(),
    CallsListView.destroy()
  ]);
canvasdebugger.js is a javascript file loaded in canvas panel document, whereas panel.js is a module.
(Note that all these 'destroy' methods don't return any promise but undefined.
That isn't the issue.)

`return Promise.all([]);` and  `return Promise.resolve();` will both work
`return Promise.all([undefined]);` won't as well as return `new Promise(d => setTimeout(d, 0));`
We just have a timing issue where we end up being called before/after document unload.

You can reproduce this issue by running this:
./mach mochitest devtools/client/canvasdebugger/test/browser_canvas-frontend-record-04.js devtools/client/canvasdebugger/test/browser_profiling-canvas.js
(with the promise refactoring patches applied, where we end up using DOM Promises instead of Promise.jsm)


Now, I haven't audited all the code, but we should ensure either using Promise.jsm or privileged DOM Promises that are never frozen.
Otherwise we are most likely keeping around toolbox shutdown breakages that lead to broken toolbox that are not correctly removed from UI.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1402779 - Force using privileged Promise in panels.

Review commit: https://reviewboard.mozilla.org/r/183612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/183612/

Comment 2

[Alexandre Poirot [:ochameau]]

A bit of context regarding bug 1387128 and Promise.jsm removal.
This patch if going to stay after Promise.jsm removal.
This should be the only require("Promise") we will do in the codebase after the removal.
All other places should use the regular "Promise" symbol available in global scope, which is going to be DOM Promises.

I made that patch independant of bug 1387128 as the toolbox destruction with frozen Promises may already happen in any toolbox using panel Promise. So I was wondering if we could uplift that to FF57?

We should be able to get rid of these require("Promise") if we manage to cleanup toolbox destruction codepath.
I tried, but it is quite challenging (breaks many tests). It would require a quarter goal to get to clean that up...

Comment 3

[Tom Tromey :tromey]

Comment on attachment 8912246 [details]
Bug 1402779 - Force using privileged Promise in panels.

https://reviewboard.mozilla.org/r/183612/#review188876

Thank you for the patch.

If each of these spots truly requires this variant of `Promise`, then there ought to be a comment by each one.  Otherwise, I fear, one of the refactoring waves will try to change this and cause confusion when it doesn't work.  Better, I think, to add a comment now by the requires to make this less confusing in the future.

::: devtools/shared/builtin-modules.js:177
(Diff revision 1)
>  exports.modules = {
>    "Services": Object.create(Services),
>    promise,
> +  // Expose "chrome" Promise, which aren't related to any document
> +  // and so are never frozen, even if the browser loader module which
> +  // pull it is destroyed.

Please mention this bug in the comment.  I think that will be helpful to someone in the future.

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8912246 [details]
Bug 1402779 - Force using privileged Promise in panels.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/183612/diff/1-2/

Comment 5

[Alexandre Poirot [:ochameau]]

(In reply to Tom Tromey :tromey from comment #3)
> If each of these spots truly requires this variant of `Promise`, then there
> ought to be a comment by each one.  Otherwise, I fear, one of the
> refactoring waves will try to change this and cause confusion when it
> doesn't work.  Better, I think, to add a comment now by the requires to make
> this less confusing in the future.

I'm not sure they all end up using Promise during panel's unload.
At least canvasdebugger do.
But this import, in all panels will ensure they won't have this unload issue if they happen to start using Promise during unload.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7fc714dba604
Force using privileged Promise in panels. r=tromey

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7fc714dba604