Closed
Bug 1387128
Opened 7 years ago
Closed 3 years ago
[META] Drop all usages of promise = require(promise)
Categories
(DevTools :: General, task, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: ochameau, Assigned: ochameau)
References
Details
(Keywords: meta)
Attachments
(6 files, 4 obsolete files)
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jryans
:
review+
|
Details |
This bug isn't 1384527, it is smaller.
If will focus on the very common pattern where we import `promise` symbol
and use it like this:
new promise(...)
promise.(resolve|reject|all|race)(...)
We should be able to automatically replace all that.
This bug depends on bug 1387123 in order to stop using promise.defer which is the other possible usage of promise API.
So once bug 1387123 is done and we replaced all "promise" with "Promise",
we will be able to drop the `promise = require("promise")` lines.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
This step may be the hardest. I may have to do it per folder to chase this failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=121981423&repo=try&lineNumber=2190
[task 2017-08-09T13:40:05.438745Z] 13:40:05 INFO - GECKO(1136) | MEMORY STAT | vsize 2260MB | residentFast 308MB | heapAllocated 128MB
[task 2017-08-09T13:40:05.440820Z] 13:40:05 INFO - TEST-OK | devtools/client/canvasdebugger/test/browser_profiling-canvas.js | took 516ms
[task 2017-08-09T13:40:05.472402Z] 13:40:05 INFO - checking window state
[task 2017-08-09T13:40:05.556910Z] 13:40:05 INFO - GECKO(1136) | console.log: TELEMETRY PING: {"client_id":"7b068e90-79fa-4040-bcab-d1e16d115aa5","addon_version":"0.0.0","locale":"en-US","session_id":"{c634b5bf-325a-4c1b-8e2d-2db6cfa023ec}","page":"about:newtab","action":"activity_stream_session","perf":{"load_trigger_type":"unexpected","topsites_first_painted_ts":1502285964738.7139}}
[task 2017-08-09T13:40:15.517016Z] 13:40:15 INFO - GECKO(1136) | WARNING: At least one completion condition is taking too long to complete. Conditions: [{"name":"DevTools: Wait until toolbox is destroyed","state":"(none)","filename":"resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js","lineNumber":2522,"stack":["resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:leakCheckObserver:2522","chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest</<:663","chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795"]}] Barrier: ShutdownLeaks: Wait for cleanup to be finished before checking for leaks
[task 2017-08-09T13:41:06.516955Z] 13:41:06 INFO - GECKO(1136) | FATAL ERROR: AsyncShutdown timeout in ShutdownLeaks: Wait for cleanup to be finished before checking for leaks Conditions: [{"name":"DevTools: Wait until toolbox is destroyed","state":"(none)","filename":"resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js","lineNumber":2522,"stack":["resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js:leakCheckObserver:2522","chrome://mochikit/content/browser-test.js:Tester.prototype.nextTest</<:663","chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795"]}] At least one completion condition failed to complete within a reasonable amount of time. Causing a crash to ensure that we do not leave the user with an unresponsive process draining resources.
[task 2017-08-09T13:41:06.519379Z] 13:41:06 INFO - GECKO(1136) | [Parent 1136] ###!!! ABORT: file resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js, line 2522
[task 2017-08-09T13:41:06.520409Z] 13:41:06 INFO - GECKO(1136) | [Parent 1136] ###!!! ABORT: file resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/framework/toolbox.js, line 2522
[
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
Ok, so It looks like the failures come from client as pushing all changes made to shared and server don't make everything fail:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c6f7bfcf9ba064181531531a0b0e944d7acf8446&filter-searchStr=dt&selectedJob=122020424
There is just 3 tests related to worker failing.
But this isn't much easier as worker tests fail only because of this line change:
http://searchfox.org/mozilla-central/source/devtools/server/actors/utils/TabSources.js#400
if (!this._useSourceMaps) {
>> return resolve(null); <<
If you start using DOM promise here, by doing:
return Promise.resolve(null);
These tests fail....
It looks like the failing stack is the following:
GECKO(5740) | FetchSourceMap(fetchSourceMap@resource://devtools/server/actors/utils/TabSources.js:400:28
GECKO(5740) | getOriginalLocation@resource://devtools/server/actors/utils/TabSources.js:611:12
GECKO(5740) | hit@resource://devtools/server/actors/breakpoint.js:145:7
GECKO(5740) | f@http://example.com/browser/devtools/client/debugger/test/mochitest/code_WorkerActor.attachThread-worker.js:5:7
GECKO(5740) | self.onmessage@http://example.com/browser/devtools/client/debugger/test/mochitest/code_WorkerActor.attachThread-worker.js:11:5
GECKO(5740) | EventHandlerNonNull*@http://example.com/browser/devtools/client/debugger/test/mochitest/code_WorkerActor.attachThread-worker.js:9:1
Unfortunately, it seems to be related to unsafeSynchronize being broken against DOM Promise.
The promise returned by FetchSourceMap ends up being passed to unsafeSynchronize and it never release the event loop. p.then's callback is never called here:
http://searchfox.org/mozilla-central/source/devtools/server/actors/script.js#1069-1072
Tom, by any chance, do you happen to know how nsIJSInspector.enterNestedEventLoop plays against DOM Promises?
http://searchfox.org/mozilla-central/source/devtools/platform/nsIJSInspector.idl#41
Assignee | ||
Comment 10•7 years ago
|
||
It may be worker specific as only worker test fail.
The following reduced test script works fine from the parent process/main thread:
var p = new Promise(done => setTimeout(done, 3000));
p.then(() => {
inspector.exitNestedEventLoop(this);
});
inspector.enterNestedEventLoop(this);
p.then callback is correctly fired, and enterNestedEventLoop eventually ends after 3s.
I'm wondering if that call to Promise code right after enterDebuggerEventLoop indicates something...
http://searchfox.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#6157-6158
// Flush the promise queue.
Promise::PerformWorkerDebuggerMicroTaskCheckpoint();
Also, this code looks very related to some special setup for workers:
http://searchfox.org/mozilla-central/source/dom/workers/RuntimeService.cpp#1208-1218
I imagine worker's enter-event-loop helper freeze worker's Promise no matter if that's actual worker modules or debugger server ones...
Assignee | ||
Comment 11•7 years ago
|
||
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8895314 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8895315 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8895316 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8895317 -
Attachment is obsolete: true
Assignee | ||
Comment 13•7 years ago
|
||
Comment on attachment 8895313 [details]
Bug 1387128 - Replace all lower case usages with capital P.
Tom, I don't know this code at all, but it looks like there is something missing to keep running Promise from debugger sandboxes.
I got the test to pass with DOM Promise and this additional patch.
Attachment #8895313 -
Flags: feedback?(ttromey)
Assignee | ||
Comment 14•7 years ago
|
||
The other kind of errors, from comment 6, aren't much more trivial.
What happens is that we now instanciate DOM Promises from Panel documents.
But during the process of toolbox close, panel documents are destroyed (removed from the DOM, document unloaded or something). And the references to DOM Promises from documents are suspended promises.
At some point during document unload, all document promises are destroyed or at least suspended.
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. We just have a timing 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));
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)
Assignee | ||
Comment 15•7 years ago
|
||
About unload breaking document's promises.
That really isn't trivial. I thought we could tweak toolbo destruction code to wait for the Promises to be resolved before removing panel iframe, but that's not possible.
When you close a Tab, toolbox iframe gets automatically removed from the DOM as that's in the notification box, which is per tab. It means that we can't control when the toolbox document get destroyed, so nor can we control when panel documents get destroyed.
Comment 16•7 years ago
|
||
Comment on attachment 8895313 [details]
Bug 1387128 - Replace all lower case usages with capital P.
We discussed the patch a bit on vidyo. I don't know much about this area, so I think somebody else ought to look at it.
Attachment #8895313 -
Flags: feedback?(ttromey)
Updated•7 years ago
|
Priority: -- → P3
Assignee | ||
Comment 17•7 years ago
|
||
So about destruction codepath of panels and toolbox, ideally we would simplify destruction and do not wait for panel destruction during toolbox destruction.
I commented panel.destroy() call from toolbox.destroy and try is 99% orange:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d764e18f4c6c1e8488f9f18adbb8d26b96c3a78a
It would be interesting to know what cleanup we are expecting to be done during tests.
We should not have to destroy much things on toolbox close. Mostly destroy the toolbox iframe and disconnect the client connection.
Assignee | ||
Comment 18•7 years ago
|
||
It looks like some of these failures are related to highlighter utils destructor before panel documents are destroyed.
So we should wait for panel unload before cleaning up things from toolbox.js:
http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#2479-2480
// Destroying the walker and inspector fronts
outstanding.push(this.destroyInspector());
Let's see what try says:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3186a961edbcac65900df55564b063d39f2376a4
Assignee | ||
Comment 19•7 years ago
|
||
About promises and unload I found some discussion around the spec:
https://github.com/w3ctag/promises-guide/issues/44
And I'm wondering if the current behavior that freezes the promises has been introduced in bug 1058695.
Updated•7 years ago
|
status-firefox57:
--- → fix-optional
Updated•6 years ago
|
Product: Firefox → DevTools
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•6 years ago
|
||
I was rebasing this work to latest m-c, to see which panel was failing during toolbox closing, but one intermediate try results was green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa7d40e0b9b958c07b181b094ca9b3f71a685fc5&selectedJob=183952485
Issues with frozen promises on toolbox destruction may be intermittent or OS specific or be gone.
Let's see with a more polished patch and multi platform try what the color is:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=115b517d2e5c960b1b53acece5f93783e7ed2437
Assignee | ||
Comment 27•6 years ago
|
||
Previous try push failed before of other patches in the queue.
I got a green try on linux:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bac4dfcc266eaf30852ef671305f8794c745b271
I'll try again on other platforms. This is surprising to not see issues with canvas editor anymore.
May be bug 1402779 worked around this issue.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8986438 [details]
Bug 1387128 - Manual test fix.
https://reviewboard.mozilla.org/r/251804/#review258272
Attachment #8986438 -
Flags: review?(jryans) → review+
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8986437 [details]
Bug 1387128 - Manually fix the couple of custom promise import and remove promise pseudo module.
https://reviewboard.mozilla.org/r/251802/#review258274
::: devtools/shared/tests/unit/test_invisible_loader.js
(Diff revision 2)
> dbg.addDebuggee(sandbox);
> Assert.ok(true);
> } catch (e) {
> do_throw("debugger could not add visible value");
> }
> -
I think you should leave this test as-is until we have completely removed Promise.jsm from all of DevTools, which I assume happens later in bug 1384527?
Attachment #8986437 -
Flags: review?(jryans) → review+
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8986436 [details]
Bug 1387128 - Also remove the lazy require to promise.
https://reviewboard.mozilla.org/r/251800/#review258276
Attachment #8986436 -
Flags: review?(jryans) → review+
Comment 37•6 years ago
|
||
mozreview-review |
Comment on attachment 8986435 [details]
Bug 1387128 - Drop all the require("promise").
https://reviewboard.mozilla.org/r/251798/#review258278
Attachment #8986435 -
Flags: review?(jryans) → review+
Comment 38•6 years ago
|
||
mozreview-review |
Comment on attachment 8986434 [details]
Bug 1387128 - Replace all other lower case usages.
https://reviewboard.mozilla.org/r/251796/#review258280
Attachment #8986434 -
Flags: review?(jryans) → review+
Comment 39•6 years ago
|
||
mozreview-review |
Comment on attachment 8895313 [details]
Bug 1387128 - Replace all lower case usages with capital P.
https://reviewboard.mozilla.org/r/166518/#review258282
Thanks for working on this! :) It's great to be much closer to full removal of Promise.jsm!
Attachment #8895313 -
Flags: review?(jryans) → review+
Updated•4 years ago
|
Comment 41•4 years ago
|
||
Since the bug didn't move forward for 3 years, let's turn it into a meta and split it in smaller, more actionable bugs
Keywords: meta
Summary: Drop all usages of promise = require(promise) → [META] Drop all usages of promise = require(promise)
Updated•3 years ago
|
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•