Closed Bug 1415483 Opened 4 years ago Closed 3 years ago

Write a rule to find and prevent unnecessary usages of Cu.importGlobalProperties

Categories

(Firefox Build System :: Lint and Formatting, enhancement, P3)

3 Branch
enhancement

Tracking

(firefox66 fixed)

RESOLVED FIXED
Tracking Status
firefox66 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

Cu.importGlobalProperties takes an array of strings that are globals to import.

Over time, some of the properties have been defined in the global scope for jsms via webidls exposed to the system, or by default.

Hence, we should be able to detect those and remove the unnecessary calls.

We might need to have a whitelist of interfaces already exposed to the System, or a way to manually generate that list occasionally.

Existing exported interfaces:

By default: http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/js/xpconnect/loader/mozJSComponentLoader.cpp#134-140

By webidl:
http://searchfox.org/mozilla-central/search?q=%3DSystem&case=false&regexp=false&path=webidl
http://searchfox.org/mozilla-central/search?q=%2CSystem&case=false&regexp=false&path=webidl
Blocks: 1369722
Realistically, the way to get an exhaustive list of system-exposed webidl is by looking at the generated code in <http://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/ResolveSystemBinding.cpp#57>.  The queries above sort of aim to catch most of the cases, but could miss some or have false positives.

The other thing to keep in mind is that Cu.importGlobalProperties can be used in any global, including sandboxes, iirc, where we don't define System webidl bits by default.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #1)
> Realistically, the way to get an exhaustive list of system-exposed webidl is
> by looking at the generated code in
> <http://searchfox.org/mozilla-central/source/__GENERATED__/dom/bindings/
> ResolveSystemBinding.cpp#57>.  The queries above sort of aim to catch most
> of the cases, but could miss some or have false positives.

Thank you for that.

> The other thing to keep in mind is that Cu.importGlobalProperties can be
> used in any global, including sandboxes, iirc, where we don't define System
> webidl bits by default.

Yeah, I meant to limit this to jsm files only (for now at least).
Summary: Write a rule to find and prevent unnecessary usages of Cu.importGlobalProperties → Write a rule to find and prevent unnecessary usages of Cu.importGlobalProperties in jsm files
Product: Testing → Firefox Build System
This may now not be necessary given bug 1501127.
Depends on: 1501127
Following discussions on other bugs with Nika, I've realised that enabling a basic rule across the tree will help move bug 1501127 and clarify what's happening.

The basic idea of the rule I'm proposing here is to disallow calling Cu.importGlobalProperties for items that are in our privileged file, this should catch most of the instances that are currently exposed.

https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/tools/lint/eslint/eslint-plugin-mozilla/lib/environments/privileged.js

I'll add some more details on the current status to bug 1501127 as well.
Assignee: nobody → standard8
Blocks: 1501127
No longer depends on: 1501127
Summary: Write a rule to find and prevent unnecessary usages of Cu.importGlobalProperties in jsm files → Write a rule to find and prevent unnecessary usages of Cu.importGlobalProperties
Note: I'm not going to land this until after the code freeze and 65 has branched, just in case there's something that tests haven't spotted.
The minor update to the patch was because I'd find a few more instances that should be tidied up.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8dea94258f54
Extend reject-importGlobalProperties to reject any priviliged items already in scope. r=mossop
https://hg.mozilla.org/integration/autoland/rev/d00748de66fc
Apply the new options to reject-importGlobalProperties across the codebase, remove unnecessary importGlobalProperties. r=nika
Backed out for multiple failures e.g on /test_message_manager_ipc.html 

backout: https://hg.mozilla.org/integration/autoland/rev/546d25793dae31f48c220842163664034fd8e769

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=d00748de66fcf465e872d412c4b974aeff038605&group_state=expanded

FAILURE LOGS:

1. dom/indexedDB/test/test_message_manager_ipc.html : https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216372735&repo=autoland&lineNumber=1641

2.sessionrestore | application crashed [unknown top frame] : https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216377191&repo=autoland&lineNumber=2247

3.tier2 TV browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js - https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=216383439&repo=autoland&lineNumber=1194
Flags: needinfo?(standard8)
The browser/base/content/test/siteIdentity/browser_secure_transport_insecure_scheme.js isn't anything to do with this - that's bug 1498333.

The other two are probably just because we're taking too much out.
Flags: needinfo?(standard8)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/85f261cb71cf
Extend reject-importGlobalProperties to reject any priviliged items already in scope. r=mossop
https://hg.mozilla.org/integration/autoland/rev/2d59d8c885a3
Apply the new options to reject-importGlobalProperties across the codebase, remove unnecessary importGlobalProperties. r=nika
Version: Version 3 → 3 Branch
Depends on: 1518969
Blocks: 1646183
You need to log in before you can comment on or make changes to this bug.