Closed Bug 1514594 Opened 6 years ago Closed 6 years ago

Stop returning the JSM global from ChromeUtils.import()

Categories

(Firefox :: General, enhancement, P3)

65 Branch
enhancement

Tracking

()

RESOLVED FIXED
Tracking Status
firefox67 --- fixed

People

(Reporter: zombie, Assigned: kmag)

References

(Blocks 1 open bug)

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

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)
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
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.
Blocks: 1524027
No longer depends on: 1523594

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

Depends on: 1526981
Depends on: 1528760
Component: General → DOM: Core & HTML
Product: Firefox → Core
Target Milestone: Firefox 67 → ---

This isn't a DOM bug.

Component: DOM: Core & HTML → General
Product: Core → Firefox
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: