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

RESOLVED FIXED

Status

enhancement
P3
normal
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

(Blocks 1 bug)

3 Branch
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
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.
(Assignee)

Comment 2

2 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

a year ago
Product: Testing → Firefox Build System
(Assignee)

Comment 3

6 months ago
This may now not be necessary given bug 1501127.
Depends on: 1501127
(Assignee)

Comment 4

5 months 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: 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
(Assignee)

Comment 5

5 months 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 8

4 months ago
The minor update to the patch was because I'd find a few more instances that should be tidied up.

Comment 9

4 months ago
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)
(Assignee)

Comment 11

4 months 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

4 months 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

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/85f261cb71cf
https://hg.mozilla.org/mozilla-central/rev/2d59d8c885a3
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Version: Version 3 → 3 Branch
Depends on: 1518969
You need to log in before you can comment on or make changes to this bug.