1667455 - Expose a "Services" property on all privileged JS scopes (like Cu/Cc/Ci)

Status: RESOLVED FIXED

(Core :: XPCOM, task)

Description

[:Gijs (he/him)]

After bug 1464542, Services.jsm is just a thin wrapper around Cu.createServicesCache();

We could expose it as a property on Chrome scopes similar to how bug 767640 exposed Cu/Cc/Ci . Then we can remove the thousands of ChromeUtils.import(".../Services.jsm") calls from our codebase.

Comment 1

[:Gijs (he/him)]

Marking fxperf as the reduction in import calls could conceivably have memory/cpu benefits

Comment 2

[Florian Quèze [:florian]]

Tentatively marking fxperf:p2 as I think this is worth doing, if only to reduce the amont of ChromeUtils.import markers we see in startup profiles. I doubt the impact will be measurable for users though, so could have been p3.

Comment 3

[Tooru Fujisawa [:arai]]

we could look into this as a part of ESMification

Comment 4

[Tooru Fujisawa [:arai]]

WIP: https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=cacc0ce3a9d09c996297a87e6d17285d45937a9b
still some error
and also it doesn't touch require consumers and SpecialPowers consumers, and some edge cases, such as generated file.

Comment 5

[Tooru Fujisawa [:arai]]

https://github.com/mozilla/fxa-pairing-channel/issues/74 needs to be done concurrently
(maybe it's not urgent, given there's no frequent update on the repo)

Comment 6

[Kris Maglione [:kmag]]

(In reply to Tooru Fujisawa [:arai] from comment #5)

https://github.com/mozilla/fxa-pairing-channel/issues/74 needs to be done concurrently
(maybe it's not urgent, given there's no frequent update on the repo)

The general rule is that anything without in-tree tests doesn't block landing any change. External add-ons without in-tree tests are responsible for keeping up with breaking changes on their own. Though it's nice to warn them ahead of time.

Comment 7

[Tooru Fujisawa [:arai]]

Sorry, my comment lacks the context.
the file is the source of in-tree auto-generated file

https://searchfox.org/mozilla-central/rev/da3cc1b31be7a7d919d6ebde4e3c033a83446e05/services/fxaccounts/FxAccountsPairingChannel.js#25

const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

Comment 8

[Kris Maglione [:kmag]]

(In reply to Tooru Fujisawa [:arai] from comment #7)

Sorry, my comment lacks the context.
the file is the source of in-tree auto-generated file

https://searchfox.org/mozilla-central/rev/da3cc1b31be7a7d919d6ebde4e3c033a83446e05/services/fxaccounts/FxAccountsPairingChannel.js#25

const {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

OK, in that case it may make sense to just leave Services.jsm for now and remove it when that is updated. Or just update the generated file in the meantime. It's a fairly trivial change.

Comment 9

[Tooru Fujisawa [:arai]]

There's another issue with Android host utils.
https://treeherder.mozilla.org/logviewer?job_id=383202080&repo=try&lineNumber=1783

The latest code is interpreted with older binary.
So the related code there (at least httpd.js) needs to be compatible both with new and old code until the host utils gets updated, that can happen only after the patch gets landed.

I'll check what part gets affected.

Comment 10

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #9)

There's another issue with Android host utils.
https://treeherder.mozilla.org/logviewer?job_id=383202080&repo=try&lineNumber=1783

The latest code is interpreted with older binary.
So the related code there (at least httpd.js) needs to be compatible both with new and old code until the host utils gets updated, that can happen only after the patch gets landed.

I'll check what part gets affected.

I hope const Services = globalThis.Services || ChromeUtils.import("resource://gre/modules/Services.jsm").Services; works.
If that's the case, the same declaration can be used also in extensions etc.

Comment 11

[Kris Maglione [:kmag]]

(In reply to Tooru Fujisawa [:arai] from comment #9)

There's another issue with Android host utils.
https://treeherder.mozilla.org/logviewer?job_id=383202080&repo=try&lineNumber=1783

The latest code is interpreted with older binary.
So the related code there (at least httpd.js) needs to be compatible both with new and old code until the host utils gets updated, that can happen only after the patch gets landed.

Yes, that's an unfortunate problem I've run into more than once. It can be updated before the code lands, but keeping a stub Services.jsm until it gets updated is fine too.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Tooru Fujisawa [:arai] from comment #10)

I hope const Services = globalThis.Services || ChromeUtils.import("resource://gre/modules/Services.jsm").Services; works.
If that's the case, the same declaration can be used also in extensions etc.

It may, depending on the kind of scope it's used in and how the global Services is declared. But for host-utils, I don't think we have to worry. We can land the binary changes that add the Services global and leave a stub Services.jsm, and then after host-utils is updated, land the migration to relying on the global and the removal of Services.jsm.

We can also just deploy a new version of host-utils from a try build, and land the config changes to start using it with the rest of this bug.

Comment 13

[Tooru Fujisawa [:arai]]

Thanks, so far the above workaround seems to work.
We can use the Services.jsm stub approach as fallback path if there's any other issue.

Comment 14

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 1: Define Services in all system globals. r?kmag!

Given the Services object must belong to the loader's shared global,
there are 2 paths added.

For loader's shared global, xpc::CREATE_SERVICES option is passed to
xpc::InitClassesWithNewWrappedGlobal and it creates the Services object
and defines it on the global.

For other globals, xpc::DEFINE_SERVICES option is passed to
xpc::InitClassesWithNewWrappedGlobal and it gets the Services object
from the loader's shared global and defines it on the newly created global.

Depends on D150885

Comment 15

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 2: Add Services to system globals in ESLint. r?kmag!

Depends on D150890

Comment 16

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 3: Re-export global Services in Services.jsm. r?kmag!

Given Services object is already created and defined on the loader's shared
global, Services.jsm shouldn't create yet another instance.

Just return the shared global's property, to make it keeps working in the
middle of this patch's stack.

Depends on D150891

Comment 17

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 4: Stop importing Services.jsm from JSM. r?kmag!

Depends on D150892

Comment 18

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 5: Stop importing Services.jsm from chrome-priv JS code, top-level single-line cases. r?kmag!

Depends on D150893

Comment 19

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 6: Stop importing Services.jsm from chrome-priv JS code, non-top-level or multi-line cases. r?kmag!

Depends on D150894

Comment 20

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 7: Stop importing Services.jsm from chrome-priv JS code, lazy cases. r?kmag!

Depends on D150895

Comment 21

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 8: Stop importing Services.jsm from chrome-priv JS code, other cases. r?kmag!

Depends on D150896

Comment 22

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 9: Stop importing Services.jsm from chrome-priv HTML code, single-line cases. r?kmag!

Depends on D150897

Comment 23

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 10: Stop importing Services.jsm from chrome-priv HTML code, multi-line cases. r?kmag!

Depends on D150898

Comment 24

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 11: Stop importing Services.jsm from chrome-priv HTML code, other cases. r?kmag!

Depends on D150899

Comment 25

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 12: Do not redefine Services in SpecialPowersSandbox. r?kmag!

Depends on D150900

Comment 26

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 13: Stop importing Services.jsm from marionette tests. r?kmag!

Depends on D150901

Comment 27

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 14: Stop importing Services.jsm in documents. r?kmag!

Depends on D150902

Comment 28

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 15: Stop using Services.jsm in eslint testcase. r?kmag!

Depends on D150903

Comment 29

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 16: Stop importing Services.jsm from AutoConfig. r?kmag!

Depends on D150904

Comment 30

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 17: Update devtools stub. r?kmag!

Depends on D150905

Comment 31

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 18: Always use SpecialPowers.Services. r?kmag!

Depends on D150906

Comment 32

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 2.6: Export Services from Devtools require("chrome"). r?kmag!

Depends on D150907

Comment 33

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 2.5: Update DevTools TypeScript support. r?kmag!

• Remove Services.jsm
• Add Services global variable

Depends on D150908

Comment 34

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 19: Stop importing Services.jsm from generated FxAccountsPairingChannel.js. r?kmag!

The source of the file is updated in https://github.com/mozilla/fxa-pairing-channel/issues/74

Depends on D150909

Comment 35

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 20: Remove Services.jsm from comments. r?kmag!

Depends on D150910

Comment 36

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 21: Use SpecialPowers.Services in docshell/test/navigation/test_contentpolicy_block_window.html. r?kmag!

Depends on D150911

Comment 37

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 22: Make httpd.js compatible both with patched and unpatched binary (hostutils). r?kmag!

Depends on D150912

Comment 38

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 25: Remove Services.jsm. r?kmag!

Depends on D150913

Comment 39

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 26: Remove Cu.createServicesCache. r?kmag!

Comment 40

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 1-b: Define Services in Sandbox with wantComponents. r?kmag!

Depends on D150890

Comment 41

[Rob Wu [:robwu]]

There may be out-of-tree code that loads Services.jsm, i.e. privileged extensions.

Privileged extensions that rely on Services.jsm in a WebExtension experiment can simply use Services as a global if it targets Firefox 88 and later, since the Services global was introduced to that scope in bug 1698158.

Comment 42

[Tooru Fujisawa [:arai]]

(In reply to Rob Wu [:robwu] from comment #41)

There may be out-of-tree code that loads Services.jsm, i.e. privileged extensions.

discussion about out-of-tree code in bug 1777486.

Privileged extensions that rely on Services.jsm in a WebExtension experiment can simply use Services as a global if it targets Firefox 88 and later, since the Services global was introduced to that scope in bug 1698158.

Good to know. I'll draft an email for privileged extensions authors.

Comment 43

[Tooru Fujisawa [:arai]]

So far, I don't see any performance impact in talos
https://treeherder.mozilla.org/perfherder/compare?originalProject=try&originalRevision=48549ef578058976af2c6149c2b4361212f8ed86&newProject=try&newRevision=125af292176784acb0ac2f3618e7a758a5824fff&framework=1

Comment 44

[Tooru Fujisawa [:arai]]

given bug 1777906 will take some time, we'd better deferring the Services.jsm file removal into followup bug, or land it later with leave-open.

Comment 45

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 23: Update DevTools TypeScript support. r?mossop!

Depends on D150913

Comment 46

[Tooru Fujisawa [:arai]]

Attachment: Bug 1667455 - Part 24: Remove Services.jsm reference in testcase that monitors module loading. r?kmag!

Comment 47

[Tooru Fujisawa [:arai]]

I'll land up to Part 24, keeping the Services.jsm file in tree, so that quitter.xpi keeps working, while there's no other Services.jsm consumer

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/34c394b9c75f
Part 1: Define Services in all system globals. r=kmag
https://hg.mozilla.org/integration/autoland/rev/87ac3e68b65c
Part 1-b: Define Services in Sandbox with wantComponents. r=kmag
https://hg.mozilla.org/integration/autoland/rev/b1b9c2253479
Part 2: Add Services to system globals in ESLint. r=kmag
https://hg.mozilla.org/integration/autoland/rev/3ca5cef1f6a5
Part 2.5: Update DevTools TypeScript support. r=devtools-reviewers,julienw
https://hg.mozilla.org/integration/autoland/rev/740d9d3682db
Part 2.6: Export Services from Devtools require("chrome"). r=kmag,devtools-reviewers,ochameau
https://hg.mozilla.org/integration/autoland/rev/05f96b585cb1
Part 3: Re-export global Services in Services.jsm. r=kmag
https://hg.mozilla.org/integration/autoland/rev/ad6234248f3d
Part 4: Stop importing Services.jsm from JSM. r=kmag,webdriver-reviewers,perftest-reviewers,webcompat-reviewers,geckoview-reviewers,application-update-reviewers,pip-reviewers,twisniewski,devtools-reviewers,m_kato,jdescottes,ochameau,mconley,sfoster,AlexandruIonescu
https://hg.mozilla.org/integration/autoland/rev/ae74e5acc07a
Part 5: Stop importing Services.jsm from chrome-priv JS code, top-level single-line cases. r=kmag,webdriver-reviewers,perftest-reviewers,webcompat-reviewers,geckoview-reviewers,extension-reviewers,application-update-reviewers,pip-reviewers,twisniewski,m_kato,jdescottes,mconley,AlexandruIonescu,mossop
https://hg.mozilla.org/integration/autoland/rev/67c4a154b997
Part 6: Stop importing Services.jsm from chrome-priv JS code, non-top-level or multi-line cases. r=kmag,perftest-reviewers,AlexandruIonescu,sparky
https://hg.mozilla.org/integration/autoland/rev/77adca5277bb
Part 7: Stop importing Services.jsm from chrome-priv JS code, lazy cases. r=kmag
https://hg.mozilla.org/integration/autoland/rev/2bfc02b367b2
Part 8: Stop importing Services.jsm from chrome-priv JS code, other cases. r=kmag,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/cdaef3be8f9b
Part 9: Stop importing Services.jsm from chrome-priv HTML code, single-line cases. r=kmag,necko-reviewers,geckoview-reviewers,extension-reviewers,m_kato,dragana
https://hg.mozilla.org/integration/autoland/rev/2b9c6e981362
Part 10: Stop importing Services.jsm from chrome-priv HTML code, multi-line cases. r=kmag
https://hg.mozilla.org/integration/autoland/rev/cc60fad3242f
Part 11: Stop importing Services.jsm from chrome-priv HTML code, other cases. r=kmag
https://hg.mozilla.org/integration/autoland/rev/ddf8e7192a86
Part 12: Do not redefine Services in SpecialPowersSandbox. r=kmag
https://hg.mozilla.org/integration/autoland/rev/47c896d5a269
Part 13: Stop importing Services.jsm from marionette tests. r=kmag,webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/a653b4080d9e
Part 14: Stop importing Services.jsm in documents. r=kmag,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/61f066cec38f
Part 15: Stop using Services.jsm in eslint testcase. r=kmag
https://hg.mozilla.org/integration/autoland/rev/1eb3c259c000
Part 16: Stop importing Services.jsm from AutoConfig. r=kmag
https://hg.mozilla.org/integration/autoland/rev/56af53154205
Part 17: Update devtools stub. r=devtools-reviewers,nchevobbe
https://hg.mozilla.org/integration/autoland/rev/3fdd6fd30f3c
Part 18: Always use SpecialPowers.Services. r=kmag
https://hg.mozilla.org/integration/autoland/rev/79a7ec7c55c6
Part 19: Stop importing Services.jsm from generated FxAccountsPairingChannel.js. r=kmag
https://hg.mozilla.org/integration/autoland/rev/7d1ca779e507
Part 20: Remove Services.jsm from comments. r=kmag
https://hg.mozilla.org/integration/autoland/rev/4a78e424e248
Part 21: Use SpecialPowers.Services in docshell/test/navigation/test_contentpolicy_block_window.html. r=kmag
https://hg.mozilla.org/integration/autoland/rev/911d540fce6f
Part 22: Make httpd.js compatible both with patched and unpatched binary (hostutils). r=kmag,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/4529b79391c8
Part 23: Update DevTools TypeScript support. r=julienw
https://hg.mozilla.org/integration/autoland/rev/563470602734
Part 24: Remove Services.jsm reference in testcase that monitors module loading. r=kmag

Comment 49

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/34c394b9c75f
https://hg.mozilla.org/mozilla-central/rev/87ac3e68b65c
https://hg.mozilla.org/mozilla-central/rev/b1b9c2253479
https://hg.mozilla.org/mozilla-central/rev/3ca5cef1f6a5
https://hg.mozilla.org/mozilla-central/rev/740d9d3682db
https://hg.mozilla.org/mozilla-central/rev/05f96b585cb1
https://hg.mozilla.org/mozilla-central/rev/ad6234248f3d
https://hg.mozilla.org/mozilla-central/rev/ae74e5acc07a
https://hg.mozilla.org/mozilla-central/rev/67c4a154b997
https://hg.mozilla.org/mozilla-central/rev/77adca5277bb
https://hg.mozilla.org/mozilla-central/rev/2bfc02b367b2
https://hg.mozilla.org/mozilla-central/rev/cdaef3be8f9b
https://hg.mozilla.org/mozilla-central/rev/2b9c6e981362
https://hg.mozilla.org/mozilla-central/rev/cc60fad3242f
https://hg.mozilla.org/mozilla-central/rev/ddf8e7192a86
https://hg.mozilla.org/mozilla-central/rev/47c896d5a269
https://hg.mozilla.org/mozilla-central/rev/a653b4080d9e
https://hg.mozilla.org/mozilla-central/rev/61f066cec38f
https://hg.mozilla.org/mozilla-central/rev/1eb3c259c000
https://hg.mozilla.org/mozilla-central/rev/56af53154205
https://hg.mozilla.org/mozilla-central/rev/3fdd6fd30f3c
https://hg.mozilla.org/mozilla-central/rev/79a7ec7c55c6
https://hg.mozilla.org/mozilla-central/rev/7d1ca779e507
https://hg.mozilla.org/mozilla-central/rev/4a78e424e248
https://hg.mozilla.org/mozilla-central/rev/911d540fce6f
https://hg.mozilla.org/mozilla-central/rev/4529b79391c8
https://hg.mozilla.org/mozilla-central/rev/563470602734

Comment 50

[Tooru Fujisawa [:arai]]

Adding reminder to move the remaining patches to followup bug at the point of soft freeze.
(this is not actually "test" reminder, but using it for now)
If quitter.xpi dependency gets sorted out before that point, we can land them here.

Comment 51

[Acasandrei Beatrice (needinfo me)]

== Change summary for alert #34800 (as of Wed, 13 Jul 2022 05:25:36 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
0.38%	Base Content JS	macosx1015-64-shippable-qr	fission	1,639,512.00 -> 1,633,304.00
0.34%	Base Content JS	linux1804-64-shippable-qr	fission	1,610,467.33 -> 1,604,954.67
0.33%	Base Content JS	windows10-64-2004-shippable-qr	fission	1,612,408.00 -> 1,607,128.00
0.31%	Base Content JS	linux1804-64-shippable-qr	fission	1,610,452.67 -> 1,605,466.67

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=34800

Comment 52

[Tooru Fujisawa [:arai]]

I'll move the removal patches to followup bug

Comment 53

[Phabricator Automation]

Comment on attachment 9283939 [details]
Bug 1667455 - Part 25: Remove Services.jsm. r?kmag!

Revision D150914 was moved to bug 1780695. Setting attachment 9283939 [details] to obsolete.

Comment 54

[Phabricator Automation]

Comment on attachment 9283942 [details]
Bug 1667455 - Part 26: Remove Cu.createServicesCache. r?kmag!

Revision D150915 was moved to bug 1780695. Setting attachment 9283942 [details] to obsolete.