Telemetry Environment: addon data collection & change detection

RESOLVED FIXED in Firefox 39

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

Trunk
mozilla39
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

(Whiteboard: [ready])

Attachments

(4 attachments, 29 obsolete attachments)

4.46 KB, patch
Dexter
: review+
vladan
: feedback+
Details | Diff | Splinter Review
12.47 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
6.19 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
29.38 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
For the telemetry environment we need to collect addon data and detect changes to the enabled addons.
(Assignee)

Updated

4 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 1

4 years ago
Created attachment 8557054 [details] [diff] [review]
bug1122050.patch

Adds addon data collection to environment.
Attachment #8557054 - Flags: feedback?(gfritzsche)
(Assignee)

Comment 2

4 years ago
Created attachment 8557498 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection.
Attachment #8557054 - Attachment is obsolete: true
Attachment #8557054 - Flags: feedback?(gfritzsche)
Attachment #8557498 - Flags: review?(gfritzsche)
(Assignee)

Comment 3

4 years ago
Created attachment 8557499 [details] [diff] [review]
part 2 - Addon change notification.
Attachment #8557499 - Flags: review?(gfritzsche)
(Assignee)

Comment 4

4 years ago
Created attachment 8557580 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection. (v2)

- Uniformed function binding in |getEnvironmentData|.
Attachment #8557498 - Attachment is obsolete: true
Attachment #8557498 - Flags: review?(gfritzsche)
Attachment #8557580 - Flags: review?(gfritzsche)
(Assignee)

Comment 5

4 years ago
Created attachment 8559063 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection. (v2)

Only updates the patch to stack on the ones from bug 1122052.
Attachment #8557580 - Attachment is obsolete: true
Attachment #8557580 - Flags: review?(gfritzsche)
(Assignee)

Comment 6

4 years ago
Created attachment 8559067 [details] [diff] [review]
part 2 - Addon change notification.

Removes the tests from this patch.
Attachment #8557499 - Attachment is obsolete: true
Attachment #8557499 - Flags: review?(gfritzsche)
(Assignee)

Comment 7

4 years ago
Created attachment 8559069 [details] [diff] [review]
part 3 - Remove duplicated flash, persona and experiment data from TelemetrySession.
(Assignee)

Comment 8

4 years ago
Created attachment 8559070 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins.
(Assignee)

Comment 9

4 years ago
Created attachment 8562655 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection. (v3)

Rebased the patch and removed references to |_checkForShutdown|.
Attachment #8559063 - Attachment is obsolete: true
Attachment #8562655 - Flags: review?(gfritzsche)
(Assignee)

Comment 10

4 years ago
Created attachment 8562659 [details] [diff] [review]
part 2 - Addon change notification (v2).

Rebased the patch and removed references to |_checkForShutdown()|
Attachment #8559067 - Attachment is obsolete: true
Attachment #8562659 - Flags: review?(gfritzsche)
(Assignee)

Comment 11

4 years ago
Created attachment 8562661 [details] [diff] [review]
part 3 - Remove duplicated flash, persona and experiment data from TelemetrySession (v2)

Rebased the patch.
Attachment #8559069 - Attachment is obsolete: true
Attachment #8562661 - Flags: review?(gfritzsche)
(Assignee)

Comment 12

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

Rebased the patch
Attachment #8559070 - Attachment is obsolete: true
Attachment #8562662 - Flags: review?(gfritzsche)
(Reporter)

Comment 13

4 years ago
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.
Attachment #8562655 - Flags: review?(gfritzsche) → review+
(Reporter)

Comment 14

4 years ago
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.
Attachment #8562659 - Flags: review?(gfritzsche)
(Reporter)

Comment 15

4 years ago
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.
Attachment #8562661 - Flags: review?(gfritzsche) → review+
(Reporter)

Comment 16

4 years ago
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?
(Reporter)

Comment 17

4 years ago
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
Attachment #8562662 - Flags: review?(gfritzsche)
(Reporter)

Updated

4 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 18

4 years ago
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
(Assignee)

Comment 19

4 years ago
Created attachment 8563492 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection. (v4 - WIP)
Attachment #8562655 - Attachment is obsolete: true
(Assignee)

Comment 20

4 years ago
Created attachment 8563493 [details] [diff] [review]
part 2 - Addon change notification (v3 - WIP).
Attachment #8562659 - Attachment is obsolete: true
(Assignee)

Comment 21

4 years ago
Created attachment 8563494 [details] [diff] [review]
part 3 - Remove persona and experiment data from TelemetrySession (v3)
Attachment #8562661 - Attachment is obsolete: true
(Assignee)

Comment 22

4 years ago
Created attachment 8563495 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v3 - WIP).
Attachment #8562662 - Attachment is obsolete: true
(Assignee)

Comment 23

4 years ago
Created attachment 8563516 [details] [diff] [review]
part 3 - Remove persona and experiment data from TelemetrySession (v4)

FLASH_VERSION slipped through last patch, restored it here. Also updated the commit message.
Attachment #8563494 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
Created attachment 8564003 [details] [diff] [review]
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".
Attachment #8563492 - Attachment is obsolete: true
Attachment #8564003 - Flags: review+
(Assignee)

Comment 25

4 years ago
Created attachment 8564074 [details] [diff] [review]
part 2 - Addon change notification (v3 - WIP).
Attachment #8563493 - Attachment is obsolete: true
(Assignee)

Comment 26

4 years ago
Created attachment 8564075 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v3 - WIP).
Attachment #8563495 - Attachment is obsolete: true
(Assignee)

Comment 27

4 years ago
Created attachment 8564117 [details] [diff] [review]
part 2 - Addon change notification (v3).
Attachment #8564074 - Attachment is obsolete: true
(Assignee)

Comment 28

4 years ago
Created attachment 8564118 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v3 - WIP).
Attachment #8564075 - Attachment is obsolete: true
(Assignee)

Comment 29

4 years ago
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
Attachment #8564117 - Attachment description: part 2 - Addon change notification (v3 - WIP). → part 2 - Addon change notification (v3).
Attachment #8564117 - Flags: review?(gfritzsche)
(Assignee)

Comment 30

4 years ago
Created attachment 8564711 [details] [diff] [review]
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;
Attachment #8564118 - Attachment is obsolete: true
Attachment #8564711 - Flags: review?(gfritzsche)
(Assignee)

Comment 31

4 years ago
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.
Attachment #8563516 - Flags: review+
Attachment #8563516 - Flags: feedback?(vdjeric)
(Reporter)

Comment 32

4 years ago
(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).
Flags: needinfo?(bmcbride)
(Utterly swamped, redirecting most bugs ATM)
Flags: needinfo?(bmcbride) → needinfo?(dtownsend)
(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)
Flags: needinfo?(dtownsend)
(Assignee)

Comment 35

4 years ago
Created attachment 8565447 [details] [diff] [review]
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!
Attachment #8564117 - Attachment is obsolete: true
Attachment #8564117 - Flags: review?(gfritzsche)
Attachment #8565447 - Flags: review?(gfritzsche)
Attachment #8565447 - Flags: feedback?(dtownsend)
(Reporter)

Comment 36

4 years ago
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.
Attachment #8565447 - Flags: review?(gfritzsche)
(Reporter)

Comment 37

4 years ago
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.
Attachment #8564711 - Flags: review?(gfritzsche) → feedback+
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.
Attachment #8565447 - Flags: feedback?(dtownsend) → feedback-
Attachment #8563516 - Flags: feedback?(vdjeric) → feedback+
(Reporter)

Comment 39

4 years ago
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);
(Assignee)

Comment 40

4 years ago
(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 :)
(Assignee)

Comment 41

4 years ago
Created attachment 8565934 [details] [diff] [review]
part 1 - Add addon, plugin and GMPlugin data collection. (v5)

Updates environment documentation.
Attachment #8564003 - Attachment is obsolete: true
Attachment #8565934 - Flags: review+
(Assignee)

Comment 42

4 years ago
Created attachment 8565972 [details] [diff] [review]
part 2 - Addon change notification (v5 - WIP)
Attachment #8565447 - Attachment is obsolete: true
(Assignee)

Comment 43

4 years ago
Created attachment 8565975 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v4 - WIP).
Attachment #8564711 - Attachment is obsolete: true
(Assignee)

Comment 44

4 years ago
Created attachment 8566538 [details] [diff] [review]
part 2 - Addon change notification (v5)

This patch changes the comment and fixes addonListener.onDisabled/onInstalled as suggested by :mossop.
Attachment #8565972 - Attachment is obsolete: true
(Assignee)

Comment 45

4 years ago
Created attachment 8566540 [details] [diff] [review]
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.
Attachment #8565975 - Attachment is obsolete: true
Attachment #8566540 - Flags: review?(gfritzsche)
(Assignee)

Comment 46

4 years ago
> 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?
Flags: needinfo?(dtownsend)
(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.
Flags: needinfo?(dtownsend)
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.
(Reporter)

Updated

4 years ago
Component: Client: Desktop → Telemetry
Product: Firefox Health Report → Toolkit
(Reporter)

Updated

4 years ago
Blocks: 1134775
(Assignee)

Comment 49

4 years ago
Created attachment 8566679 [details] [diff] [review]
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?
Attachment #8566538 - Attachment is obsolete: true
Attachment #8566679 - Flags: review?(gfritzsche)
Attachment #8566679 - Flags: feedback?(dtownsend)
(Reporter)

Updated

4 years ago
No longer blocks: 1134775
(Reporter)

Comment 50

4 years ago
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()
Attachment #8566679 - Flags: review?(gfritzsche) → review+
(Reporter)

Comment 51

4 years ago
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()
Attachment #8566540 - Flags: review?(gfritzsche) → review+
Comment on attachment 8566679 [details] [diff] [review]
part 2 - Addon change notification (v6)

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

Looks good
Attachment #8566679 - Flags: feedback?(dtownsend) → feedback+
(Assignee)

Comment 53

4 years ago
Created attachment 8566969 [details] [diff] [review]
part 2 - Addon change notification (v7)

Taken care of the Nits from the last review.
Attachment #8566679 - Attachment is obsolete: true
Attachment #8566969 - Flags: review+
(Assignee)

Comment 54

4 years ago
Created attachment 8567080 [details] [diff] [review]
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.
Attachment #8566540 - Attachment is obsolete: true
Attachment #8567080 - Flags: review+
Attachment #8567080 - Flags: feedback?(gfritzsche)
(Reporter)

Comment 55

4 years ago
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.
Attachment #8567080 - Flags: feedback?(gfritzsche) → feedback+
(Assignee)

Comment 56

4 years ago
Created attachment 8567226 [details] [diff] [review]
part 4 - Add test coverage for addons and plugins (v6)

Thanks for the feedback Georg. Changed the comment.
Attachment #8567080 - Attachment is obsolete: true
Attachment #8567226 - Flags: review+
(Reporter)

Updated

4 years ago
Whiteboard: [ready]
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.