Closed Bug 1733549 Opened 2 months ago Closed 1 month ago

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

Categories

(Firefox :: Enterprise Policies, task)

task

Tracking

()

RESOLVED FIXED
96 Branch
Tracking Status
firefox96 --- fixed

People

(Reporter: standard8, Assigned: raquelvargas, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(3 files)

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.
Keywords: good-first-bug
Blocks: 1609271

Can I be assigned this task?

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

(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.

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

Flags: needinfo?(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.

Flags: needinfo?(standard8)

Thank you very much Mark.

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

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?

Flags: needinfo?(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 ....

Flags: needinfo?(standard8)
Attached file 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: 

```

```

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.

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

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

Attached file 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:

(In reply to raquelvargas@gmail.com 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.

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.

Assignee: nobody → raquelvargas
Status: NEW → ASSIGNED
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c4f862d825ca
Remove uses of ChromeUtils.import(..., null) in enterprise policy code. r=Standard8,mkaply
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 96 Branch
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/2133b49b165b
Port bug 1733549 - Remove uses of ChromeUtils.import(..., null) in enterprise policy code. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.