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

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, enhancement, P3)

Description

[Mark Banner (:standard8)]

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Mark Banner (:standard8)]

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

Comment 3

[Mark Banner (:standard8)]

This may now not be necessary given bug 1501127.

Comment 4

[Mark Banner (:standard8)]

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.

Comment 5

[Mark Banner (:standard8)]

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.

Comment 6

[Mark Banner (:standard8)]

Attachment: Bug 1415483 - Extend reject-importGlobalProperties to reject any priviliged items already in scope. r?mossop

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1415483 - Apply the new options to reject-importGlobalProperties across the codebase, remove unnecessary importGlobalProperties. r?nika

Depends on D13753

Comment 8

[Mark Banner (:standard8)]

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

[Natalia Csoregi [:nataliaCs]]

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

Comment 11

[Mark Banner (: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.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/85f261cb71cf
https://hg.mozilla.org/mozilla-central/rev/2d59d8c885a3