Closed Bug 1455226 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: General, defect, P2)

61 Branch
defect

Tracking

(firefox-esr52 unaffected, firefox60 unaffected, firefox61+ fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox60 --- unaffected
firefox61 + fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

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.
Regression from Bug 1440321
Blocks: 1440321
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.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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
Attachment #8969356 - Flags: review?(poirot.alex) → review+
Comment on attachment 8969357 [details]
Bug 1455226 - Move browserRequire creation to panel.js;

https://reviewboard.mozilla.org/r/238092/#review243816
Attachment #8969357 - Flags: review?(poirot.alex) → review+
Attachment #8969357 - Attachment is obsolete: true
Thanks for the review! Addressed  the comment and folded the patches. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d16da7bad3e9cd5d9fd938e432f187f75b4710b
Blocks: 1455421
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ff1831623026
Load memory/initializer.js via browserRequire instead of a script tag;r=ochameau
https://hg.mozilla.org/mozilla-central/rev/ff1831623026
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
Flags: in-qa-testsuite+
You need to log in before you can comment on or make changes to this bug.