Closed Bug 2062989 Opened 1 month ago Closed 1 month ago

Fix incorrect 'this' in the Sync add-on install error handler

Categories

(Firefox :: Sync, task)

task

Tracking

()

RESOLVED FIXED
156 Branch
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: nobody → gopal
Status: NEW → ASSIGNED

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?

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?

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);

(In reply to Gopalarathnam Venkatesan from comment #4)

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);

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 :)

Pushed by ezuehlcke@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/c2700114fcc1 https://hg.mozilla.org/integration/autoland/rev/ddd6c36e5d6a Fix incorrect 'this' in the Sync add-on install error handler. r=mkaply,sync-reviewers,markh
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 156 Branch
QA Whiteboard: [qa-triage-done-c157/b156]
You need to log in before you can comment on or make changes to this bug.