Stop using ChromeUtils.import(..., null) which returns the global object for a module instead of its exports
Categories
(Core :: XPConnect, task, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox101 | --- | fixed |
People
(Reporter: bgrins, Assigned: standard8)
References
(Blocks 3 open bugs)
Details
Attachments
(2 files, 6 obsolete files)
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.
Reporter | ||
Comment 1•5 years ago
|
||
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•5 years ago
|
||
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)
Reporter | ||
Comment 3•5 years ago
|
||
(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•5 years ago
|
||
Comment 5•5 years ago
|
||
Comment 6•5 years ago
|
||
(attaching the rule as a patch here for now, not sure if we want to actually land it, but can be useful anyway)
Updated•5 years ago
|
Updated•5 years ago
|
Comment 8•5 years ago
|
||
Comment 9•5 years ago
|
||
Depends on D61489
Comment 10•5 years ago
|
||
Depends on D61490
Comment 11•5 years ago
|
||
Depends on D61491
Comment 12•5 years ago
|
||
Depends on D61492
Updated•5 years ago
|
Comment 13•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
(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.
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•5 years ago
|
||
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•4 years ago
|
||
Any plans to finish these patches?
Comment 18•4 years ago
|
||
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.
Assignee | ||
Comment 19•4 years ago
|
||
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
Updated•3 years ago
|
Comment 20•3 years ago
|
||
Setting Severity = N/A to move this bug out of XPConnect's bug triage query.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 21•3 years ago
|
||
Depends on D140515
Assignee | ||
Comment 22•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 23•3 years ago
|
||
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•3 years ago
|
||
Comment 25•3 years ago
|
||
bugherder |
Description
•