1514594 - Stop returning the JSM global from ChromeUtils.import()

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P3)

Description

[Tomislav Jovanovic :zombie]

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

[Ted Campbell [:tcampbell]]

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.

Comment 2

[Tomislav Jovanovic :zombie]

Attachment: Bug 1514594 - Detect un-exported symbols from JSM globals used via Cu.import() return value

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

Comment 3

[Tomislav Jovanovic :zombie]

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.

Comment 4

[Kris Maglione [:kmag]]

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.

Comment 5

[Tomislav Jovanovic :zombie]

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

[:Gijs (he/him)]

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

Comment 7

[Tomislav Jovanovic :zombie]

(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

[:Gijs (he/him)]

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

Comment 9

[Tomislav Jovanovic :zombie]

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

Comment 10

[Tomislav Jovanovic :zombie]

Attachment: unexported.csv

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 1 - Add JS_AssignObject method. r=tcampbell

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

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 2 - Update modules.json. r=Gijs

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.

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 3a - Change ChromeUtils.import to return an exports object; not pollute global. r=mccr8

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.

Comment 14

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 3c - Update ESLint plugin for ChromeUtils.import API changes. r=Standard8

Comment 15

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 3d - Remove thousands of duplicate imports from sync tests. r=Gijs

Comment 16

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 3e - Remove no-op ChromeUtils.import() calls. r=Gijs

Comment 17

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 3f.2 - Cleanup various non-test corner cases after mass rewrite. r=Gijs

Comment 18

[Kris Maglione [:kmag]]

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

Comment 20

[Kris Maglione [:kmag]]

Attachment: Bug 1514594: Part 3f.1 - Cleanup various test corner cases after mass rewrite. r=Gijs

Comment 21

[Kris Maglione [:kmag]]

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

Comment 22

[Kris Maglione [:kmag]]

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

[:Gijs (he/him)]

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

Comment 24

[Kris Maglione [:kmag]]

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.

Comment 25

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a5acf832e58a25a2f4a041f10c9c4752d6ff002
Bug 1514594: Follow-up: Fix ESLint error. r=bustage CLOSED TREE

Comment 26

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/abb20e21a1bb3d744acacd2961e98cff364f6e3f
Bug 1514594: Follow-up: Fix merge bustage in test. r=bustage

Comment 27

[Kris Maglione [:kmag]]

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

Comment 28

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3edf05dad0576ebf2bad0e1678ed57127381d84d
Bug 1514594: Follow-up: Fix Mac merge bustage. r=bustage

Comment 29

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/92c2bce023f46a30308c68a0fbb31d65f72abe51
Bug 1514594: Follow-up: Fix merge bustage in another test. r=bustage

Comment 30

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6528b01b83d071af1b6133a19d4050d7d0647ea
Bug 1514594: Follow-up: Fix pageloader talos tests. r=bustage

Comment 31

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/292f1ecddf1686be768b81333ae1559321ebc38c
Bug 1514594: Follow-up: Fix damp talos tests. r=bustage

Comment 32

[Kris Maglione [:kmag]]

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

[Cosmin Sabou [:CosminS]]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/815f7149b30b
Follow-up: Remove unused imports merged from autoland. a=eslint-fix

Comment 35

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/4746

Comment 37

[[github robot]]

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

[Ted Campbell [:tcampbell]]

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

Comment 39

[Dave Townsend [:mossop]]

This isn't a DOM bug.