Remove uses of Promise.jsm from Firefox

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Async Tooling
P1
normal
RESOLVED FIXED
3 months ago
28 days ago

People

(Reporter: Paolo, Assigned: florian)

Tracking

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

unspecified
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:meta] [photon-performance])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 months ago
Now that bug 939636 is resolved, most imports of the Promise.jsm module can already be removed. Because some callers still use Promise.defer(), bug 1034599 should be fixed before all instances can be removed.
(Reporter)

Updated

3 months ago
No longer depends on: 1034599
(Reporter)

Updated

3 months ago
Depends on: 1034599
(Reporter)

Updated

3 months ago
Blocks: 1363777
Whiteboard: [qf][photon]
(Reporter)

Updated

3 months ago
Depends on: 1095267

Updated

3 months ago
Whiteboard: [qf][photon] → [qf] [photon-performance] [triage]

Updated

3 months ago
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [qf] [photon-performance] [triage] → [qf] [photon-performance]
(Assignee)

Comment 1

3 months ago
In case anybody needs to be convinced that this is related to performance, see this profile captured at a random time on a slow netbook: https://perf-html.io/public/4799687e95491a7258e702308f81cfb01b3c18cd/calltree/?implementation=js&invertCallstack&thread=0
See how much time is being spent within Promise-backend.js.

Updated

3 months ago
Whiteboard: [qf] [photon-performance] → [qf:meta] [photon-performance]
(Assignee)

Comment 2

2 months ago
Created attachment 8880187 [details] [diff] [review]
script generated patch

This excludes files in a /test/ or /tests/ folder, or files from testing/.
Attachment #8880187 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

2 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 months ago
Created attachment 8880189 [details]
promise.jsm.js xpcshell script
(Assignee)

Comment 4

2 months ago
Created attachment 8880190 [details] [diff] [review]
blacklist Promise.jsm before first paint

We can now blacklist Promise.jsm before first paint to avoid regressions.
We unfortunately can't blacklist it completely from startup because http://searchfox.org/mozilla-central/rev/714606a8145636d93b116943d3a65a6a49d2acf8/devtools/shared/builtin-modules.js#18 still imports it during delayed startup, and I don't feel like fighting that devtools code to make it lazy... especially when all of this is going to move to an add-on soon.
Attachment #8880190 - Flags: review?(paolo.mozmail)
(Assignee)

Comment 5

2 months ago
Editing the summary to match what I'm doing here.

Try seems green-ish: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b5e36f64ce03327016be15f53c97035bdf3fadc

And Talos seems happy: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=16f96f3cda75b5d312c62e7627aed240c2b5ac96&newProject=try&newRevision=8b5e36f64ce03327016be15f53c97035bdf3fadc&framework=1
Summary: Remove uses of Promise.jsm from mozilla-central → Remove uses of Promise.jsm from Firefox
(Assignee)

Comment 6

2 months ago
Created attachment 8880451 [details] [diff] [review]
remove Promise.jsm imports in tests

This is an attempt to also cleanup tests where it's easy. If it's too hard to have a green run with this, it can be postponed to a follow-up.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58c5d52d708102b3c54c7554865770dab7563c3f
Attachment #8880451 - Flags: review?(paolo.mozmail)

Updated

2 months ago
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
(Assignee)

Comment 7

2 months ago
Created attachment 8880474 [details] [diff] [review]
remove Promise.jsm imports in tests v2

Fixed test failures from the previous try push, new try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=804660f53717455a0bbe9595af98545d038f6eea
Attachment #8880474 - Flags: review?(paolo.mozmail)
(Assignee)

Updated

2 months ago
Attachment #8880451 - Attachment is obsolete: true
Attachment #8880451 - Flags: review?(paolo.mozmail)
Feel free to set me as the reviewer if paolo is busy / overloaded.
Comment on attachment 8880187 [details] [diff] [review]
script generated patch

Review of attachment 8880187 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine - though I didn't check to see if this was exhaustive (nor that there were no remaining usages of Promise.jsm stuff in any of these files...)
Attachment #8880187 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8880190 [details] [diff] [review]
blacklist Promise.jsm before first paint

Review of attachment 8880190 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8880190 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8880474 [details] [diff] [review]
remove Promise.jsm imports in tests v2

Review of attachment 8880474 [details] [diff] [review]:
-----------------------------------------------------------------

As before, I didn't actually check to see if there were any remaining uses of Promise.jsm in these folders with this patch, but the changes themselves look good.

You probably already had this in mind, but probably it'd be a good idea to send mail to firefox-dev / dev-platform and let them know that Promise.jsm is no longer to be used.

Great job!

::: toolkit/components/osfile/tests/xpcshell/test_creationDate.js
@@ -9,5 @@
>   * Test to ensure that deprecation warning is issued on use
>   * of creationDate.
>   */
>  add_task(async function test_deprecatedCreationDate () {
> -  let deferred = Promise.defer();

So, I guess nothing was using deferred.promise? Weird. I wonder if this will fix some orange!
Attachment #8880474 - Flags: review?(paolo.mozmail) → review+

Comment 12

2 months ago
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c7e4d4547b7
Remove imports of Promise.jsm from Firefox, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b1c441ed4c
blacklist Promise.jsm before first paint, r=mconley.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d121d4c2033
remove Promise.jsm imports in tests, r=mconley.
Depends on: 1376105

Comment 13

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c7e4d4547b7
https://hg.mozilla.org/mozilla-central/rev/e1b1c441ed4c
https://hg.mozilla.org/mozilla-central/rev/6d121d4c2033
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

2 months ago
Depends on: 1378171
Blocks: 1378173
You need to log in before you can comment on or make changes to this bug.