If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Drop all usages of promise = require(promise)

NEW
Assigned to

Status

()

Firefox
Developer Tools
P3
normal
2 months ago
7 days ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fix-optional)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

2 months ago
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

a month 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

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bd81439e511da3ae8fe3c7e956527752eed7b9d
(Assignee)

Comment 8

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccf76f5c7fe551ac21dbf4dc40a3ce781f111245
(Assignee)

Comment 9

a month 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

a month 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

a month ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=862f93b1e96ec4f0b76e4d5758075c5b25b13dcb
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8895314 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8895315 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8895316 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Attachment #8895317 - Attachment is obsolete: true
(Assignee)

Comment 13

a month ago
Comment on attachment 8895313 [details]
Bug 1387128 - Keep running debugger Promise while debugging workers.

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

a month 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

a month 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

a month ago
Comment on attachment 8895313 [details]
Bug 1387128 - Keep running debugger Promise while debugging workers.

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)
Priority: -- → P3
(Assignee)

Updated

a month ago
Depends on: 1392540
(Assignee)

Comment 17

a month 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

29 days 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

24 days 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.
status-firefox57: --- → fix-optional
You need to log in before you can comment on or make changes to this bug.