Stop assigning properties to the global `this` in JSMs - Round 2
Categories
(Firefox :: General, task, P5)
Tracking
()
Tracking | Status | |
---|---|---|
firefox103 | --- | fixed |
People
(Reporter: bgrins, Assigned: arai)
References
(Blocks 1 open bug)
Details
Attachments
(24 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
+++ This bug was initially created as a clone of Bug #1608278 +++
For example this.LoginBreaches = { ... }
: https://searchfox.org/mozilla-central/rev/ba4fab1cc2f1c9c4e07cdb71542b8d441707c577/browser/components/aboutlogins/LoginBreaches.jsm#25.
This should generally work by changing to const LoginBreaches = { ... }
I did some manual changes in Bug 1608278, so spinning out a new bug that depends on Bug 1608281 & Bug 1609271 to do some more automated changes.
Comment 1•4 years ago
|
||
The leave-open keyword is there and there is no activity for 6 months.
:Dolske, maybe it's time to close this bug?
Reporter | ||
Comment 2•4 years ago
|
||
The keyword was copied over from the cloned bug - clearing it
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 4•3 years ago
|
||
Depends on D144107
Assignee | ||
Comment 5•3 years ago
|
||
Depends on D144108
Assignee | ||
Comment 6•3 years ago
|
||
Depends on D144109
Assignee | ||
Comment 7•3 years ago
|
||
Depends on D144110
Assignee | ||
Comment 8•3 years ago
|
||
Depends on D144111
Assignee | ||
Comment 9•3 years ago
|
||
Depends on D144112
Assignee | ||
Comment 10•3 years ago
|
||
Depends on D144113
Assignee | ||
Comment 11•3 years ago
|
||
Depends on D144114
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D144115
Assignee | ||
Comment 13•3 years ago
|
||
Depends on D144116
Assignee | ||
Comment 14•3 years ago
|
||
Depends on D144117
Assignee | ||
Comment 15•3 years ago
|
||
Depends on D144118
Assignee | ||
Comment 16•3 years ago
|
||
Depends on D144119
Assignee | ||
Comment 17•3 years ago
|
||
Depends on D144120
Assignee | ||
Comment 18•3 years ago
|
||
Depends on D144121
Assignee | ||
Comment 19•3 years ago
|
||
Depends on D144122
Assignee | ||
Comment 20•3 years ago
|
||
Depends on D144123
Assignee | ||
Comment 21•3 years ago
|
||
Depends on D144124
Assignee | ||
Comment 22•3 years ago
|
||
Depends on D144125
Assignee | ||
Comment 23•3 years ago
|
||
Depends on D144126
Assignee | ||
Comment 24•3 years ago
|
||
Depends on D144127
Assignee | ||
Comment 25•3 years ago
|
||
Depends on D144128
Assignee | ||
Comment 26•3 years ago
|
||
Looks like AutoConfig script is also a consumer of Cu.import
and removing this.foo = foo;
in JSM here might affect it.
Assignee | ||
Comment 27•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #26)
Looks like AutoConfig script is also a consumer of
Cu.import
and removingthis.foo = foo;
in JSM here might affect it.
The change here can affect when AutoConfig script does var { foo } = Cu.import(...);
, and JSM does this.foo = foo;
.
Cu.import
returns the per-JSM global object, instead of object that has exported symbols and values,
and the global object doesn't have lexical variable as its properties.
So, removing this.foo = foo;
breaks it, and foo
in the AutoConfig results in undefined
.
If AutoConfig script does just Cu.import(".../Foo.jsm"); foo;
, or Cu.import(".../Foo.jsm", obj); obj.foo;
, there's no effect.
And thus it depends on how widely the return value of Cu.import
is used there.
Assignee | ||
Comment 28•3 years ago
|
||
I've looked around for a while, and I don't find any example provided for AutoConfig script that uses the return value of Cu.import
.
So the impact here might be small enough.
bug 1765167 still requires some investigation how much the deprecation warning message affects.
Assignee | ||
Comment 29•3 years ago
|
||
Here's the list of affected modules.
If there's code that receives the return value of Cu.import
for those modules and gets those properties, such code will stop working with these patches applied.
The list is long mostly because I choose const
as replacement for this.foo = ...
as much as possible.
Some case can be avoided just by using var
instead as replacement.
for other cases (mostly this.foo = foo
with lexical variable), we could change the original definition to var
.
Module | Symbols removed from Cu.import return value's property |
---|---|
chrome://remote/content/cdp/observers/ChannelEventSink.jsm |
ChannelEventSinkFactory |
chrome://remote/content/cdp/observers/NetworkObserver.jsm |
NetworkObserver |
chrome://remote/content/marionette/accessibility.js |
accessibility |
chrome://remote/content/marionette/action.js |
action |
chrome://remote/content/marionette/addon.js |
Addon |
chrome://remote/content/marionette/browser.js |
browser , Context , WindowState |
chrome://remote/content/marionette/capture.js |
capture |
chrome://remote/content/marionette/cert.js |
allowAllCerts |
chrome://remote/content/marionette/cookie.js |
cookie |
chrome://remote/content/marionette/dom.js |
WebElementEventTarget , ContentEventObserverService |
chrome://remote/content/marionette/driver.js |
GeckoDriver |
chrome://remote/content/marionette/element.js |
element , WebElement , ContentWebElement , ContentShadowRoot , ContentWebWindow , ContentWebFrame , ChromeWebElement |
chrome://remote/content/marionette/evaluate.js |
evaluate , sandbox , Sandboxes |
chrome://remote/content/marionette/event.js |
event |
chrome://remote/content/marionette/interaction.js |
interaction |
chrome://remote/content/marionette/l10n.js |
l10n |
chrome://remote/content/marionette/legacyaction.js |
legacyaction , action |
chrome://remote/content/marionette/message.js |
Message , Command , Response |
chrome://remote/content/marionette/modal.js |
modal |
chrome://remote/content/marionette/navigate.js |
navigate |
chrome://remote/content/marionette/permissions.js |
permissions |
chrome://remote/content/marionette/prefs.js |
MarionettePrefs , Branch , EnvironmentPrefs |
chrome://remote/content/marionette/reftest.js |
reftest |
chrome://remote/content/marionette/server.js |
TCPListener , TCPConnection |
chrome://remote/content/marionette/stream-utils.js |
StreamUtils |
chrome://remote/content/marionette/sync.js |
DebounceCallback |
chrome://remote/content/shared/PDF.jsm |
print |
chrome://remote/content/shared/messagehandler/Errors.jsm |
error |
chrome://remote/content/shared/webdriver/Assert.jsm |
assert |
chrome://remote/content/shared/webdriver/Capabilities.jsm |
Capabilities , PageLoadStrategy , Proxy , Timeouts , UnhandledPromptBehavior |
chrome://remote/content/shared/webdriver/Errors.jsm |
error |
chrome://remote/content/shared/webdriver/KeyData.jsm |
keyData |
resource:///modules/BrowserWindowTracker.jsm |
BrowserWindowTracker |
resource:///modules/ESEDBReader.jsm |
ESE , KERNEL , gLibs |
resource:///modules/EveryWindow.jsm |
EveryWindow |
resource:///modules/LoginBreaches.jsm |
LoginBreaches |
resource:///modules/OpenInTabsUtils.jsm |
OpenInTabsUtils |
resource:///modules/PingCentre.jsm |
PingCentreConstants , PingCentre |
resource:///modules/policies/BookmarksPolicies.jsm |
BookmarksPolicies |
resource://activity-stream/common/Actions.jsm |
MAIN_MESSAGE_TYPE , CONTENT_MESSAGE_TYPE , PRELOAD_MESSAGE_TYPE , UI_CODE , BACKGROUND_PROCESS , actionCreators , actionUtils , globalImportContext , actionTypes |
resource://activity-stream/common/Dedupe.jsm |
Dedupe |
resource://activity-stream/common/Reducers.jsm |
reducers , INITIAL_STATE , TOP_SITES_DEFAULT_ROWS , TOP_SITES_MAX_SITES_PER_ROW |
resource://activity-stream/lib/ASRouter.jsm |
ASRouter , MessageLoaderUtils , _ASRouter |
resource://activity-stream/lib/ASRouterPreferences.jsm |
ASRouterPreferences , _ASRouterPreferences , TEST_PROVIDERS , TARGETING_PREFERENCES |
resource://activity-stream/lib/ASRouterTargeting.jsm |
ASRouterTargeting , getSortedMessages , QueryCache , CachedTargetingGetter |
resource://activity-stream/lib/ASRouterTriggerListeners.jsm |
ASRouterTriggerListeners |
resource://activity-stream/lib/AboutPreferences.jsm |
AboutPreferences , PREFERENCES_LOADED_EVENT |
resource://activity-stream/lib/ActivityStream.jsm |
ActivityStream |
resource://activity-stream/lib/ActivityStreamMessageChannel.jsm |
ActivityStreamMessageChannel , DEFAULT_OPTIONS |
resource://activity-stream/lib/ActivityStreamPrefs.jsm |
Prefs , DefaultPrefs |
resource://activity-stream/lib/ActivityStreamStorage.jsm |
ActivityStreamStorage |
resource://activity-stream/lib/CFRMessageProvider.jsm |
CFRMessageProvider |
resource://activity-stream/lib/CFRPageActions.jsm |
PageAction , CFRPageActions |
resource://activity-stream/lib/DefaultSites.jsm |
DEFAULT_SITES |
resource://activity-stream/lib/DiscoveryStreamFeed.jsm |
DiscoveryStreamFeed |
resource://activity-stream/lib/DownloadsManager.jsm |
DownloadsManager |
resource://activity-stream/lib/FaviconFeed.jsm |
FaviconFeed |
resource://activity-stream/lib/HighlightsFeed.jsm |
HighlightsFeed |
resource://activity-stream/lib/InfoBar.jsm |
InfoBar |
resource://activity-stream/lib/LinksCache.jsm |
LinksCache |
resource://activity-stream/lib/MomentsPageHub.jsm |
MomentsPageHub , _MomentsPageHub |
resource://activity-stream/lib/NewTabInit.jsm |
NewTabInit |
resource://activity-stream/lib/OnboardingMessageProvider.jsm |
OnboardingMessageProvider |
resource://activity-stream/lib/PanelTestProvider.jsm |
PanelTestProvider |
resource://activity-stream/lib/PersistentCache.jsm |
PersistentCache |
resource://activity-stream/lib/PersonalityProvider/PersonalityProvider.jsm |
PersonalityProvider |
resource://activity-stream/lib/PlacesFeed.jsm |
PlacesFeed |
resource://activity-stream/lib/PrefsFeed.jsm |
PrefsFeed |
resource://activity-stream/lib/RecommendationProvider.jsm |
RecommendationProvider |
resource://activity-stream/lib/RemoteL10n.jsm |
RemoteL10n |
resource://activity-stream/lib/Screenshots.jsm |
Screenshots |
resource://activity-stream/lib/SearchShortcuts.jsm |
CUSTOM_SEARCH_SHORTCUTS , SEARCH_SHORTCUTS_EXPERIMENT , SEARCH_SHORTCUTS_SEARCH_ENGINES_PREF , SEARCH_SHORTCUTS_HAVE_PINNED_PREF , SEARCH_SHORTCUTS , getSearchProvider , getSearchFormURL , checkHasSearchEngine |
resource://activity-stream/lib/SectionsManager.jsm |
SectionsFeed , SectionsManager |
resource://activity-stream/lib/SnippetsTestMessageProvider.jsm |
SnippetsTestMessageProvider |
resource://activity-stream/lib/Spotlight.jsm |
Spotlight |
resource://activity-stream/lib/Store.jsm |
Store |
resource://activity-stream/lib/SystemTickFeed.jsm |
SystemTickFeed , SYSTEM_TICK_INTERVAL |
resource://activity-stream/lib/TelemetryFeed.jsm |
TelemetryFeed |
resource://activity-stream/lib/TippyTopProvider.jsm |
TippyTopProvider |
resource://activity-stream/lib/ToolbarBadgeHub.jsm |
ToolbarBadgeHub , _ToolbarBadgeHub |
resource://activity-stream/lib/ToolbarPanelHub.jsm |
ToolbarPanelHub , _ToolbarPanelHub |
resource://activity-stream/lib/TopSitesFeed.jsm |
TopSitesFeed , DEFAULT_TOP_SITES |
resource://activity-stream/lib/TopStoriesFeed.jsm |
TopStoriesFeed , STORIES_UPDATE_TIME , TOPICS_UPDATE_TIME , SECTION_ID , SPOC_IMPRESSION_TRACKING_PREF , REC_IMPRESSION_TRACKING_PREF , DEFAULT_RECS_EXPIRE_TIME |
resource://activity-stream/lib/UTEventReporting.jsm |
UTEventReporting |
resource://autofill/CreditCardRuleset.jsm |
creditCardRulesets |
resource://autofill/FormAutofillHeuristics.jsm |
FormAutofillHeuristics |
resource://autofill/FormAutofillStorage.jsm |
formAutofillStorage |
resource://autofill/FormAutofillTelemetryUtils.jsm |
CreditCardTelemetry |
resource://autofill/FormAutofillUtils.jsm |
FormAutofillUtils |
resource://gre/modules/Corroborate.jsm |
Corroborate |
resource://gre/modules/ExtensionPreferencesManager.jsm |
ExtensionPreferencesManager |
resource://gre/modules/ExtensionStorageIDB.jsm |
ExtensionStorageIDB |
resource://gre/modules/ExtensionStorageSyncKinto.jsm |
CryptoCollection , ExtensionStorageSync , EncryptionRemoteTransformer , KeyRingEncryptionRemoteTransformer , CollectionKeyEncryptionRemoteTransformer |
resource://gre/modules/FormHistory.jsm |
FormHistory |
resource://gre/modules/FxAccounts.jsm |
AccountState |
resource://gre/modules/FxAccountsPairing.jsm |
FxAccountsPairingFlow |
resource://gre/modules/FxAccountsWebChannel.jsm |
FxAccountsWebChannel , FxAccountsWebChannelHelpers |
resource://gre/modules/InsecurePasswordUtils.jsm |
InsecurePasswordUtils |
resource://gre/modules/LoginFormFactory.jsm |
LoginFormFactory |
resource://gre/modules/LoginHelper.jsm |
LoginHelper |
resource://gre/modules/LoginManagerChild.jsm |
LoginManagerChild |
resource://gre/modules/LoginManagerContextMenu.jsm |
LoginManagerContextMenu |
resource://gre/modules/LoginRecipes.jsm |
LoginRecipesContent |
resource://gre/modules/NewPasswordModel.jsm |
NewPasswordModel |
resource://gre/modules/PasswordGenerator.jsm |
PasswordGenerator |
resource://gre/modules/PictureInPictureControls.jsm |
KEYBOARD_CONTROLS , TOGGLE_POLICIES , TOGGLE_POLICY_STRINGS |
resource://gre/modules/Schemas.jsm |
Schemas |
resource://gre/modules/ServiceWorkerCleanUp.jsm |
ServiceWorkerCleanUp |
resource://gre/modules/SyncedBookmarksMirror.jsm |
SyncedBookmarksMirror |
resource://gre/modules/UrlClassifierExceptionListService.jsm |
UrlClassifierExceptionListService |
resource://gre/modules/UrlClassifierLib.jsm |
HTTP_FOUND , HTTP_SEE_OTHER , HTTP_TEMPORARY_REDIRECT |
resource://gre/modules/addons/XPIDatabase.jsm |
XPIDatabase , XPIDatabaseReconcile |
resource://gre/modules/handlers/HandlerList.jsm |
kHandlerListVersion , kHandlerList |
resource://gre/modules/reflect.jsm |
Reflect |
resource://services-common/kinto-storage-adapter.js |
FirefoxAdapter |
resource://services-common/uptake-telemetry.js |
UptakeTelemetry |
resource://services-sync/SyncDisconnect.jsm |
SyncDisconnectInternal , SyncDisconnect |
resource://services-sync/engines/prefs.js |
isAllowedPrefName |
Assignee | ||
Comment 30•3 years ago
|
||
bug 1768060 patch adds support for lexical variables in Cu.import
return value, so, if we can go with it, converting var
or this.foo = ...
with lexical won't break not-in-tree code
Assignee | ||
Comment 31•3 years ago
|
||
Now that bug 1768060 is fixed, we can use lexical variable from Cu.import
return value, and this.foo = foo
pattern can be removed.
Also all other this.foo = ...
pattern can be replaced with any kind of declaration, including let
, const
, and class
.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 32•3 years ago
|
||
Depends on D144129
Comment 33•3 years ago
|
||
How do you find all the cases? Just looking, or do you have some linting rules to help?
Assignee | ||
Comment 34•3 years ago
|
||
bug 1607331 has a patch that checks all global this
occurrences.
Assignee | ||
Comment 35•3 years ago
|
||
(In reply to Tooru Fujisawa [:arai] from comment #34)
bug 1607331 has a patch that checks all global
this
occurrences.
This bug is for mozilla/no-lexical-to-global
and mozilla/no-this-property-assign
rules.
Comment 36•2 years ago
|
||
Assignee | ||
Comment 37•2 years ago
|
||
Comment 38•2 years ago
|
||
Comment 39•2 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3e33a7869c38
https://hg.mozilla.org/mozilla-central/rev/55a37af2715c
https://hg.mozilla.org/mozilla-central/rev/117f88bb3fe2
https://hg.mozilla.org/mozilla-central/rev/993c2365e718
https://hg.mozilla.org/mozilla-central/rev/448c19f0852f
https://hg.mozilla.org/mozilla-central/rev/21f780633529
https://hg.mozilla.org/mozilla-central/rev/b85cdf82e653
https://hg.mozilla.org/mozilla-central/rev/fbc1b0f8822f
https://hg.mozilla.org/mozilla-central/rev/4b8af697e8f1
https://hg.mozilla.org/mozilla-central/rev/2cbdaa8212d6
https://hg.mozilla.org/mozilla-central/rev/37595d52b3b6
https://hg.mozilla.org/mozilla-central/rev/9a629a288cbe
https://hg.mozilla.org/mozilla-central/rev/17e7fa877bf9
https://hg.mozilla.org/mozilla-central/rev/5440a6b811a1
https://hg.mozilla.org/mozilla-central/rev/37c0be10fc9a
https://hg.mozilla.org/mozilla-central/rev/e7f336d03dc1
https://hg.mozilla.org/mozilla-central/rev/98d3d796d1ce
https://hg.mozilla.org/mozilla-central/rev/5f9e6effc07b
https://hg.mozilla.org/mozilla-central/rev/e53750149153
https://hg.mozilla.org/mozilla-central/rev/0aa523e6056c
https://hg.mozilla.org/mozilla-central/rev/18d0f182dae9
https://hg.mozilla.org/mozilla-central/rev/de0343d8cb76
https://hg.mozilla.org/mozilla-central/rev/5b7440a3cba0
https://hg.mozilla.org/mozilla-central/rev/bf6da7ec431f
Description
•