Closed Bug 1387128 Opened 7 years ago Closed 3 years ago

[META] Drop all usages of promise = require(promise)

Categories

(DevTools :: General, task, P3)

task

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: ochameau, Assigned: ochameau)

References

Details

(Keywords: meta)

Attachments

(6 files, 4 obsolete files)

59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, text/x-review-board-request
jryans
: review+
Details
59 bytes, 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.
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
[
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
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...
Attachment #8895314 - Attachment is obsolete: true
Attachment #8895315 - Attachment is obsolete: true
Attachment #8895316 - Attachment is obsolete: true
Attachment #8895317 - Attachment is obsolete: true
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)
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)
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 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)
Priority: -- → P3
Depends on: 1392540
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.
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
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.
Depends on: 1402779
Depends on: 1402846
Depends on: 1436978
Product: Firefox → DevTools
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
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 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 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 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 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+
Type: enhancement → task
Depends on: 1679536, 1679539, 1707946

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)
Depends on: 1708625
Depends on: 1708626
Depends on: 1732627
Depends on: 1732678
Depends on: 1732679
Depends on: 1732913
Depends on: 1733643
Depends on: 1734592
Depends on: 1734782
Depends on: 1734785
Depends on: 1734786
No longer depends on: 1402846
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.