1733549 - Remove uses of ChromeUtils.import(..., null) in enterprise policy code

Status: RESOLVED FIXED

(Firefox :: Enterprise Policies, task)

Description

[Mark Banner (:standard8)]

The ChromeUtils.import(..., null) form of importing modules causes the import to return the global object rather than just the items that have been exported from the module. As we look towards a new module system we need to prevent this behaviour.

I'm happy to mentor this. For instructions on how to get your local build of Firefox up and running and submit your patch, see https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html

What needs doing:

• Remove the 5 references (4 for browser and one toolkit) for this rule from the top-level .eslintrc.js
• Run ./mach eslint browser/components/enterprisepolicies toolkit/components/enterprisepolicies to see where the issues are.
• For each import, check the file being imported to see if the imports are listed in that files EXPORTED_SYMBOLS, if they are not then add them.
• Remove the second argument (null) to the import function.
• Re-run ESLint and ensure issues are fixed. If there are issues about formatting you can re-run eslint with --fix to automatically fix those.
• Run the tests to check there are no failures:
  • ./mach xpcshell-test browser/components/enterprisepolicies
  • ./mach mochitest browser/components/enterprisepolicies
  • ./mach xpcshell-test toolkit/components/enterprisepolicies
• Now submit your patch.

Comment 1

[Oladapo Ajala]

Can I be assigned this task?

I am new to contributing to open source and would love to take on this task.

Comment 2

[Mark Banner (:standard8)]

(In reply to Oladapo Ajala from comment #1)

Can I be assigned this task?

I am new to contributing to open source and would love to take on this task.

Please do, bugs will be automatically assigned when the first patch is attached.

Comment 3

[Oladapo Ajala]

Thanks while trying to setup my local copy of the Mozilla central repository using the link you posted (https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html) I discovered the repository size is about 5GB and 20GB after building. I would like to confirm if these file sizes are true so as to create sufficient space on my laptop.

Is this the right place to ask questions or there is another place (slack or discord) where I can interact to get support?

Thanks

Comment 4

[Mark Banner (:standard8)]

(In reply to Oladapo Ajala from comment #3)

Thanks while trying to setup my local copy of the Mozilla central repository using the link you posted (https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html) I discovered the repository size is about 5GB and 20GB after building. I would like to confirm if these file sizes are true so as to create sufficient space on my laptop.

They sound about what I'd expect.

Is this the right place to ask questions or there is another place (slack or discord) where I can interact to get support?

The best place for set-up questions is on Matrix - https://wiki.mozilla.org/Matrix in the #introduction channel. There's generally people around that can help you though sometimes you have to wait a little.

Comment 5

[Oladapo Ajala]

Thank you very much Mark.

Comment 6

[Oladapo Ajala]

I just successfully setup the mozilla-central codebase on my system, I'm looking to get started on fixing the bug today. Should I have any blockers I'd reach out on here!

Thanks

Comment 7

[[e-mail]]

Hi, I'm an Outreachy applicant and I was trying to tackle this issue. I'm not sure I understood how to check if the imports are listed in EXPORTED_SYMBOLS. I ran into a few errors that I'm trying to understand, starting from running ./mach xpcshell-test browser/components/enterprisepolicies.

FAIL browser/components/enterprisepolicies/tests/xpcshell/test_proxy.js - xpcshell return code: 0
FAIL browser/components/enterprisepolicies/tests/xpcshell/test_runOnce_helper.js - xpcshell return code: 0
ERROR Unexpected exception TypeError: can't access property Symbol.iterator, PROXY_TYPES_MAP is undefined at /home/raquel/mozilla-unified/obj-x86_64-pc-linux-gnu/_tests/xpcshell/browser/components/enterprisepolicies/tests/xpcshell/test_proxy.js:15
test_proxy_modes_and_autoconfig@/home/raquel/mozilla-unified/obj-x86_64-pc-linux-gnu/_tests/xpcshell/browser/components/enterprisepolicies/tests/xpcshell/test_proxy.js:15:37
_run_next_test/<@/home/raquel/mozilla-unified/testing/xpcshell/head.js:1666:22
_run_next_test@/home/raquel/mozilla-unified/testing/xpcshell/head.js:1666:38
run@/home/raquel/mozilla-unified/testing/xpcshell/head.js:812:9
_do_main@/home/raquel/mozilla-unified/testing/xpcshell/head.js:240:6
_execute_test@/home/raquel/mozilla-unified/testing/xpcshell/head.js:597:5
@-e:1:1

ERROR Unexpected exception TypeError: runOnce is not a function at /home/raquel/mozilla-unified/obj-x86_64-pc-linux-gnu/_tests/xpcshell/browser/components/enterprisepolicies/tests/xpcshell/test_runOnce_helper.js:16
test_runonce_helper@/home/raquel/mozilla-unified/obj-x86_64-pc-linux-gnu/_tests/xpcshell/browser/components/enterprisepolicies/tests/xpcshell/test_runOnce_helper.js:16:10
_run_next_test/<@/home/raquel/mozilla-unified/testing/xpcshell/head.js:1666:22
_run_next_test@/home/raquel/mozilla-unified/testing/xpcshell/head.js:1666:38
run@/home/raquel/mozilla-unified/testing/xpcshell/head.js:812:9
_do_main@/home/raquel/mozilla-unified/testing/xpcshell/head.js:240:6
_execute_test@/home/raquel/mozilla-unified/testing/xpcshell/head.js:597:5
@-e:1:1

The eslint tests don't have any error in formatting. Could you shine a light to which direction I should head?

Comment 8

[Mark Banner (:standard8)]

EXPORTED_SYMBOLS is defined in each of our .jsm modules, e.g. https://searchfox.org/mozilla-central/rev/8b56462cbd9ad0b642e7feebda1974abfa25f48b/browser/components/enterprisepolicies/helpers/ProxyPolicies.jsm#36

That highlighted line is the rough equivalent of an es6 module saying export var ProxyPolicies ....

Comment 9

[[e-mail]]

Attachment: Unexpected Results

That was incredibly helpful, and I managed to make `./mach xpcshell-test browser/components/enterprisepolicies` and `./mach xpcshell-test toolkit/components/enterprisepolicies` tests to pass. I got a few fails on `./mach mochitest browser/components/enterprisepolicies`. These are the errors I came across: 

```

```

Comment 10

[[e-mail]]

Just an update, that was incredibly helpful, and I managed to make ./mach xpcshell-test browser/components/enterprisepolicies and ./mach xpcshell-test toolkit/components/enterprisepolicies tests to pass.
However, I got 16 fails on ./mach mochitest browser/components/enterprisepolicies, 7 of those are in browser/components/enterprisepolicies/tests/browser/browser_policies_setAndLockPref_API.js. It seems that the lock statuses are not what expected.
I've tried going in browser/components/enterprisepolicies/Policies.jsm and changing function setDefaultPref(prefName, prefValue, locked = false) to locked = true and I got 47 errors instead of the original 16. I'll keep investigating what's going on.

Comment 11

[Mike Kaply [:mkaply]]

Is it possible your machine is using policies? What do you see when you go to about:policies?

Comment 12

[Mark Banner (:standard8)]

Can you also post your patch so far? It might make it easier for us to see any issues.

Comment 13

[[e-mail]]

Attachment: Policies.jsm

(In reply to Mike Kaply [:mkaply] from comment #11)
> Is it possible your machine is using policies? What do you see when you go to about:policies?

I'm using FIrefox Developer Edition in Ubuntu 21.10 and it says the enterprise policies services is deactivated.


(In reply to Mark Banner (:standard8) from comment #12)
> Can you also post your patch so far? It might make it easier for us to see any issues.

Sure, no problem. I redid all the importing and removed each `ChromeUtils.import(..., null)` import listed in the bug description and managed to track what was happening. In `toolkit/components/enterprisepolicies/tests/EnterprisePolicyTesting.jsm`, I'm getting errors when removing the `null` parts in `var PoliciesPrefTracker`:

```javascript
  start() {
    let PoliciesBackstage = ChromeUtils.import(
      "resource:///modules/policies/Policies.jsm",
      null
    );
    this._originalFunc = PoliciesBackstage.setDefaultPref;
    PoliciesBackstage.setDefaultPref = this.hoistedSetDefaultPref.bind(this);
  },

  stop() {
    this.restoreDefaultValues();

    let PoliciesBackstage = ChromeUtils.import(
      "resource:///modules/policies/Policies.jsm",
      null
    );
    PoliciesBackstage.setDefaultPref = this._originalFunc;
    this._originalFunc = null;
  },
```
This is what my `browser/components/enterprisepolicies/Policies.jsm` file is looking like:

Comment 14

[[e-mail]]

(In reply to [e-mail] from comment #13)

(In reply to Mike Kaply [:mkaply] from comment #11)

Is it possible your machine is using policies? What do you see when you go to about:policies?

I'm using FIrefox Developer Edition in Ubuntu 21.10 and it says the
enterprise policies services is deactivated.

(In reply to Mark Banner (:standard8) from comment #12)

Can you also post your patch so far? It might make it easier for us to see any issues.

Sure, no problem. I redid all the importing and removed each
ChromeUtils.import(..., null) import listed in the bug description and
managed to track what was happening. In
toolkit/components/enterprisepolicies/tests/EnterprisePolicyTesting.jsm,
I'm getting errors when removing the null parts in var PoliciesPrefTracker:

  start() {
    let PoliciesBackstage = ChromeUtils.import(
      "resource:///modules/policies/Policies.jsm",
      null
    );
    this._originalFunc = PoliciesBackstage.setDefaultPref;
    PoliciesBackstage.setDefaultPref = this.hoistedSetDefaultPref.bind(this);
  },

  stop() {
    this.restoreDefaultValues();

    let PoliciesBackstage = ChromeUtils.import(
      "resource:///modules/policies/Policies.jsm",
      null
    );
    PoliciesBackstage.setDefaultPref = this._originalFunc;
    this._originalFunc = null;
  },

This is what my browser/components/enterprisepolicies/Policies.jsm file is
looking like:

Ok, so I didn't figure out a way to remove the null parts from that piece of code without breaking everything, I could really use some guidance.

Comment 15

[Mark Banner (:standard8)]

Please can you submit your patch so far to phabricator. It helps a lot if we can comment on the code directly.

Also, have a look through some of the patches of the resolved bugs on https://bugzilla.mozilla.org/showdependencytree.cgi?id=1609271&hide_resolved=0 - they may give pointers to what you need to do.

Comment 16

[[e-mail]]

Attachment: Bug 1733549 - Remove uses of ChromeUtils.import(..., null) in enterprise policy code. r?standard8

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4f862d825ca
Remove uses of ChromeUtils.import(..., null) in enterprise policy code. r=Standard8,mkaply

Comment 18

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/c4f862d825ca

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2133b49b165b
Port bug 1733549 - Remove uses of ChromeUtils.import(..., null) in enterprise policy code. rs=bustage-fix