824528 - Addons provider for Firefox Health Report

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Addons provider, v1

We need to collect add-on info for Firefox Health Report.

Attached is my first stab at it. Not tested.

Dave, Blair: I'm looking for feedback on the AddonManager interaction code. I don't imagine those parts will be changing before final revision.

I hit up both of you because I'm not sure what Blair's status is. Feel free to punt to the other ;)

Richard: For the initial revision, I'm going to "cheat" and store JSON in storage. We can adapt a richer backend format later if we so chose.

Comment 1

[Gregory Szorc [:gps]]

Oh, the FHR spec says we are supposed to collect a "has binary component" flag for each add-on. Looking at https://developer.mozilla.org/en-US/docs/Addons/Add-on_Manager/Addon, I don't see an obvious way to do this. Help?

Comment 2

[Daniel Einspanjer [:dre] [:deinspanjer]]

You could take a look at the original prototype code.  It collected all of the desired information about add-ons.

Comment 3

[Gregory Szorc [:gps]]

(In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #2)
> You could take a look at the original prototype code.  It collected all of
> the desired information about add-ons.

Oh, I've been liberally lifting the original prototype code! However, the patch I've been looking at doesn't have any mention of add-ons. If you could point me to the patch that does, I'll look at it!

Comment 4

[Mark Reid [:mreid]]

(In reply to Gregory Szorc [:gps] from comment #3)
> (In reply to Daniel Einspanjer :dre [:deinspanjer] from comment #2)
> > You could take a look at the original prototype code.  It collected all of
> > the desired information about add-ons.
> 
> Oh, I've been liberally lifting the original prototype code! However, the
> patch I've been looking at doesn't have any mention of add-ons. If you could
> point me to the patch that does, I'll look at it!

It's in the "mdp-v2-10-main-metrics-gatherdata.patch" file in the prototype.

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 695537 [details] [diff] [review]
Addons provider, v1

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

::: services/healthreport/addonsprovider.jsm
@@ +42,5 @@
> +
> +  // Whenever these AddonListener callbacks are called, we repopulate
> +  // and store the set of addons. Note that these events will only fire
> +  // for restartless add-ons. For actions that require a restart, we
> +  // will catch the change after restart. The alternative is a lot of

What triggers the collection after restart? I presume it doesn't run on every startup.

@@ +110,5 @@
> +        appDisabled: addon.appDisabled,
> +        version: addon.version,
> +        type: addon.type,
> +        scope: addon.scope,
> +        foreignInstall: addon.foreignInstall,

Seems like you could simplify this to:

for (let prop of ["userDisabled", "appDisabled", ...]) obj[prop] = addon[prop];

Less vulnerable to typos.

@@ +121,5 @@
> +      if (addon.updateDate) {
> +        obj.updateDay = addon.updateDate.getTime() / MILLISECONDS_PER_DAY;
> +      }
> +
> +      // TODO hasbinaryComponents

Is there any special processing to be done to this?

Comment 6

[Gregory Szorc [:gps]]

(In reply to Dave Townsend (:Mossop) from comment #5)
> > +  // Whenever these AddonListener callbacks are called, we repopulate
> > +  // and store the set of addons. Note that these events will only fire
> > +  // for restartless add-ons. For actions that require a restart, we
> > +  // will catch the change after restart. The alternative is a lot of
> 
> What triggers the collection after restart? I presume it doesn't run on
> every startup.

It will run at app startup but execution will be delayed by a few seconds (at least 10) so it doesn't adversely affect startup time. Is there an "Addon Manager is ready" event I should be waiting for?

Comment 7

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #6)
> (In reply to Dave Townsend (:Mossop) from comment #5)
> > > +  // Whenever these AddonListener callbacks are called, we repopulate
> > > +  // and store the set of addons. Note that these events will only fire
> > > +  // for restartless add-ons. For actions that require a restart, we
> > > +  // will catch the change after restart. The alternative is a lot of
> > 
> > What triggers the collection after restart? I presume it doesn't run on
> > every startup.
> 
> It will run at app startup but execution will be delayed by a few seconds
> (at least 10) so it doesn't adversely affect startup time. Is there an
> "Addon Manager is ready" event I should be waiting for?

The Add-ons Manager is always ready, but a few seconds sounds short enough that loading the add-ons database could adversely affect startup. Even 10 seconds sounds too short to me. Make sure you have the snappy guys looped into this.

Comment 8

[Gregory Szorc [:gps]]

One of the requirements for FHR is that it not adversely affect application performance in any noticeable way. I have already been talking with Taras, others about perf impacts of all this work. But, loading the add-ons database was a new one to me (I thought it was loaded as part of start-up). Thanks for letting us know.

Comment 9

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #8)
> One of the requirements for FHR is that it not adversely affect application
> performance in any noticeable way. I have already been talking with Taras,
> others about perf impacts of all this work. But, loading the add-ons
> database was a new one to me (I thought it was loaded as part of start-up).
> Thanks for letting us know.

We do our best to avoid loading it, but basically the first person to call the Add-ons Manager after that incurs the penalty. Reads are async so shouldn't be too much of a problem but IIRC opening the database connection itself is a sync operation.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Addons provider, v2

Now with per-day add-on counts!

Incorporated feedback from Mossop.

This could use a lot more tests. Tests touching add-ons are somewhat difficult to implement. See Sync's tests. Before I go down that road again, I'd like to see if I can convince the add-on people to refactor the test code so some helpful functions are available as testing-only JSMs. But, I don't think that's here or now.

I've verified this provider works with my main profile if that means anything.

The FHR probe spreadsheet has the requirement to not submit add-ons that have the "dont-submit-id flag set by the developer." I have no clue what that refers to but mreid's older version of the code checks the pref:

  "extensions." + addon.id + ".getAddons.cache.enabled";

Mossop: Can you confirm that accessing this pref is the preferred way to determine "dont-submit-id" for an add-on?

Comment 11

[Gregory Szorc [:gps]]

Attachment: Addons provider, v3

Proper patch, ugh.

Comment 12

[Dave Townsend [:mossop]]

(In reply to Gregory Szorc [:gps] from comment #10)
> Created attachment 698215 [details] [diff] [review]
> Addons provider, v2
> 
> Now with per-day add-on counts!
> 
> Incorporated feedback from Mossop.
> 
> This could use a lot more tests. Tests touching add-ons are somewhat
> difficult to implement. See Sync's tests. Before I go down that road again,
> I'd like to see if I can convince the add-on people to refactor the test
> code so some helpful functions are available as testing-only JSMs. But, I
> don't think that's here or now.
> 
> I've verified this provider works with my main profile if that means
> anything.
> 
> The FHR probe spreadsheet has the requirement to not submit add-ons that
> have the "dont-submit-id flag set by the developer." I have no clue what
> that refers to but mreid's older version of the code checks the pref:
> 
>   "extensions." + addon.id + ".getAddons.cache.enabled";
> 
> Mossop: Can you confirm that accessing this pref is the preferred way to
> determine "dont-submit-id" for an add-on?

That's correct, per https://developer.mozilla.org/en-US/docs/Addons/Working_with_AMO

Comment 13

[Gregory Szorc [:gps]]

Attachment: Addons provider, v4

Now with opt-out. Thanks Mossop!

Comment 14

[Richard Newman [:rnewman]]

Comment on attachment 698218 [details] [diff] [review]
Addons provider, v4

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

Obviously needs tweaks for underlying changes, but looks good.

::: services/healthreport/providers.jsm
@@ +612,5 @@
> +/**
> + * Stores the set of active addons in storage.
> + *
> + * We do things a little differently than most other measurements. Because
> + * addons are difficult to shoehorn into distinct fileds, we simply store a

s/fileds/fields

@@ +630,5 @@
> +    return this.registerStorageField("addons", this.storage.FIELD_LAST_TEXT);
> +  },
> +
> +  serialize: function (data, format, day) {
> +    if (format != this.SERIALIZE_JSON) {

This'll need tweaking.

@@ +646,5 @@
> +
> +    let json = data.get("addons")[1];
> +
> +    // Exceptions are caught in caller.
> +    return JSON.parse(json);

This makes me a little sad perf-wise -- we'll be parsing each blob of addons JS over and over -- but we can address this later, perhaps by having a method on the JSON serializer that's initially equivalent to JSON.stringify, but for this blob will be passthrough?

@@ +804,5 @@
> +      if (!data.counts[type]) {
> +        data.counts[type] = 0;
> +      }
> +
> +      data.counts[type]++;

Replace both clauses with just one line:

  data.counts[type] = (data.counts[type] || 0) + 1;

::: services/healthreport/tests/xpcshell/test_provider_addons.js
@@ +16,5 @@
> +}
> +
> +add_test(function test_constructor() {
> +  let provider = new AddonsProvider();
> +

Do you need to init test logging here?

@@ +45,5 @@
> +  do_check_true(data.singular.has("addons"));
> +
> +  let json = data.singular.get("addons")[1];
> +  let addons = JSON.parse(json);
> +  do_check_eq(typeof(addons), "object");

Test serialization here, too.

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/1bbcfe1c6608

Complete with oh-so-hacky monkeypatching to bypass the AddonManager. Will file follow-up bug.

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/1bbcfe1c6608

Comment 17

[Mark Reid [:mreid]]

The map of full addon detail (as created by the "_createDataStructure" function) should only include data for addons of type "extension" and "plugin".

The addon counts by type are correct in that they should include all types of addons.