Stop returning the JSM global from ChromeUtils.import()
Categories
(Firefox :: General, enhancement, P3)
Tracking
()
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.
Comment 1•5 years ago
|
||
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.
Reporter | ||
Comment 2•5 years ago
|
||
Bug 1514594 - Detect un-exported symbols from JSM globals used via Cu.import() return value
Reporter | ||
Comment 3•5 years 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•5 years 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.
Reporter | ||
Comment 5•5 years 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•5 years 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.
Reporter | ||
Comment 7•5 years 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•5 years 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•5 years 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•5 years ago
|
||
Assignee | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 11•5 years 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 years 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 years 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 14•5 years ago
|
||
Assignee | ||
Comment 15•5 years ago
|
||
Assignee | ||
Comment 16•5 years ago
|
||
Assignee | ||
Comment 17•5 years ago
|
||
Assignee | ||
Comment 18•5 years ago
|
||
Well, Phabricator crashes when I try to submit the actual mass rewrite part. Here it is for reference: https://gist.github.com/ee5041b717b0025828b572aeee89159d
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 20•5 years ago
|
||
Updated•5 years ago
|
Assignee | ||
Comment 21•5 years 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.
Assignee | ||
Comment 22•5 years 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 years 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.
Assignee | ||
Comment 24•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b9aff104cc714f0008ce9b91a8b23bca1e830c3 Bug 1514594: Part 1 - Add JS_AssignObject method. r=tcampbell https://hg.mozilla.org/integration/mozilla-inbound/rev/de28d9b122485e915cb4b63254c99fb13b8000c9 Bug 1514594: Part 2 - Update modules.json. r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/6b56696d713a7f7858f16235e37baa8307e73b49 Bug 1514594: Part 3 - Change ChromeUtils.import API.
Assignee | ||
Comment 25•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5acf832e58a25a2f4a041f10c9c4752d6ff002 Bug 1514594: Follow-up: Fix ESLint error. r=bustage CLOSED TREE
Assignee | ||
Comment 26•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/abb20e21a1bb3d744acacd2961e98cff364f6e3f Bug 1514594: Follow-up: Fix merge bustage in test. r=bustage
Assignee | ||
Comment 27•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4bc770419d00d9286601bb81212479ed13c2eb Bug 1514594: Follow-up: Fix ESLint bustage on merge from autoland. r=bustage
Assignee | ||
Comment 28•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/3edf05dad0576ebf2bad0e1678ed57127381d84d Bug 1514594: Follow-up: Fix Mac merge bustage. r=bustage
Assignee | ||
Comment 29•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/92c2bce023f46a30308c68a0fbb31d65f72abe51 Bug 1514594: Follow-up: Fix merge bustage in another test. r=bustage
Assignee | ||
Comment 30•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6528b01b83d071af1b6133a19d4050d7d0647ea Bug 1514594: Follow-up: Fix pageloader talos tests. r=bustage
Assignee | ||
Comment 31•5 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/292f1ecddf1686be768b81333ae1559321ebc38c Bug 1514594: Follow-up: Fix damp talos tests. r=bustage
Assignee | ||
Comment 32•5 years 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 33•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5b9aff104cc7
https://hg.mozilla.org/mozilla-central/rev/de28d9b12248
https://hg.mozilla.org/mozilla-central/rev/6b56696d713a
https://hg.mozilla.org/mozilla-central/rev/4a5acf832e58
https://hg.mozilla.org/mozilla-central/rev/abb20e21a1bb
https://hg.mozilla.org/mozilla-central/rev/1b4bc770419d
https://hg.mozilla.org/mozilla-central/rev/3edf05dad057
https://hg.mozilla.org/mozilla-central/rev/92c2bce023f4
https://hg.mozilla.org/mozilla-central/rev/d6528b01b83d
https://hg.mozilla.org/mozilla-central/rev/292f1ecddf16
https://hg.mozilla.org/mozilla-central/rev/a4d0476fae4c
Comment 34•5 years 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
Comment 35•5 years ago
|
||
Comment 37•5 years 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.
Comment 38•5 years ago
|
||
Kris, can you close out the phabricator reviews. Thanks.
Updated•2 years ago
|
Comment 39•2 years ago
|
||
This isn't a DOM bug.
Description
•