Remove uses of Promise.jsm from Firefox

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Paolo, Assigned: florian)

Tracking

(Blocks: 2 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

2 years 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

2 years ago
No longer depends on: 1034599
(Reporter)

Updated

2 years ago
Depends on: 1034599
(Reporter)

Updated

2 years ago
Blocks: 1363777
Whiteboard: [qf][photon]
(Reporter)

Updated

2 years ago
Depends on: 1095267
Whiteboard: [qf][photon] → [qf] [photon-performance] [triage]
Flags: qe-verify-
Priority: -- → P2
Whiteboard: [qf] [photon-performance] [triage] → [qf] [photon-performance]
(Assignee)

Comment 1

2 years 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.
Whiteboard: [qf] [photon-performance] → [qf:meta] [photon-performance]
(Assignee)

Comment 2

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

Updated

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

Comment 4

2 years ago
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 6

2 years ago
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)
Iteration: --- → 56.1 - Jun 26
Priority: P2 → P1
(Assignee)

Comment 7

2 years ago
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 years 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 years 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 years 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 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
(Assignee)

Updated

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