1455226 - Developer tools in a separate window cannot be reopened after using the Memory tool

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Julian Descottes [:jdescottes]]

STRs:
- open DevTools
- undock to separate window
- select Memory tool
- close DevTools
- try to open DevTools
=> DevTools no longer open

No error log anywhere. Used to work in FF60, so this must be a recent regression.
Marking as P2, but might be P1, we should fix that for FF61.

Comment 1

[Julian Descottes [:jdescottes]]

Regression from Bug 1440321

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1455226 - Load memory/initializer.js via browserRequire instead of a script tag;

"async" methods defined in scripts loaded via a <script> tag seem
to indirectly use the document's Promise, which is not usable after
the DOM of the document has been destroyed.

This is an issue if we invoke this code during the destroy of the
toolbox.

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

Comment 3

[Julian Descottes [:jdescottes]]

Attachment: Bug 1455226 - Move browserRequire creation to panel.js;

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

Comment 4

[Julian Descottes [:jdescottes]]

Tests in devtools/client/memory pass locally, here is a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07bd394f9b26bd4f920cbc365781f655c9ee8226

Alex found the root cause of the issue: if you define async methods in scripts loaded via a <script> tag, they will have the same limitations as document's Promises, described in Bug 1402779. If such an async method is invoked while the document of the toolbox is destroyed, the method will never resolve, which will break the destroy sequence of the toolbox.

The approach here is to stop loading any code via script tags for the memory panel. Once we agree on the basics for the memory panel we should log a follow up to do the same in all DevTools panels.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8969356 [details]
Bug 1455226 - Load memory/initializer.js via browserRequire instead of a script tag;

https://reviewboard.mozilla.org/r/238090/#review243814

Thanks a lot for looking into this, I'm really glad we better understand frozen promises/asyncs!

I have some comment that are easier to address with both patches applied.

::: devtools/client/memory/initializer.js:26
(Diff revision 1)
> -
> -/**
> - * Variables set by `initialize()`
> - */
> -var gStore, gRoot, gApp, gProvider, unsubscribe, isHighlighted;
> -
> +    front: window.gFront,
> +    heapWorker: window.gHeapAnalysesClient
> +  });
> +  window.gProvider = createElement(Provider, { store: window.gStore }, window.gApp);
> +  ReactDOM.render(window.gProvider, window.gRoot);
> +  window.unsubscribe = window.gStore.subscribe(onStateChange);

Most of these variables are only used within this module:
gRoot, gApp, gProvider, isHighlighted, unsubscribe

gStore and gFront are used in many places outside of this module, but could be exposed on panel instead of window. (But you can keep it as-is)

gToolbox can be passed from panel via an argument:
https://searchfox.org/mozilla-central/search?q=gToolbox&case=true&regexp=false&path=memory%2F

Comment 6

[Alexandre Poirot [:ochameau]]

Comment on attachment 8969357 [details]
Bug 1455226 - Move browserRequire creation to panel.js;

https://reviewboard.mozilla.org/r/238092/#review243816

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8969356 [details]
Bug 1455226 - Load memory/initializer.js via browserRequire instead of a script tag;

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

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8969357 [details]
Bug 1455226 - Move browserRequire creation to panel.js;

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

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8969356 [details]
Bug 1455226 - Load memory/initializer.js via browserRequire instead of a script tag;

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

Comment 10

[Julian Descottes [:jdescottes]]

Thanks for the review! Addressed  the comment and folded the patches. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d16da7bad3e9cd5d9fd938e432f187f75b4710b

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8969356 [details]
Bug 1455226 - Load memory/initializer.js via browserRequire instead of a script tag;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238090/diff/3-4/

Comment 12

[Julian Descottes [:jdescottes]]

Comment on attachment 8969356 [details]
Bug 1455226 - Load memory/initializer.js via browserRequire instead of a script tag;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238090/diff/4-5/

Comment 13

[Julian Descottes [:jdescottes]]

try for last version: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aabd4351141ce1eb7e2a1e8b11eba00813431124

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ff1831623026
Load memory/initializer.js via browserRequire instead of a script tag;r=ochameau

Comment 15

[Raul Gurzau (:RaulG)]

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