Stop returning the JSM global from ChromeUtils.import()

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: zombie, Assigned: kmag)

Tracking

(Depends on 1 bug, Blocks 3 bugs)

65 Branch
Firefox 67
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(10 attachments, 1 obsolete attachment)

27.76 KB, application/vnd.ms-excel
Details
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
52 bytes, text/x-github-pull-request
Details | Review
Reporter

Description

6 months ago
One step toward aligning our module system with ES modules, required for bug 1432901.  

Since this will require refactoring/removing all uses first, this can be a meta bug.
Kris, it seems like you are started some of these changes (or similar?). Mind giving some comments/bugs for what is in flight? :zombie has offered to help with fixing up tests and other JS-related changes needed.
Flags: needinfo?(kmaglione+bmo)
Reporter

Comment 2

6 months ago
Bug 1514594 - Detect un-exported symbols from JSM globals used via Cu.import() return value
Reporter

Comment 3

6 months ago
The above is based on Dave's idea to use a Proxy and instrument all property reads from the JSM global returned by Cu.import().

It seems to work for mochitest: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217307023&repo=try&lineNumber=1440-1540

But xpcshell tests only print to log in case of a failure, and I can't figure out how to either:
 - log a test failure from inside xpcomponent js (without throwing/aborting the test), or
 - turn verbose mode for all xpcshell tests in tree.

The second should probably be doable via passing some secret ENV variable, or patching the test runner default somewhere, but I'll need to find someone who knows something about that.
Assignee

Comment 4

6 months ago
My plan is to have ChromeUtils.import() return an exports object rather than a "global" unless `null` is passed as a second arg, and rewrite existing callers to destructure the result rather than rely on it setting properties on its global. I have some WIP scripts for that.
Flags: needinfo?(kmaglione+bmo)
Reporter

Comment 5

6 months ago
I managed to force verbose output, and to extract the list of unexported symbols accessed during the xpcshell, mochitest and marionette test by downloading the logs and running something like:

> egrep -oh "\|\|UNEXPORTED_SYMBOL\|.+\|.+\|\|" * | sort | uniq | awk -F "|" '{print $5 " " $4}' | sort > unexported.txt

It possibly missed some because of a few test failures, and there are a few false positives because of the use of `const`, but this should be the bulk of it:


    chrome://marionette/content/prefs.js EnvironmentPrefs
    resource:///modules/CustomizableUI.jsm CustomizableUIInternal
    resource:///modules/CustomizableUI.jsm gAreas
    resource:///modules/CustomizableUI.jsm gFuturePlacements
    resource:///modules/CustomizableUI.jsm gPalette
    resource:///modules/CustomizableUI.jsm gPlacements
    resource:///modules/CustomizableUI.jsm gSavedState
    resource:///modules/CustomizableUI.jsm gSeenWidgets
    resource:///modules/CustomizableUI.jsm kVersion
    resource:///modules/LiveBookmarkMigrator.jsm OS
    resource:///modules/policies/Policies.jsm runOnce
    resource:///modules/policies/Policies.jsm setAndLockPref
    resource:///modules/policies/Policies.jsm setDefaultPref
    resource:///modules/policies/ProxyPolicies.jsm PROXY_TYPES_MAP
    resource:///modules/syncedtabs/SyncedTabsDeckView.js DeckView
    resource:///modules/syncedtabs/TabListView.js View
    resource:///modules/translation/LanguageDetector.jsm IDLE_TIMEOUT
    resource:///modules/translation/LanguageDetector.jsm workerManager
    resource://devtools/shared/Loader.jsm Services
    resource://devtools/shared/base-loader.js parseStack
    resource://devtools/shared/base-loader.js serializeStack
    resource://devtools/shared/worker/worker.js workerify
    resource://formautofill/FormAutofillParent.jsm FormAutofillParent
    resource://formautofill/FormAutofillStorage.jsm FormAutofillStorage
    resource://formautofill/FormAutofillSync.jsm AutofillRecord
    resource://formautofill/FormAutofillSync.jsm sanitizeStorageObject
    resource://gre/actors/SelectChild.jsm SelectContentHelper
    resource://gre/modules/BackgroundPageThumbs.jsm BackgroundPageThumbs
    resource://gre/modules/Battery.jsm Debugging
    resource://gre/modules/CloudStorage.jsm CloudStorageInternal
    resource://gre/modules/CrashManager.jsm CrashStore
    resource://gre/modules/CrashManager.jsm dateToDays
    resource://gre/modules/CrashManager.jsm gCrashManager
    resource://gre/modules/EventPing.jsm Policy
    resource://gre/modules/Extension.jsm GlobalManager
    resource://gre/modules/Extension.jsm Management
    resource://gre/modules/ExtensionChild.jsm ExtensionManager
    resource://gre/modules/ExtensionContent.jsm DocumentManager
    resource://gre/modules/ExtensionStorageSync.jsm CollectionKeyEncryptionRemoteTransformer
    resource://gre/modules/ExtensionStorageSync.jsm CryptoCollection
    resource://gre/modules/ExtensionStorageSync.jsm EncryptionRemoteTransformer
    resource://gre/modules/ExtensionStorageSync.jsm FirefoxAdapter
    resource://gre/modules/ExtensionStorageSync.jsm KeyRingEncryptionRemoteTransformer
    resource://gre/modules/ExtensionStorageSync.jsm idToKey
    resource://gre/modules/ExtensionStorageSync.jsm keyToId
    resource://gre/modules/FxAccounts.jsm AccountState
    resource://gre/modules/FxAccountsStorage.jsm LoginManagerStorage
    resource://gre/modules/FxAccountsWebChannel.jsm FxAccountsWebChannel
    resource://gre/modules/FxAccountsWebChannel.jsm FxAccountsWebChannelHelpers
    resource://gre/modules/HealthPing.jsm Policy
    resource://gre/modules/LoginManagerContent.jsm LoginUtils
    resource://gre/modules/PerformanceStats.jsm performanceStatsService
    resource://gre/modules/PushBroadcastService.jsm BroadcastService
    resource://gre/modules/PushCrypto.jsm CryptoError
    resource://gre/modules/PushCrypto.jsm getCryptoParams
    resource://gre/modules/PushCrypto.jsm getCryptoParamsFromHeaders
    resource://gre/modules/PushService.jsm PushDB
    resource://gre/modules/PushService.jsm PushServiceHttp2
    resource://gre/modules/PushService.jsm PushServiceWebSocket
    resource://gre/modules/Subprocess.jsm SubprocessImpl
    resource://gre/modules/TelemetryController.jsm Policy
    resource://gre/modules/TelemetryEnvironment.jsm Policy
    resource://gre/modules/TelemetryReportingPolicy.jsm Policy
    resource://gre/modules/TelemetryReportingPolicy.jsm TelemetryReportingPolicyImpl
    resource://gre/modules/TelemetrySend.jsm PING_SUBMIT_TIMEOUT_MS
    resource://gre/modules/TelemetrySend.jsm Policy
    resource://gre/modules/TelemetrySend.jsm SendScheduler
    resource://gre/modules/TelemetrySend.jsm TelemetrySendImpl
    resource://gre/modules/TelemetrySend.jsm gzipCompressString
    resource://gre/modules/TelemetrySession.jsm Policy
    resource://gre/modules/TelemetrySession.jsm TelemetryScheduler
    resource://gre/modules/TelemetryStorage.jsm Policy
    resource://gre/modules/WebNavigation.jsm Manager
    resource://gre/modules/WebRequest.jsm WebRequest
    resource://gre/modules/addons/GMPProvider.jsm EME_ADOBE_ID
    resource://gre/modules/addons/GMPProvider.jsm GMPInstallManager
    resource://gre/modules/addons/GMPProvider.jsm GMPPrefs
    resource://gre/modules/addons/GMPProvider.jsm GMPProvider
    resource://gre/modules/addons/GMPProvider.jsm GMP_PLUGINS
    resource://gre/modules/addons/GMPProvider.jsm WIDEVINE_ID
    resource://gre/modules/addons/XPIProvider.jsm XPIDatabase
    resource://gre/modules/addons/XPIProvider.jsm XPIStates
    resource://gre/modules/osfile/osfile_async_front.jsm Scheduler
    resource://gre/modules/subprocess/subprocess_unix.jsm libc
    resource://normandy/actions/ShowHeartbeatAction.jsm Heartbeat
    resource://services-common/blocklist-clients.js AddonBlocklistClient
    resource://services-common/blocklist-clients.js GfxBlocklistClient
    resource://services-common/blocklist-clients.js OneCRLBlocklistClient
    resource://services-common/blocklist-clients.js PinningBlocklistClient
    resource://services-common/blocklist-clients.js PluginBlocklistClient
    resource://services-sync/SyncDisconnect.jsm SyncDisconnectInternal
    resource://services-sync/browserid_identity.js telemetryHelper
    resource://services-sync/doctor.js REPAIR_ADVANCE_PERIOD
    resource://services-sync/record.js PostQueue
    resource://testing-common/AddonTestUtils.jsm escaped

Comment 6

6 months ago
(In reply to Tomislav Jovanovic :zombie from comment #5)
> I managed to force verbose output, and to extract the list of unexported
> symbols accessed during the xpcshell, mochitest and marionette test by
> downloading the logs and running something like:
> 
> > egrep -oh "\|\|UNEXPORTED_SYMBOL\|.+\|.+\|\|" * | sort | uniq | awk -F "|" '{print $5 " " $4}' | sort > unexported.txt
> 
> It possibly missed some because of a few test failures, and there are a few
> false positives because of the use of `const`, but this should be the bulk
> of it:


Do you still have the original list/output, and if so, can we re-generate the list but include the path to the test that actually touches the symbol(s) ? That'd simplify writing patches to fix them.

Updated

6 months ago
Blocks: 1515056
Reporter

Comment 7

6 months ago
(In reply to :Gijs (he/him) from comment #6)
> Do you still have the original list/output, and if so, can we re-generate
> the list but include the path to the test that actually touches the
> symbol(s) ? That'd simplify writing patches to fix them.

The current logs don't include the caller information, I need to add it and run the tests again.

Which is the easy part, the tedious part being downloading all the logs from a Try run, which I need to figure out how to automate, since I don't want to do it manually again.  ;)

(I'll get back to this before Friday, unless you want to do it sooner.)

Comment 8

6 months ago
(In reply to Tomislav Jovanovic :zombie from comment #7)
> (In reply to :Gijs (he/him) from comment #6)
> > Do you still have the original list/output, and if so, can we re-generate
> > the list but include the path to the test that actually touches the
> > symbol(s) ? That'd simplify writing patches to fix them.
> 
> The current logs don't include the caller information, I need to add it and
> run the tests again.

I'm confused; the logs have the test names that start/end, and your log info is presumably interspersed with the test names, so you can infer from that, right?
Reporter

Comment 9

6 months ago
Inferring is not reliable for xpcshell tests run in parallel, and also not all uses are in test code, so that that wouldn't account for everything.

So I added callers to the log, and luckily found Eric's Log Spam Hell https://github.com/EricRahm/log-spam-hell
Reporter

Comment 10

6 months ago
Posted file unexported.csv
Reporter

Updated

5 months ago
Blocks: 1517952
Assignee

Updated

5 months ago
Depends on: 1519596
Assignee

Updated

5 months ago
Assignee: nobody → kmaglione+bmo
Priority: -- → P3
Assignee

Comment 11

5 months ago
This lets us do the approximate equivalent of Object.assign from native code,
using the optimized implementation of that method when possible.
Assignee

Comment 12

5 months ago
This adds some missing modules with odd export patterns, along with some full
path overrides for a few modules with duplicate leaf names, which previously
had a single entry with combined export lists of all matching modules.
Assignee

Comment 13

5 months ago
This changes the behavior of ChromeUtils.import() to return an exports object,
rather than a module global, in all cases except when `null` is passed as a
second argument, and changes the default behavior not to pollute the global
scope with the module's exports. Thus, the following code written for the old
model:

  ChromeUtils.import("resource://gre/modules/Services.jsm");

is approximately the same as the following, in the new model:

  var {Services} = ChromeUtils.import("resource://gre/modules/Services.jsm");

Since the two behaviors are mutually incompatible, this patch will land with a
scripted rewrite to update all existing callers to use the new model rather
than the old.
Assignee

Comment 18

5 months ago

Well, Phabricator crashes when I try to submit the actual mass rewrite part. Here it is for reference: https://gist.github.com/ee5041b717b0025828b572aeee89159d

Reporter

Updated

5 months ago
Duplicate of this bug: 1517952
Depends on: 1520643
Attachment #9037060 - Attachment description: Bug 1514594: Part 3c - Update ESLint plugin for ChromeUtils.import API changes. r=Gijs → Bug 1514594: Part 3c - Update ESLint plugin for ChromeUtils.import API changes. r=Standard8
Attachment #9031758 - Attachment is obsolete: true
Attachment #9037063 - Attachment description: Bug 1514594: Part 3f - Cleanup various corner cases after mass rewrite. r=Gijs → Bug 1514594: Part 3f.2 - Cleanup various non-test corner cases after mass rewrite. r=Gijs
Assignee

Comment 21

5 months ago

(In reply to Kris Maglione [:kmag] from comment #18)

Well, Phabricator crashes when I try to submit the actual mass rewrite part. Here it is for reference: https://gist.github.com/ee5041b717b0025828b572aeee89159d

Gijs, can you rubber stamp this? Sorry, I don't have a good way to upload it for a proper review given that phabricator keeps crashing.

Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 22

5 months ago

Also, note I'm planning to squash all of the Part 3* patches into a single revision to land, so things don't blow up if someone bisects.

Comment 23

5 months ago

(In reply to Kris Maglione [:kmag] from comment #21)

(In reply to Kris Maglione [:kmag] from comment #18)

Well, Phabricator crashes when I try to submit the actual mass rewrite part. Here it is for reference: https://gist.github.com/ee5041b717b0025828b572aeee89159d

Gijs, can you rubber stamp this? Sorry, I don't have a good way to upload it for a proper review given that phabricator keeps crashing.

rs=me . Thanks for doing this.

Flags: needinfo?(gijskruitbosch+bugs)
Assignee

Comment 27

5 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4bc770419d00d9286601bb81212479ed13c2eb
Bug 1514594: Follow-up: Fix ESLint bustage on merge from autoland. r=bustage

Updated

5 months ago
Blocks: 1523488
Assignee

Comment 32

5 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a4d0476fae4c561b80766e0bc47c156ea63f0071
Bug 1514594: Follow-up: Fix more Mac bustage in the same script. r=bustage

Comment 34

5 months ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/815f7149b30b
Follow-up: Remove unused imports merged from autoland. a=eslint-fix
Depends on: 1523594
Duplicate of this bug: 1523739

Comment 37

5 months ago
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/01a4d1659bfe76763250f6d6756b67ae3d4d66d2
Port Bug 1514594: Part 3a - Change ChromeUtils.import to return an exports object; not pollute global. r=mccr8 (#4746)

Update to eslint-plugin-mozilla@1.1.0, babel-plugin-jsm-to-commonjs@0.5.0 and babel-plugin-jsm-to-esmodules@0.6.0. Fix up eslint and general test failures to work with locally scoped imports.
Depends on: 1523808
Reporter

Updated

5 months ago
Blocks: 1524027
No longer depends on: 1523594

Kris, can you close out the phabricator reviews. Thanks.

Updated

4 months ago
Depends on: 1525200
Depends on: 1526981
Depends on: 1528760
You need to log in before you can comment on or make changes to this bug.