Closed
Bug 1415483
Opened 7 years ago
Closed 6 years ago
Write a rule to find and prevent unnecessary usages of Cu.importGlobalProperties
Categories
(Developer Infrastructure :: Lint and Formatting, enhancement, P3)
Tracking
(firefox66 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: standard8, Assigned: standard8)
References
(Blocks 1 open bug)
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®exp=false&path=webidl
http://searchfox.org/mozilla-central/search?q=%2CSystem&case=false®exp=false&path=webidl
Comment 1•7 years ago
|
||
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.
Assignee | ||
Comment 2•7 years ago
|
||
(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
Updated•7 years ago
|
Product: Testing → Firefox Build System
Assignee | ||
Comment 3•6 years ago
|
||
This may now not be necessary given bug 1501127.
Depends on: 1501127
Assignee | ||
Comment 4•6 years ago
|
||
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 | ||
Comment 5•6 years ago
|
||
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.
Assignee | ||
Comment 6•6 years ago
|
||
Assignee | ||
Comment 7•6 years ago
|
||
Depends on D13753
Assignee | ||
Comment 8•6 years ago
|
||
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
Comment 10•6 years ago
|
||
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)
Assignee | ||
Comment 11•6 years ago
|
||
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)
Comment 12•6 years ago
|
||
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
Comment 13•6 years ago
|
||
bugherder |
Updated•6 years ago
|
Version: Version 3 → 3 Branch
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•