1609271 - Stop using ChromeUtils.import(..., null) which returns the global object for a module instead of its exports

Status: RESOLVED FIXED

(Core :: XPConnect, task, P5)

Description

[Brian Grinstead [:bgrins]]

Some things that break my patches in Bug 1608278 where properties are no longer assigned to this (for example this.foo = 1 is changed to const foo = 1) are backstage passes.

When null is passed into ChromeUtils.import it allows access to properties set on this but not exported (https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/dom/chrome-webidl/ChromeUtils.webidl#297-301 & https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/dom/base/ChromeUtils.cpp#409,422). For example https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/browser/base/content/test/sync/browser_fxa_web_channel.js#15 uses properties from https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/services/fxaccounts/FxAccountsWebChannel.jsm that aren't exported.

We should come up with some kind of convention for objects needed for tests (prefixing objects with _?), and rewrite callers to use it.

Comment 1

[Brian Grinstead [:bgrins]]

There's a "no-this-in-ChromeUtils.import" eslint rule Mossop made in https://phabricator.services.mozilla.com/D58856, it could probably be modified to look for null instead to get a list of places that need updating.

Hopefully they are mostly tests such that we can just remove the , null and then iterate on fixing failures via try.

Comment 2

[Kris Maglione [:kmag]]

We don't use BackstagePasses in ChromeUtils.import. I mean, except to the extent that the shared JSM global happens to be a BackstagePass, which will still be the case even when we switch to ES6 modules. (And for mochitest modules, because they do horrible things to the compartment flags of the global they're loaded in)

Comment 3

[Brian Grinstead [:bgrins]]

(In reply to Kris Maglione [:kmag] from comment #2)

We don't use BackstagePasses in ChromeUtils.import. I mean, except to the extent that the shared JSM global happens to be a BackstagePass, which will still be the case even when we switch to ES6 modules. (And for mochitest modules, because they do horrible things to the compartment flags of the global they're loaded in)

I guess the terminology is wrong then - I was reading the comment at https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/browser/base/content/test/sync/browser_fxa_web_channel.js#16 when I filed the bug.

I meant this to mean that we shouldn't do ChromeUtils.import(..., null); - I've updated the bug summary.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: chromeutils-import-null violations

(In reply to Brian Grinstead [:bgrins] from comment #1)
> There's a "no-this-in-ChromeUtils.import" eslint rule Mossop made in https://phabricator.services.mozilla.com/D58856, it could probably be modified to look for `null` instead to get a list of places that need updating.
> 
> Hopefully they are mostly tests such that we can just remove the `, null` and then iterate on fixing failures via try.

Here's a try push with a rule checking against this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9de7c458d2337c024d41d1fc42c865f9c57438fc&selectedJob=285213700

The list of eslint violations is attached here

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609271 - Add linting rule to prevent using ChromeUtils.import(..., null)

Comment 6

[Julian Descottes [:jdescottes]]

(attaching the rule as a patch here for now, not sure if we want to actually land it, but can be useful anyway)

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609271 - Fix ChromeUtils.import(..., null) in browser_datachoices_notification.js

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609271 - Fix ChromeUtils.import(..., null) for FxAccountsWebChannel.jsm

Depends on D61489

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609271 - Fix ChromeUtils.import(..., null) for Telemetry JSMs

Depends on D61490

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609271 - Fix ChromeUtils.import(..., null) for test_uptake_telemetry.js

Depends on D61491

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1609271 - Fix ChromeUtils.import(..., null) for ESEDBReader.jsm

Depends on D61492

Comment 13

[Julian Descottes [:jdescottes]]

Hi Ted, I would like to have your opinion on a first set of patches to cleanup the usage of ChromeUtils.import(..., null) issue.

I used one of the solutions that was mentioned in slack previously: export everything needed by the tests via an alias prefixed with an underscore. This won't make the tests cleaner but it makes the change less intrusive both in tests and JSMs. Alternatively we could expose "configuration" functions, but I'm afraid this will take much more time.

I think technically this should work fine when transitioning to ES modules? The main requirement is that the consumer should be able to update properties of an exported object, eg:

// file1.js
const someObject = { someProperty: "initial value" };
export { someObject };

// file2.js 
import { someObject } from './file1.js'
someObject.someProperty = "mocked value"; // <- should update the object in the original module

I think you can get an idea of how this looks by checking https://phabricator.services.mozilla.com/D61491 and https://phabricator.services.mozilla.com/D61492 (which needed more updating than the typical test). More complex tests to update are yet to be handled (esp those using extension JSMs, they are really intrusive).

Let me know what you think, thanks!

Comment 14

[Tomislav Jovanovic :zombie]

(In reply to Julian Descottes [:jdescottes] from comment #13)

(esp those using extension JSMs, they are really intrusive).

Can you please give some examples of what you mean by "intrusive"?

Comment 15

[Julian Descottes [:jdescottes]]

(In reply to :Tomislav Jovanovic :zombie from comment #14)

(In reply to Julian Descottes [:jdescottes] from comment #13)

(esp those using extension JSMs, they are really intrusive).

Can you please give some examples of what you mean by "intrusive"?

When I say intrusive here, I usually mean that the test or test helper will override properties directly on the JSM scope in order to mock complex behaviors.

For instance https://searchfox.org/mozilla-central/search?q=GMPScope&path=toolkit%2Fmozapps%2Fextensions%2Ftest%2Fxpcshell%2Ftest_gmpProvider.js

This test imports https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/GMPProvider.jsm, which has 0 exports at the moment. It uses the following properties from the JSM:

• GMP_PLUGINS
• GMPPrefs
• GMPProvider
• KEY_UPDATE_LAST_CHECK
• SEC_IN_A_DAY

But it also mocks 2 properties directly on the JSM:

• GMPInstallManager
• gmpService

Other example in AddonTestUtils: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#963-969 . Similar one directly in a test https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/rs-blocklist/test_blocklistchange.js#614-618

When I was trying to fix all violations a few weeks ago, it seemed to me that extensions-related tests & helpers were relying on ChromeUtils.import(..., null) to mock complicated behaviors more frequently than others. But searching today, it does seem that most of the complexity is in a few files (AddonTestUtils for instance), so maybe it's not that bad.

Comment 16

[Ted Campbell [:tcampbell]]

Your examples are all fine in ES modules. ES Modules actually allow an even more powerful syntax such as export { variable1 as name1, variable2 as name2, …, nameN }.

Once we complete the switch to ES modules, updating direct bindings is also possible:

// file1.js
export let someProperty = "initial value";

// file2.js 
import { someProperty } from './file1.js'
someProperty = "mocked value"; // <- should update the object in the original module
//OR
let mod = ChromeUtils.import('file1.js');
mod.someProperty = "mocked value";

Comment 17

[Magnus Melin [:mkmelin]]

Any plans to finish these patches?

Comment 18

[Julian Descottes [:jdescottes]]

Unassigning here to reflect that I haven't looked at this in over a year. If anyone has cycles to spare and can work on this feel free to take over. I think the eslint rule written in https://hg.mozilla.org/try/rev/cf4336dd7a920a8be5f5f9cb1992273b385f77e2 could still be helpful here to get a fresh list of the violations.

Comment 19

[Mark Banner (:standard8)]

To note: bug 1611229 and bug 1608272 added an ESLint rule (mozilla/reject-chromeutils-import-params) to disallow parameters to ChromeUtils.import, i.e. both this and null.

There are still quite a few instances allowed via the top-level .eslintrc.js: https://searchfox.org/mozilla-central/rev/37edd2782e67e716dd07a85016da07b4d6275e5d/.eslintrc.js#488-640

Comment 20

[Chris Peterson [:cpeterson]]

Setting Severity = N/A to move this bug out of XPConnect's bug triage query.

Comment 21

[Mark Banner (:standard8)]

Attachment: Bug 1609271 - Disallow passing null as the target argument to ChromeUtils.import. r?evilpie

Depends on D140515

Comment 22

[Mark Banner (:standard8)]

The dependencies on this bug now cover what I believe are the remaining items for this to complete. The majority of them have patches, so I've also posted an initial stab at disallowing the target argument being null from the code. I'm not sure if this will land, it might be that we can remove the second argument completely - we're not too far off from that being redundant now.

Comment 23

[Mark Banner (:standard8)]

I decided to get this landed - removing the second argument is a little bit more work than expected (last 10%...), so lets get this bit tided up and it also will definitely prevent any new usages of null.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/78ae535c13b6
Disallow passing null as the target argument to ChromeUtils.import. r=evilpie,emilio

Comment 25

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/78ae535c13b6