1122050 - Telemetry Environment: addon data collection & change detection

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Georg Fritzsche [:gfritzsche]]

For the telemetry environment we need to collect addon data and detect changes to the enabled addons.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: bug1122050.patch

Adds addon data collection to environment.

Comment 2

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification.

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection. (v2)

- Uniformed function binding in |getEnvironmentData|.

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection. (v2)

Only updates the patch to stack on the ones from bug 1122052.

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification.

Removes the tests from this patch.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Remove duplicated flash, persona and experiment data from TelemetrySession.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins.

Comment 9

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection. (v3)

Rebased the patch and removed references to |_checkForShutdown|.

Comment 10

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v2).

Rebased the patch and removed references to |_checkForShutdown()|

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Remove duplicated flash, persona and experiment data from TelemetrySession (v2)

Rebased the patch.

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v2).

Rebased the patch

Comment 13

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562655 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection. (v3)

Review of attachment 8562655 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the below addressed.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +662,5 @@
> +   * Get the addon data in object form.
> +   * @return Object containing the addon data.
> +   */
> +  _getActiveAddons: Task.async(function* () {
> +    this._log.trace("_getActiveAddons");

Less tracing.

@@ +669,5 @@
> +
> +    let activeAddons = {};
> +
> +    // Request addons, asynchronously.
> +    AddonManager.getAddonsByTypes(["extension", "service"], (allAddons) => {

Lets add a top-level wrapper promiseGetAddonsByTypes() that we use here and in the other functions.

@@ +703,5 @@
> +   * Get the currently active theme data in object form.
> +   * @return Object containing the active theme data.
> +   */
> +  _getActiveTheme: Task.async(function* () {
> +    this._log.trace("_getActiveTheme");

Less tracing.

@@ +710,5 @@
> +
> +    let activeTheme = {};
> +
> +    // Request themes, asynchronously.
> +    AddonManager.getAddonsByTypes(["theme"], (themes) => {

promiseGetAddonsbyTypes()

@@ +713,5 @@
> +    // Request themes, asynchronously.
> +    AddonManager.getAddonsByTypes(["theme"], (themes) => {
> +      for (let theme of themes) {
> +        // We only store information about the active theme.
> +        if (theme.isActive) {

Probably easier to read using |let active = themes.find(theme => theme.isActive)|.

@@ +745,5 @@
> +   * Get the plugins data in object form.
> +   * @return Object containing the plugins data.
> +   */
> +  _getActivePlugins: function () {
> +    this._log.trace("_getActivePlugins");

Less tracing.

@@ +780,5 @@
> +   * Get the GMPlugins data in object form.
> +   * @return Object containing the GMPlugins data.
> +   */
> +  _getActiveGMPlugins: Task.async(function* () {
> +    this._log.trace("_getActiveGMPlugins");

Less tracing.

@@ +787,5 @@
> +
> +    let activeGMPlugins = {};
> +
> +    // Request plugins, asynchronously.
> +    AddonManager.getAddonsByTypes(["plugin"], (allPlugins) => {

promiseGetAddonsByTypes()

@@ +812,5 @@
> +   * Get the active experiment data in object form.
> +   * @return Object containing the active experiment data.
> +   */
> +  _getActiveExperiment: function () {
> +    this._log.trace("_getActiveExperiment");

Less tracing.

@@ +837,5 @@
> +   * Get the addon data in object form.
> +   * @return Object containing the addon data.
> +   */
> +  _getAddons: Task.async(function* () {
> +    this._log.trace("_getAddons");

Less tracing.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +28,5 @@
> +  // Setup the addonManager
> +  let addonManager = Cc["@mozilla.org/addons/integration;1"]
> +                       .getService(Ci.nsIObserver)
> +                       .QueryInterface(Ci.nsITimerCallback);
> +  addonManager.observe(null, "addons-startup", null);

Why do need to partially duplicate that fragile logic here instead of doing something like this:
https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/browser/experiments/test/xpcshell/head.js#l145

That is admittedly crude, but i think that's the closest to shared addon manager setup routines we have now.

The helper function can go into head.js as we'll have to do this in other tests too.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562659 [details] [diff] [review]
part 2 - Addon change notification (v2).

Review of attachment 8562659 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +876,5 @@
>    /**
> +   * Start watching the addons for changes.
> +   */
> +  _startWatchingAddons: function () {
> +    this._log.trace("_startWatchingAddons");

Less tracing.

@@ +877,5 @@
> +   * Start watching the addons for changes.
> +   */
> +  _startWatchingAddons: function () {
> +    this._log.trace("_startWatchingAddons");
> +    if (this._shutdown) {

This should only be called internally, so we don't need to double-check here.

@@ +886,5 @@
> +    const ADDON_LISTENER_CALLBACKS = [
> +      "onEnabled",
> +      "onDisabled",
> +      "onInstalled",
> +      "onUninstalled",

onInstalled & onUninstalled should be followed/preceded by onEnabled/onDisabled.
Please verify and remove onInstalled/onUninstalled to avoid unnecessary change events.

Also, can't we just create a listener directly instead of dynamically here:
let callback = addon => this._onAddonsChanged(addon);
this._addonsListener = {
  onEnabled: callback,
  ...

@@ +892,5 @@
> +
> +    // Define a listener to catch addons changes from the AddonManager.
> +    this._addonsListener = {};
> +    for (let method of ADDON_LISTENER_CALLBACKS) {
> +      this._addonsListener[method] = (addon) => this._onAddonsChanged();

We need to filter this for the addon types we are interested in ("extension", "service", ...).
Maybe just filter in _onAddonsChanged(), then we can also trace the addon id there for QA/bug-hunting/...

@@ +902,5 @@
> +  /**
> +   * Stop watching addons for changes.
> +   */
> +  _stopWatchingAddons: function () {
> +    this._log.trace("_stopWatchingAddons");

Less tracing.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562661 [details] [diff] [review]
part 3 - Remove duplicated flash, persona and experiment data from TelemetrySession (v2)

Review of attachment 8562661 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the below fixed.
Please update and f?vladan for heads-up.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ -568,5 @@
>        ret.addons = this._addons;
>  
> -    let flashVersion = this.getFlashVersion();
> -    if (flashVersion)
> -      ret.flashVersion = flashVersion;

We can't remove this yet. We only submit active plugins in the environment so far, while this always submits the flash version - even if it is disabled.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ -171,5 @@
>      appVersion: "1", 
>      appName: "XPCShell", 
>      appBuildID: "2007010101",
>      platformBuildID: "2007010101",
> -    flashVersion: FLASH_VERSION

Same for the tests - we can't remove flashVersion here yet.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562659 [details] [diff] [review]
part 2 - Addon change notification (v2).

Review of attachment 8562659 [details] [diff] [review]:
-----------------------------------------------------------------

Also:
* we need to detect experiment changes (observe "experiments-changed")
* can we detect persona changes?

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562662 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v2).

Review of attachment 8562662 [details] [diff] [review]:
-----------------------------------------------------------------

Can go to a follow-up bug:
* we need |activeTheme| coverage
* we need |activeGMP| coverage, test_openh264.js (soon test_gmpProvider.js?) shows how to fake them

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +10,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);
> +Cu.import("resource://testing-common/httpd.js");
> +
> +// We are allowed to load |LightweightThemeManager| without caring if we are using gonk
> +// since xpcshell.ini skips the tests in that case.

This here:
https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/toolkit/components/telemetry/tests/unit/xpcshell.ini#l4
... points to bug 1066735. From a quick look they were disabled because of manifest parsing errors, not because they are not intended to run on b2g.

Lets not count on them staying disabled there.

@@ +441,5 @@
> +  TelemetryEnvironment.registerChangeListener("testWatchAddons_Install", deferred.resolve);
> +
> +  AddonManager.getInstallForURL(ADDON_INSTALL_URL, (aInstall) => aInstall.install(),
> +                                "application/x-xpinstall");
> +  yield deferred.promise;

We also need test-coverage for not triggering change events for addon types we don't care about.

@@ +479,5 @@
> +  }
> +
> +  // Check the active addon.
> +  let activeAddons = data.addons.activeAddons;
> +  Assert.ok(!!activeAddons[ADDON_ID], "We must have one active addon.");

Assert.ok(ADDON_ID in activeAddons)?

@@ +487,5 @@
> +
> +  Assert.equal(activeAddons[ADDON_ID].name, ADDON_NAME,
> +               "The addon must have the correct name.");
> +  Assert.equal(activeAddons[ADDON_ID].type, ADDON_TYPE,
> +               "The addon must have the correct type.");

We know all the fields values. Lets do a complete check here.

@@ +493,5 @@
> +  // Check the plugins.
> +  let activePlugins = data.addons.activePlugins;
> +  Assert.equal(activePlugins.length, 1, "We must have only one active plugin.");
> +  for (let f of EXPECTED_PLUGIN_FIELDS) {
> +    Assert.ok(f in activePlugins[0]);

We should know all the values, so lets explicitly check them all below.

::: toolkit/components/telemetry/tests/unit/xpcshell.ini
@@ +5,5 @@
>  
>  [test_nsITelemetry.js]
>  [test_TelemetryEnvironment.js]
> +support-files =
> +  ../../../../mozapps/extensions/test/xpinstall/restartless.xpi

Lets not depend on that one. Its relatively easy to add our own. Hackish makefile:
https://dxr.mozilla.org/mozilla-central/source/browser/experiments/Makefile.in
... then we only need RDFs in separate folders, e.g. (with the type adjusted):
https://dxr.mozilla.org/mozilla-central/source/browser/experiments/test/addons/experiment-1

Comment 18

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8562659 [details] [diff] [review]
part 2 - Addon change notification (v2).

Review of attachment 8562659 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +239,5 @@
>      }
>  
>      this._log.trace("shutdown");
>      this._stopWatchingPrefs();
> +    this._stopWatchingAddons();

This should be after _shutdown=true

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection. (v4 - WIP)

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v3 - WIP).

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Remove persona and experiment data from TelemetrySession (v3)

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v3 - WIP).

Comment 23

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Remove persona and experiment data from TelemetrySession (v4)

FLASH_VERSION slipped through last patch, restored it here. Also updated the commit message.

Comment 24

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection. (v4)

Removed most of the tracing, introduced |promiseGetAddonsByTypes|, changed and moved addon manager setup routines to "head.js".

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v3 - WIP).

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v3 - WIP).

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v3).

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v3 - WIP).

Comment 29

[Alessio Placitelli [:Dexter]]

Comment on attachment 8564117 [details] [diff] [review]
part 2 - Addon change notification (v3).

>onInstalled & onUninstalled should be followed/preceded by onEnabled/onDisabled.
>Please verify and remove onInstalled/onUninstalled to avoid unnecessary change >events.

Apparently, when installing restartless addons, I'm just receiving "onInstalled" and no "onEnabled" (for example when installing [1] and an example restartless XPI).

> * can we detect persona changes?

"Persona" seems to be an alias for "theme" and, given [2] and [3], we should be able to catch that through theme change notifications. I'll be covering that in tests.

[1] - https://addons.mozilla.org/en-US/firefox/addon/html5-video-everywhere/?src=cb-dl-hotness
[2] - https://hg.mozilla.org/mozilla-central/annotate/a7c177546ca0/toolkit/components/telemetry/TelemetrySession.jsm#l619
[3] - https://hg.mozilla.org/mozilla-central/annotate/a7c177546ca0/toolkit/mozapps/extensions/LightweightThemeManager.jsm#l84

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v3).

* adds |activeTheme| coverage;
* adds type check;
* adds value check for all the controlled test values;
* adds field names in Assert.* messages when in for loops to help with the identification of potential problems from logs;
* adds XPI file generation and bundles .rdf manifests;

Comment 31

[Alessio Placitelli [:Dexter]]

Comment on attachment 8563516 [details] [diff] [review]
part 3 - Remove persona and experiment data from TelemetrySession (v4)

This patch removes persona and experiment data from TelemetrySession.jsm, as they were moved to the Telemetry Environment data (see Part 1 of this bug).

As a sidenote, removing |LightweightThemeManager.currentTheme = dummyTheme("1234");| from test_TelemetryPing.js makes the tests fail due to bug 1131585. It seems that this line somehow triggers histogram collection, but I did not dig into this issue just yet.

Comment 32

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #29)
> Comment on attachment 8564117 [details] [diff] [review]
> part 2 - Addon change notification (v3).
> 
> >onInstalled & onUninstalled should be followed/preceded by onEnabled/onDisabled.
> >Please verify and remove onInstalled/onUninstalled to avoid unnecessary change >events.
> 
> Apparently, when installing restartless addons, I'm just receiving
> "onInstalled" and no "onEnabled" (for example when installing [1] and an
> example restartless XPI).

Ok, so i think we could either:
* check the active state in onInstalled/onUninstalled and ignore if its not active/inactive yet
* detect an addon is not restartless and ignore onInstalled/onUninstalled in that case (addon.operationsRequiringRestart == OP_NEEDS_RESTART_*, which?)

Unfocused, does the above sound ok? Which is the proper route?
We only want to detect when the set of active addons changes (of a certain set of addon types).
For that we are using addon listeners (onInstalled, onUninstalled, onEnabled, onDisabled) and filter out the relevant addon types. We should not double detect classic, non-restartless, addon activations and disabling (onInstalled & onEnabled).

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(Utterly swamped, redirecting most bugs ATM)

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(And the US has a holiday today, soo....)

So, some of this depends on what you mean by "restartless". A traditional old "non-restartless" add-on can be uninstalled without a restart, if it was already disabled. A bootstrapped "restartless" add-on can require a restart to install if it's an upgrade where the previous version was non-restartless. The Add-ons Manager generally tries it's best not to care about restartless/non-restartless *add-ons*, just restartless/non-restartless *actions*.

For the general case of bootstrapped add-ons, you can check:
  addon.operationsRequiringRestart==AddonManager.OP_NEEDS_RESTART_NONE


onenabled doesn't get called for installs. onDisabled *can* get called for uninstalls, because we currently fake this to allow undo uninstall for restartless uninstalls (bug 612168), so check operationsRequiringRestart.

For installs: onInstalled. Also, addon.pendingOperations will equal PENDING_NONE
For uninstalls: onUninstalled. This only gets called for restartless uninstalls (see caveat above - restartless action, not restartless add-on)

Comment 35

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v4)

This patch changes addons changes notifications so that we don't get double notifications when an active addon is installed/uninstalled.

@mossop: Would you kindly check if |TelemetryEnvironment._startWatchingAddons| correctly deals with the edge cases of addons event notifications? We only want to detect when the set of active addons changes, ideally without getting double notifications. Thanks!

Comment 36

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8565447 [details] [diff] [review]
part 2 - Addon change notification (v4)

Review of attachment 8565447 [details] [diff] [review]:
-----------------------------------------------------------------

We need to test and address the plugin situation below, the rest seems good.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +835,5 @@
> +    // tricky, as we might have the same change notified twice if we don't filter them.
> +    // Also, we only want to detect when the set of active addons changes.
> +    //
> +    // We identified the following cases:
> +    // - onEnabled: Doesn't get called for onInstalls.

s/onInstalls/addon installs/

@@ +836,5 @@
> +    // Also, we only want to detect when the set of active addons changes.
> +    //
> +    // We identified the following cases:
> +    // - onEnabled: Doesn't get called for onInstalls.
> +    // - onDisabled: *Can* get called for onUninstalls (restartless uninstalls), but we can

s/onUninstalls/addon uninstalls/

@@ +838,5 @@
> +    // We identified the following cases:
> +    // - onEnabled: Doesn't get called for onInstalls.
> +    // - onDisabled: *Can* get called for onUninstalls (restartless uninstalls), but we can
> +    //               check that addon.operationsRequiringRestart requires no restart
> +    //               (AddonManager.OP_NEEDS_RESTART_NONE) and ignore it.

Update the comment or be less specific about RESTART_NONE

@@ +842,5 @@
> +    //               (AddonManager.OP_NEEDS_RESTART_NONE) and ignore it.
> +    // - onInstalled: Gets called for *all* addon installs and will have
> +    //                addon.pendingOperations == AddonManager.PENDING_NONE.
> +    // - onUninstalled: Gets called for restartless addons uninstall.
> +    this._addonsListener = {

Have you tested this with plugins?
It's correct to watch them here, but they should always trigger both:
https://hg.mozilla.org/mozilla-central/annotate/09f4968d5f42/toolkit/mozapps/extensions/internal/PluginProvider.jsm#l264

We might just need to filter them out in onInstalled/onUninstalled.

Let's also have test-coverage for them and that they don't trigger more then one notification.

@@ +850,5 @@
> +      },
> +      onDisabled: addon => {
> +        this._log.trace("onDisabled - Operation requires restart: " +
> +                        addon.operationsRequiringRestart);
> +        if (!(addon.operationsRequiringRestart & OP_NEEDS_RESTART_UNINSTALL)) {

Comment this oddity and point to bug 612168.

Comment 37

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8564711 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v3).

Review of attachment 8564711 [details] [diff] [review]:
-----------------------------------------------------------------

I'm missing test coverage for proper data for GMPs and experiments.
Do we want to do that here or in a follow-up?

::: toolkit/components/telemetry/docs/environment.rst
@@ -122,5 @@
>        addons: {
>          activeAddons: { // the currently enabled addons
>            <addon id>: {
>              blocklisted: <bool>,
> -            description: <string>,

Nit: This belongs in the patch that implements that behavior, not the test patch.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +724,5 @@
>    TelemetryEnvironment.unregisterChangeListener("testWatchPrefs_reset");
>    yield TelemetryEnvironment.shutdown();
>  });
>  
> +add_task(function* test_addonsWatch_InterestingChange() {

We need to add a similar test for plugins that checks for the correct notification counts for adding and removing plugins.
PluginProvider triggers things on "plugins-list-updated":
https://hg.mozilla.org/mozilla-central/annotate/09f4968d5f42/toolkit/mozapps/extensions/internal/PluginProvider.jsm#l90

Side-note: Check platform availability (Android) of PluginProvider.

@@ +731,5 @@
> +
> +  let deferred = PromiseUtils.defer();
> +  TelemetryEnvironment.registerChangeListener("testWatchAddons_Install", deferred.resolve);
> +
> +  promiseInstallAddon(ADDON_INSTALL_URL);

yield promiseInstall... first? (Cleaner stacks probably for rejections etc.)

@@ +732,5 @@
> +  let deferred = PromiseUtils.defer();
> +  TelemetryEnvironment.registerChangeListener("testWatchAddons_Install", deferred.resolve);
> +
> +  promiseInstallAddon(ADDON_INSTALL_URL);
> +  yield deferred.promise;

Lets assert correct notification counts here too.
Lets also add checks for uninstall, disabling, enabling.

Comment 38

[Dave Townsend [:mossop]]

Comment on attachment 8565447 [details] [diff] [review]
part 2 - Addon change notification (v4)

Review of attachment 8565447 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure exactly what you're asking. If you want to know if this code could result in a call to _onEnvironmentChange when there hasn't actually been a change to the active set of add-ons then yes that is possible, I've commented on a couple of them. Also this won't catch changes that happen during startup, I assume that is handled somewhere else?

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +850,5 @@
> +      },
> +      onDisabled: addon => {
> +        this._log.trace("onDisabled - Operation requires restart: " +
> +                        addon.operationsRequiringRestart);
> +        if (!(addon.operationsRequiringRestart & OP_NEEDS_RESTART_UNINSTALL)) {

I don't think this filter is necessary. All it is doing is checking that the add-on is restartless, and you can't get an onDisabled notification for a non-restartless add-on.

@@ +857,5 @@
> +      },
> +      onInstalled: addon => {
> +        this._log.trace("onInstalled - Pending operations " + addon.pendingOperations);
> +        if (addon.pendingOperations == AddonManager.PENDING_NONE) {
> +          this._onActiveAddonsChanged(addon);

Even if there are no pending operations doesn't mean that an add-on has become active. Check addon.isActive too.

@@ +862,5 @@
> +        }
> +      },
> +      onUninstalled: addon => {
> +        this._log.trace("onUninstalled");
> +        this._onActiveAddonsChanged(addon);

If the add-on was already disabled before being uninstalled (always the case for add-ons uninstalled through the UI) then this onUninstalled notification won't signal a change to the active set of add-ons.

Comment 39

[Georg Fritzsche [:gfritzsche]]

Stacking the patches up to the ones here brings back the fun failure in test_TelemetryPing.js:
 0:05.46 TEST_STATUS: Thread-1 test_saveLoadPing FAIL [test_saveLoadPing : 244] false == true
    /Users/gfritzsche/moz/mc1/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js:checkPayload:244
    /Users/gfritzsche/moz/mc1/obj/_tests/xpcshell/toolkit/components/telemetry/tests/unit/test_TelemetryPing.js:test_saveLoadPing:563

-> do_check_true(("STARTUP_" + name) in payload.histograms);

Comment 40

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #39)
> Stacking the patches up to the ones here brings back the fun failure in
> test_TelemetryPing.js:
>  0:05.46 TEST_STATUS: Thread-1 test_saveLoadPing FAIL [test_saveLoadPing :
> 244] false == true
>    
> /Users/gfritzsche/moz/mc1/obj/_tests/xpcshell/toolkit/components/telemetry/
> tests/unit/test_TelemetryPing.js:checkPayload:244
>    
> /Users/gfritzsche/moz/mc1/obj/_tests/xpcshell/toolkit/components/telemetry/
> tests/unit/test_TelemetryPing.js:test_saveLoadPing:563
> 
> -> do_check_true(("STARTUP_" + name) in payload.histograms);

Yes, as explained in comment 31 :)

Comment 41

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Add addon, plugin and GMPlugin data collection. (v5)

Updates environment documentation.

Comment 42

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v5 - WIP)

Comment 43

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v4 - WIP).

Comment 44

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v5)

This patch changes the comment and fixes addonListener.onDisabled/onInstalled as suggested by :mossop.

Comment 45

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v4)

This patch fixes test coverage for plugin installation/removal. Adds assertion on notification counts for addons watches.

Comment 46

[Alessio Placitelli [:Dexter]]

> I'm not sure exactly what you're asking. If you want to know if this code
> could result in a call to _onEnvironmentChange when there hasn't actually
> been a change to the active set of add-ons then yes that is possible, I've
> commented on a couple of them. Also this won't catch changes that happen
> during startup, I assume that is handled somewhere else?

Yes, sorry if I wasn't clear and thank you for your help. That's what we'd like to do be sure about:

- We need to catch all the changes to the active set of addons.
- We don't want to be notified twice about the same change.

We only want the actual changes, not the scheduled changes (we're not interested to changes happening after a restart, for example).

> @@ +862,5 @@
> > +        }
> > +      },
> > +      onUninstalled: addon => {
> > +        this._log.trace("onUninstalled");
> > +        this._onActiveAddonsChanged(addon);
> 
> If the add-on was already disabled before being uninstalled (always the case
> for add-ons uninstalled through the UI) then this onUninstalled notification
> won't signal a change to the active set of add-ons.

Does this happen for plugins as well?

Comment 47

[Dave Townsend [:mossop]]

(In reply to Alessio Placitelli [:Dexter] from comment #46)
> > I'm not sure exactly what you're asking. If you want to know if this code
> > could result in a call to _onEnvironmentChange when there hasn't actually
> > been a change to the active set of add-ons then yes that is possible, I've
> > commented on a couple of them. Also this won't catch changes that happen
> > during startup, I assume that is handled somewhere else?
> 
> Yes, sorry if I wasn't clear and thank you for your help. That's what we'd
> like to do be sure about:
> 
> - We need to catch all the changes to the active set of addons.
> - We don't want to be notified twice about the same change.

So this last part is the tricky bit. The patch looks mostly there, the only exception is the onUninstalled event. Instead of using onUninstalled I recommend this:

    onUninstalling: function(addon, requiresRestart) {
      if (!addon.isActive || requiresRestart)
        return;
      this._onActiveAddonsChanged(addon);
    }

This is technically a fudge because you're recording the change moments before it happens but I don't think anything can stop it happening at that point, at least beyond system failure. If you're really concerned then cache the add-on's ID and then record when you see the onUninstalled event.

> We only want the actual changes, not the scheduled changes (we're not
> interested to changes happening after a restart, for example).
> 
> > @@ +862,5 @@
> > > +        }
> > > +      },
> > > +      onUninstalled: addon => {
> > > +        this._log.trace("onUninstalled");
> > > +        this._onActiveAddonsChanged(addon);
> > 
> > If the add-on was already disabled before being uninstalled (always the case
> > for add-ons uninstalled through the UI) then this onUninstalled notification
> > won't signal a change to the active set of add-ons.
> 
> Does this happen for plugins as well?

No and to be clear it doesn't always happen for restartless add-ons. If they are uninstalled through the API you don't get an onDisabled event or anything before onUninstalled. I think we have a bug open on this somewhere.

Comment 48

[Dave Townsend [:mossop]]

Comment on attachment 8566538 [details] [diff] [review]
part 2 - Addon change notification (v5)

Review of attachment 8566538 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +855,5 @@
> +      },
> +      onInstalled: addon => {
> +        this._log.trace("onInstalled - Pending operations " + addon.pendingOperations);
> +        if (addon.isActive &&
> +            (addon.pendingOperations == AddonManager.PENDING_NONE)) {

Testing pendingOperations is unnecessary here, if the install was pending a restart you wouldn't get the onInstalled notification.

Comment 49

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v6)

Thank you for your time and help, :mossop! Now things are clearer :-)

Do you think this is good to go, as far as addon changes are concerned?

Comment 50

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8566679 [details] [diff] [review]
part 2 - Addon change notification (v6)

Review of attachment 8566679 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me with the below addressed if Mossop agrees on the addons listener filtering.

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +45,5 @@
>  const PREF_UPDATE_AUTODOWNLOAD = "app.update.auto";
>  
>  const MILLISECONDS_PER_DAY = 24 * 60 * 60 * 1000;
>  
> +const EXPERIMENT_CHANGED_TOPIC = "experiments-changed";

Nit: Missing S: EXPERIMENTS_...

@@ +840,5 @@
> +    // * onEnabled:   Gets called when a restartless addon is enabled. Doesn't get called
> +    //                if the restartless addon is installed and directly enabled.
> +    // * onDisabled:  Gets called when disabling a restartless addon or can get called when
> +    //                uninstalling a restartless addon from the UI (see bug 612168).
> +    // * onInstalled: Gets called for all addon installs.

Add onUninstalling.

@@ +844,5 @@
> +    // * onInstalled: Gets called for all addon installs.
> +
> +    this._addonsListener = {
> +      onEnabled: addon => {
> +        this._log.trace("onEnabled");

this._log.trace("_addonsListener - onEnabled " + addon.id);
... similarly for the other ones below.

@@ +891,5 @@
> +    const INTERESTING_ADDONS = [ "extension", "plugin", "service", "theme" ];
> +
> +    this._log.trace("_onActiveAddonsChanged - id " + aAddon.id + ", type " + aAddon.type);
> +
> +    if (INTERESTING_ADDONS.indexOf(aAddon.type) >= 0) {

Nit: .find()

Comment 51

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8566540 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v4)

Review of attachment 8566540 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with comments fixed - ping me if anything on the below comments is unclear.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +742,5 @@
> +  let deferred = PromiseUtils.defer();
> +  let receivedNotifications = 0;
> +  let callback = () => {
> +    receivedNotifications++;
> +    if (receivedNotifications == EXPECTED_NOTIFICATIONS) {

Let's test for receiving one notification after each change instead.
That:
a) allows us to see exactly what went wrong and
b) doesnt make us pass unexpectedly if only one change triggers 4 callbacks

@@ +750,5 @@
> +  TelemetryEnvironment.registerChangeListener("testWatchAddons_Changes", callback);
> +
> +  yield AddonTestUtils.installXPIFromURL(ADDON_INSTALL_URL);
> +  yield promiseSetAddonEnabled(ADDON_ID, false);
> +  yield promiseSetAddonEnabled(ADDON_ID, true);

We can just add AddonTestUtils.getAddonById() and then:
let addon = yield AddonTestUtils.getAddonById(ADDON_ID);
addon.userDisabled = ...

@@ +792,5 @@
> +  let plugin = gInstalledPlugins.find(plugin => (plugin.name == PLUGIN2_NAME));
> +  Assert.ok(plugin, "The test plugin must exist.");
> +
> +  // Remove it from the PluginHost.
> +  gInstalledPlugins.splice(gInstalledPlugins.indexOf(plugin), 1);

This could be less confusing.
Can't you just .filter() or  or something?

@@ +889,5 @@
> +
> +  // Check plugin mime types.
> +  Assert.ok(targetPlugin.mimeTypes.indexOf(PLUGIN_MIME_TYPE1) >= 0);
> +  Assert.ok(targetPlugin.mimeTypes.indexOf(PLUGIN_MIME_TYPE2) >= 0);
> +  Assert.ok(targetPlugin.mimeTypes.indexOf("Not There.") < 0);

.find()

Comment 52

[Dave Townsend [:mossop]]

Comment on attachment 8566679 [details] [diff] [review]
part 2 - Addon change notification (v6)

Review of attachment 8566679 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good

Comment 53

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Addon change notification (v7)

Taken care of the Nits from the last review.

Comment 54

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v5)

This patch changes addons test to check for the correct notification count at each step and fixes the other nits from Georg.

Comment 55

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8567080 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v5)

Review of attachment 8567080 [details] [diff] [review]:
-----------------------------------------------------------------

f+ with one note.

::: toolkit/mozapps/extensions/test/AddonManagerTesting.jsm
@@ +23,5 @@
>    /**
> +   * Get the add-on that is specified by its ID.
> +   *
> +   * @return {Promise<Object>} A promise that resolves returning the found addon or fails
> +   *         if it is not found.

I don't see this actually failing. It will resolve to the addon or null.

Comment 56

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Add test coverage for addons and plugins (v6)

Thanks for the feedback Georg. Changed the comment.

Comment 57

[Georg Fritzsche [:gfritzsche]]

Pushed in: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=b83c3fa8c80f

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7fecb9b2f002
https://hg.mozilla.org/mozilla-central/rev/d19b4a2fab52
https://hg.mozilla.org/mozilla-central/rev/85e18605a65b
https://hg.mozilla.org/mozilla-central/rev/0a9eaecca94d