Closed Bug 1454373 Opened 6 years ago Closed 6 years ago

Use DOM Promises in protocol.js (instead of Promise.jsm)

Categories

(DevTools :: Framework, defect)

defect
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

While working on figuring out why bug 1449162 regress performances, I realized that Promise.jsm stacks were very visible in perf-html profile.
And it looks like replacing Promise.jsm usages in favor of DOM Promises improves the tests that use to regress when converting netmonitor actors that I tried to convert to procotol.js.

https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=try&originalRevision=05b1f4ebfd22&newProject=try&newRevision=751b03561072ed409796b58b735825d76e5baf05&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyConfident=1&framework=1
Assignee: nobody → poirot.alex
It looks like we can stop using Promise.jsm from defer module without errors:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa369d2480020ba2b2fe0d3afbe4e2aa2c78e6a7
Comment on attachment 8968188 [details]
Bug 1454373 - Switch protocol.js to native promises.

https://reviewboard.mozilla.org/r/236866/#review242712

Great, thanks for doing this! :)
Attachment #8968188 - Flags: review?(jryans) → review+
Comment on attachment 8968230 [details]
Bug 1454373 - Make defer module use DOM Promises.

https://reviewboard.mozilla.org/r/236922/#review242714

::: devtools/shared/defer.js
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> -// See bug 1273941 to understand this choice of promise.

Hmm, the bug here doesn't really help me understand why this was put here, haha... :S

Anyway, we do want to remove it, so hopefully it works.
Attachment #8968230 - Flags: review?(jryans) → review+
Comment on attachment 8968230 [details]
Bug 1454373 - Make defer module use DOM Promises.

https://reviewboard.mozilla.org/r/236922/#review242714

> Hmm, the bug here doesn't really help me understand why this was put here, haha... :S
> 
> Anyway, we do want to remove it, so hopefully it works.

Ah, I guess it was about rejection tracking, but hopefully that's fixed now.
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8968230 [details]
> Bug 1454373 - Make defer module use DOM Promises.
> 
> https://reviewboard.mozilla.org/r/236922/#review242714
> 
> > Hmm, the bug here doesn't really help me understand why this was put here, haha... :S
> > 
> > Anyway, we do want to remove it, so hopefully it works.
> 
> Ah, I guess it was about rejection tracking, but hopefully that's fixed now.

Yes, that was it. I hope it is properly handled now. If not, we have to fix it ASAP as a lot of code now use DOM Promise.
While I was working on Promise.jsm, I used to miss some frames in stacks, but it seems to no longer be true.
I think it is thanks to bug 1438121.
Before, this protcol.js test I'm modifying here, used to miss the "onConnect" frame completely!
Depends on: 1454580
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2ada4fddfa0
Switch protocol.js to native promises. r=jryans
https://hg.mozilla.org/integration/autoland/rev/4ec712f0aa13
Make defer module use DOM Promises. r=jryans
Backed out for mochitest devtools failures on "OS X 10.10 opt" - devtools/client/shadereditor/test/browser_se_*

Backout link: https://hg.mozilla.org/integration/autoland/rev/3ee4cf8dc2ed4ae1b81d229398706e243f3c7c3f
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=174098374&repo=autoland
Flags: needinfo?(poirot.alex)
Hum. I knew it wouldn't be so easy...

I don't get why this test passed in this try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa369d2480020ba2b2fe0d3afbe4e2aa2c78e6a7
While the test is failing locally, on my linux...
Flags: needinfo?(poirot.alex)
So it is about Promise.jsm again, and the famous DOM Promise that are zombified on panel's iframe removal.

*But* this time we are not involving any panel document's iframe!
So it doesn't really make sense... We already hit the exact same code in bug 1444755.

Some destruction code within Shader Editor starts failing. A promise never resolve.
The promise is this one:
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/client/shadereditor/shadereditor.js#439
  deferred.resolve(editor);

It starts failing i.e. it no longer resolves, i.e. the callsite's `then` function argument is no longer called:
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/client/shadereditor/shadereditor.js#457
  return this._getEditor(type).then(editor => {

The arrow function here, is never called.

It starts failing once we remove the use of Promise.jsm in defer module:
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/shared/defer.js#8
-  const Promise = require("promise");

The following change still fails, even if `Promise` symbol is a special DOM Promise, coming from module loader Sandbox and supposed to be always alive and working!
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/client/shadereditor/shadereditor.js#439
-  deferred.resolve(editor);
+  return Promise.resolve(editor);

It also still fails if I switch from Promise.jsm to these special Promises in defer:
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/shared/defer.js#8
-  const Promise = require("promise");
+  const Promise = require("Promise");
Note that we should not even need that. defer.js is loaded in a non-browser-loader sandbox and should be always alive.

The only code I found that was still working, was to do:
https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/devtools/client/shadereditor/shadereditor.js#439
-  deferred.resolve(editor);
+  return {
+    then(a) {
+      a(editor);
+    }
+  };

But this is far from being satisfying!
Attachment #8968230 - Attachment is obsolete: true
So. Modifying defer.js ends up being a much bigger project, related to Promise.jsm removal from client usages.
It would require to stop involving any promise for all panels during destruction. Which is a very large project as we would also have to do that for unmaintained panels.

What I don't understand is why require("Promise") doesn't work. Why even these promises get frozen and never resolves. That would be the only workaround I know about to not have to refactor everything.

In the meantime, I would like to suggest embedding a copy of defer into protocol.js that doesn't use Promise.jsm...
Comment on attachment 8968188 [details]
Bug 1454373 - Switch protocol.js to native promises.

Could you take another look and see if you agree on the defer workaround?
Attachment #8968188 - Flags: review+ → review?(jryans)
Comment on attachment 8968188 [details]
Bug 1454373 - Switch protocol.js to native promises.

https://reviewboard.mozilla.org/r/236866/#review243180

Sorry to hear about the pain you went through! :(

A workaround like this seems fine to me given the perf improvement.  Could you also add a comment in defer.js to point future attempts to change it towards your experience?
Attachment #8968188 - Flags: review?(jryans) → review+
Pushed by apoirot@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cde33317ae7a
Switch protocol.js to native promises. r=jryans
https://hg.mozilla.org/mozilla-central/rev/cde33317ae7a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Blocks: dt-pjs
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.