Closed Bug 1514594 Opened 2 years ago Closed 2 years ago

Stop returning the JSM global from ChromeUtils.import()

Categories

(Firefox :: General, enhancement, P3)

65 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 67
Tracking Status
firefox67 --- fixed

People

(Reporter: zombie, Assigned: kmag)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(10 files, 1 obsolete file)

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
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)
Bug 1514594 - Detect un-exported symbols from JSM globals used via Cu.import() return value
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.
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)
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
(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.
Blocks: 1515056
(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.)
(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?
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
Attached file unexported.csv
Blocks: 1517952
Depends on: 1519596
Assignee: nobody → kmaglione+bmo
Priority: -- → P3
This lets us do the approximate equivalent of Object.assign from native code,
using the optimized implementation of that method when possible.
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.
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.

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

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

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

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.

(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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4bc770419d00d9286601bb81212479ed13c2eb
Bug 1514594: Follow-up: Fix ESLint bustage on merge from autoland. r=bustage
Blocks: 1523488
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
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
Blocks: 1524027
No longer depends on: 1523594

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

Depends on: 1526981
You need to log in before you can comment on or make changes to this bug.