1801199 - Make sure sitepermission addon install flows are recording an addonManager.install telemetry event when the addon has completed the install flow

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, task, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

The new SitePermsAddonProvider "sitepermission" addon type implementation is already recording for the pre-existing addonsManager.install telemetry events, but while the events with extra var step set to "site_warning" and "permission_prompt" are being recorded as expected, the event for the step "completed" is not being recorded.

This issue is meant to track:

• investigating why the "completed" step is not being recorded
• confirming if other events for the other remaining steps should be recorded by they are not (e.g. "download_started" and "downloaded_completed" are two that are expected to not be recorded for this addon type)
• prepare a fix to maker sure the telemetry event for the "completed" step is recorded as expected

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

So the "completed" step is recorded from https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/AddonManager.jsm#4722-4729

onInstallEnded(install) {
  this.recordInstallEvent(install, { step: "completed" });

this method is registered as an install listener from AddonManager.jsm#4652,4665,4676

But from SitePermsAddonProvider, we never call the install listeners. Instead we exposes an addListener method (SitePermsAddonProvider.sys.mjs); those listeners are then triggered from

https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs#387-408

/**
 * Call the listeners callbacks for a given event.
 *
 * @param {String} eventName: The event to fire. Should be one of `this.#installEvents`
 */
#callInstallListeners(eventName) {
  if (!Object.values(this.#installEvents).includes(eventName)) {
    console.warn(`Unknown "${eventName}" "event`);
    return;
  }

  for (const listener of this.#listeners) {
    try {
      listener[eventName]?.(this);
    } catch (e) {
      console.warn(
        `SitePermsAddonInstall threw exception when calling listener callback for event "${eventName}":`,
        e
      );
    }
  }
}

And we register a listener from installSitePermsAddonFromWebpage https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/AddonManager.jsm#2262-2278

const installListener = {
  onInstallFailed() {
    synthAddonInstall.removeListener(installListener);
    reject(new Error("Install Failed"));
  },

  onInstallCancelled() {
    synthAddonInstall.removeListener(installListener);
    reject(new Error("Install Cancelled"));
  },

  onInstallEnded() {
    synthAddonInstall.removeListener(installListener);
    resolve();
  },
};
synthAddonInstall.addListener(installListener);

In the end, we never end up calling AddonManager.callInstallListeners, and so we're missing some telemetry events.
A fix could be to call lazy.AddonManagerPrivate.callInstallListeners(eventName, [], this) from https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs#387-408, but I think we'd need an extra step to filter out events we don't want (like "onDownloadEnded")

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)

A fix could be to call lazy.AddonManagerPrivate.callInstallListeners(eventName, [], this) from https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs#387-408, but I think we'd need an extra step to filter out events we don't want (like "onDownloadEnded")

While I was implementing this, I wonder if this couldn't lead to unwanted behavior, as those registered event listeners might not be suited for SitePermsAddons.
A more explicit solution would be to directly call AMTelemetry.onInstall* from the callbacks we have in installSitePermsAddonFromWebpage https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/AddonManager.jsm#2262-2278

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #1)

A fix could be to call lazy.AddonManagerPrivate.callInstallListeners(eventName, [], this) from https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/internal/SitePermsAddonProvider.sys.mjs#387-408, but I think we'd need an extra step to filter out events we don't want (like "onDownloadEnded")

While I was implementing this, I wonder if this couldn't lead to unwanted behavior, as those registered event listeners might not be suited for SitePermsAddons.
A more explicit solution would be to directly call AMTelemetry.onInstall* from the callbacks we have in installSitePermsAddonFromWebpage https://searchfox.org/mozilla-central/rev/aa329cf7506ddd966542e642ec00223fd7461599/toolkit/mozapps/extensions/AddonManager.jsm#2262-2278

There aren't that many install listeners on the implementation side, at the moment only 2 to be precise (from https://searchfox.org/mozilla-central/search?q=addInstallListener&path=):

• AMTelemetry from AddonManager.jsm
• the AddonCard (through the AddonManagerListenerHandler) from aboutaddons.js

The rest of the use of install listeners is on the test side.

And so it doesn't seem it may be trigger some other unexpected behaviors, and the approach you suggest in comment 1 may be actually a good one to keep consistency with the existing behaviors and abstractions (and btw thanks for having digged into it, the analysis you did and described in comment 1 is matching what I was expecting).

While looking into your proposal I noticed two other related details:

• it doesn't seem that calling this.#callInstallListeners(this.#installEvents.DOWNLOAD_ENDED) is serving any actual purpose:

  • commenting didn't trigger any failure in the tests we added to test specifically the install flow for the SitePermsAddonProvider nor making a visible difference when trying the install flow manually locally
  • do you recall if we had any specific reason for calling the install listeners for the onDownloadedEnded listener or if it was sent just because the initial status we choose for the SitePermsAddonInstall instance is STATE_DOWNLOADED?
  • if there isn't maybe we should just remove it (it seems the only side-effect would be that we would record a telemetry event that we wouldn't need nor want to be collected)

• AMTelemetry install events are setting install.installId as the value of the telemetry events, the value is meant to help correlating multiple events that are part of the same install flow, while we are here it would be nice to also take care of that, e.g. (similar to what we do in XPInstall.jsm):

// Numeric id included in the install telemetry events to correlate multiple events related
// to the same install or update flow.
let nextInstallId = 0;

...

class SitePermsAddonInstall {
  ...
  constructor(installingPrincipal, sitePerm) {
    ...
    this.installId = ++nextInstallId;
  }

And then once we wire up the AMTelemetry through the calls to callInstallListener the value of the telemetry events should be the auto-incremented numeric installId.

If we remove that this.#callInstallListeners(this.#installEvents.DOWNLOAD_ENDED); statement and we are not concerned about potential unexpected behavior (based on what I described at the start of this comment), I think that the #callInstallListeners method would look more or less like the following:

#callInstallListeners(eventName) {
    if (!Object.values(this.#installEvents).includes(eventName)) {
      console.warn(`Unknown "${eventName}" "event`);
      return;
    }

    lazy.AddonManagerPrivate.callInstallListeners(eventName, Array.from(this.#listeners), this);
}

Let me know what do you think about it (not necessarily in bugzilla comment, we can as well continue in a phabricator revision if we think that we have enough element to start drafting a patch).

Thanks again for quickly digging into this and pin point what was the underlying issue.

Comment 4

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Luca Greco [:rpl] [:luca] [:lgreco] from comment #3)

There aren't that many install listeners on the implementation side, at the moment only 2 to be precise (from https://searchfox.org/mozilla-central/search?q=addInstallListener&path=):

• AMTelemetry from AddonManager.jsm
• the AddonCard (through the AddonManagerListenerHandler) from aboutaddons.js

The rest of the use of install listeners is on the test side.

And so it doesn't seem it may be trigger some other unexpected behaviors, and the approach you suggest in comment 1 may be actually a good one to keep consistency with the existing behaviors and abstractions (and btw thanks for having digged into it, the analysis you did and described in comment 1 is matching what I was expecting).

Alright, I should have waited a bit before implementing the other solution 😁

While looking into your proposal I noticed two other related details:

• it doesn't seem that calling this.#callInstallListeners(this.#installEvents.DOWNLOAD_ENDED) is serving any actual purpose:
  • commenting didn't trigger any failure in the tests we added to test specifically the install flow for the SitePermsAddonProvider nor making a visible difference when trying the install flow manually locally
  • do you recall if we had any specific reason for calling the install listeners for the onDownloadedEnded listener or if it was sent just because the initial status we choose for the SitePermsAddonInstall instance is STATE_DOWNLOADED?
  • if there isn't maybe we should just remove it (it seems the only side-effect would be that we would record a telemetry event that we wouldn't need nor want to be collected)

I can't remember a specific reason. I'll check on the initial review for the initial patch, and if I don't find anything I'll simply drop it

• AMTelemetry install events are setting install.installId as the value of the telemetry events, the value is meant to help correlating multiple events that are part of the same install flow, while we are here it would be nice to also take care of that, e.g. (similar to what we do in XPInstall.jsm):

// Numeric id included in the install telemetry events to correlate multiple events related
// to the same install or update flow.
let nextInstallId = 0;

...

class SitePermsAddonInstall {
  ...
  constructor(installingPrincipal, sitePerm) {
    ...
    this.installId = ++nextInstallId;
  }

And then once we wire up the AMTelemetry through the calls to callInstallListener the value of the telemetry events should be the auto-incremented numeric installId.

If we remove that this.#callInstallListeners(this.#installEvents.DOWNLOAD_ENDED); statement and we are not concerned about potential unexpected behavior (based on what I described at the start of this comment), I think that the #callInstallListeners method would look more or less like the following:

#callInstallListeners(eventName) {
    if (!Object.values(this.#installEvents).includes(eventName)) {
      console.warn(`Unknown "${eventName}" "event`);
      return;
    }

    lazy.AddonManagerPrivate.callInstallListeners(eventName, Array.from(this.#listeners), this);
}

I had something similar in my first patch for this. I have a question though, should we pass this.#listeners ?
If I read AddonManager.jsm#1449-1450,1453 correctly, it would trigger all the listeners each time ?

I'll have a new version of the patch up soon and we can discuss there

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1801199 - Record install telemetry event for SitePermsAddons. r=rpl.

This is done by calling AddonManagerPrivate.callInstallListeners when we handle
SitePermsAddonInstall's specific listeners.
We take this as an opportunity to remove support for the onDownloadEnded event
as it didn't do anything and isn't something we want to record for SitePermsAddon.
We also set SitePermsAddonInstall#installId which is used in AMTelemetry.

Assertions on registered telemetry events are added in browser_midi_permission_gated.js,
and a test case is added to ensure cancelling the event from the second popup works
as expected, since it wasn't checked before.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a76e51a0ea3c
Record install telemetry event for SitePermsAddons. r=rpl.

Comment 7

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/a76e51a0ea3c

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 9304120 [details]
Bug 1801199 - Record install telemetry event for SitePermsAddons. r=rpl.

Beta/Release Uplift Approval Request

• User impact if declined: no user impact, but we'll be missing some telemetry information on a new feature which wouldn't be great
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:

1. Go to https://webmidi-examples.glitch.me/
2. A prompt should be displayed, Install the addon
3. Go to about:telemetry#events-tab_search=sitepermission
4. Check that there's an "install" event

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): telemetry change, covered by automated tests
• String changes made/needed:
• Is Android affected?: No

Comment 9

[Alex Cornestean]

Verified as Fixed on the latest Nightly (109.0a1/20221122214324). Tested on Windows 10 x64 and Ubuntu 16.04 LTS.

The telemetry event for the completed add-on install flow is now properly registered in about:telemetry.

On add-on install the following are registered: site_warning , permissions_prompt , completed.
On uninstall the following are registered: uninstall.

For further details, see the attached screenshot.

Comment 10

[Alex Cornestean]

Attachment: 2022-11-23_09h09_06.png

Comment 11

[Dianna Smith [:diannaS]]

Comment on attachment 9304120 [details]
Bug 1801199 - Record install telemetry event for SitePermsAddons. r=rpl.

Approved for 108.0b6

Comment 12

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2e47b57ab7b8

Comment 13

[Alex Cornestean]

Verified as Fixed on the latest Beta (108.0b6/20221124185931). Tested on Windows 10 x64 and Ubuntu 16.04 LTS.

The telemetry event for the completed add-on install flow is now properly registered in about:telemetry.

On add-on install the following are registered: site_warning , permissions_prompt , completed.
On uninstall the following are registered: uninstall.

For further details, see the attached screenshot.

Comment 14

[Alex Cornestean]

Attachment: 2022-11-25_09h15_42.png