Fix incorrect 'this' in the Sync add-on install error handler
Categories
(Firefox :: Sync, task)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox156 | --- | fixed |
People
(Reporter: mkaply, Assigned: gopal, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: [lang=js])
Attachments
(1 file)
Filing as a good first bug to learn workflows.
In services/sync/modules/addonutils.sys.mjs, the onInstallEnded handler inside
installAddonFromSearchResult() logs a failure with this._log:
try {
addon.enable();
} catch (e) {
this._log.error("Failed to enable the incoming theme", e);
} finally {
onInstallEnded is a method on the listener object literal, so this is the
listener, not AddonUtilsInternal. The listener has no _log property, so
this._log is undefined and the error handler itself throws a TypeError,
hiding the original theme-enabling failure.
installAddonFromSearchResult() already captures the logger in a local:
let log = this._log;
and the sibling onInstallStarted handler uses that log variable. The fix is
to do the same here, changing this._log.error(...) to log.error(...).
Link to the code (see onInstallEnded inside installAddonFromSearchResult):
https://searchfox.org/firefox-main/source/services/sync/modules/addonutils.sys.mjs
To verify the fix:
./mach lint services/sync/modules/addonutils.sys.mjs
./mach test services/sync/tests/unit/test_addon_utils.js
The existing tests do not cover this path, so the main check is that the
corrected line matches the log variable already used by the neighbouring
handler in the same object.
Tutorial to contribute:
https://firefox-source-docs.mozilla.org/contributing/contribution_quickref.html
https://firefox-source-docs.mozilla.org/contributing/stack_quickref.html
Please don't ask for the bug to be assigned. It will be automatically assigned to the first patch.
| Assignee | ||
Comment 1•1 month ago
|
||
Updated•1 month ago
|
| Assignee | ||
Comment 2•1 month ago
|
||
I found a few cleanups that I could pick up in the same file, I guess that should go as a separate commit and a bugzilla ticket and not pile on this one, right?
| Reporter | ||
Comment 3•1 month ago
|
||
If they really are cleanup, you could put them in one psych and maybe the commit message "General code cleanup" or something like that. What are the other changes?
| Assignee | ||
Comment 4•1 month ago
|
||
The install param passed to the onInstall* callbacks aren't needed since it's already in scope (line 71).
const install = await this.getInstallFromSearchResult(addon);
Comment 5•1 month ago
|
||
(In reply to Gopalarathnam Venkatesan from comment #4)
The
installparam passed to theonInstall*callbacks aren't needed since it's already in scope (line 71).const install = await this.getInstallFromSearchResult(addon);
That might be correct but is a little riskier - callers still pass it. We could remove it because js lets you do dumb things, but I'm not entirely sure that's an improvement in the 10s I thought about it :)
Comment 7•1 month ago
|
||
| bugherder | ||
Updated•1 month ago
|
Description
•