928575 - Rework addon data format for desktop FHR

Status: RESOLVED FIXED

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

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Rework plugin & extension data

* Plugins have a different data format now
* Both extensions and plugins get human readable and (if present) descriptions

This contains the changes needed for desktop.

https://tbpl.mozilla.org/?tree=Try&rev=8e5c2a3238a7

Comment 2

[Georg Fritzsche [:gfritzsche]]

Side note: i don't think it's worth the time to separate out the small changes for extensions. It's all small changes to the addons provider anyway.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Fixed try run: https://tbpl.mozilla.org/?tree=Try&rev=cb86032b0504

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Rework plugin & extension data, desktop, v2

I missed updating the documentation.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8364477 [details] [diff] [review]
Rework plugin & extension data, desktop, v2

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

I can't comment on how proper it is to use nsIPluginHost to populate this data. But I know you know how plugins work and am willing to trust you on this. Still, we may want an extra review from bsmedberg on just those bits.

The FHR-specific stuff looks mostly fine. I have a few nitpicks, but nothing too major.

Main thread I/O via nsIFile is the only hard blocker.

::: services/healthreport/docs/dataformat.rst
@@ +534,5 @@
>  -------------------------
>  
>  This measurement contains information about the currently-installed add-ons.
>  
> +Version 2

You need to preserve the documentation for all old versions.

::: services/healthreport/providers.jsm
@@ +911,5 @@
> +    function getUserdisabled(tag) {
> +      if (tag.disabled)
> +        return true;
> +      if (tag.clicktoplay)
> +        return "askToActivate";

Nit: Always use braces: it helps avoid future bugs due to refactoring and keeps diffs more readable.

@@ +918,5 @@
> +
> +    function getModifiedDay(provider, tag) {
> +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +      file.initWithPath(tag.fullpath);
> +      return provider._dateToDays(new Date(file.lastModifiedTime));

This performs main thread I/O and may cause jank. Use OS.File.stat().

@@ +927,5 @@
> +                       getPluginTags({});
> +
> +    for (let tag of pluginTags) {
> +      data.counts["plugin"] = (data.counts["plugin"] || 0) + 1;
> +      let obj = {

For peace of mind, could you please document somewhere here that we do not store the plugin ID because it can be used for fingerprinting. I worry someone may come along in the future and want to store it. A comment should help prevent that.

@@ +936,5 @@
> +        appDisabled: tag.blocklisted,
> +        userDisabled: getUserdisabled(tag),
> +        updateDay: getModifiedDay(this, tag),
> +        mimeTypes: tag.getMimeTypes({}),
> +      };

It feels odd to me that we're grabbing non-plugin add-ons and plugins from different sources yet stuffing them in the same list.
The overlap between the two sources seems significant.

Please answer the following questions:

1) Why can't we treat plugins specially when iterating over the add-ons from the AddonsManager? i.e. what's available in nsIPluginHost that isn't reported by the AddonsManager?
2) Why don't we put add-ons and plugins in separate lists in the FHR data?

::: services/healthreport/tests/xpcshell/test_provider_addons.js
@@ +171,5 @@
> +  let addonIds = getAddonIds(value);
> +  do_check_true(addonIds.indexOf("addon0") != -1);
> +  do_check_true(addonIds.indexOf("addon1") == -1);
> +  do_check_true(addonIds.indexOf("addon2") == -1);
> +  do_check_true(addonIds.indexOf("addon3") != -1);

You seem to be using addonIds as a set. Might as well have getAddonIds return a Set.

Comment 6

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #5)
> @@ +918,5 @@
> > +
> > +    function getModifiedDay(provider, tag) {
> > +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> > +      file.initWithPath(tag.fullpath);
> > +      return provider._dateToDays(new Date(file.lastModifiedTime));
> 
> This performs main thread I/O and may cause jank. Use OS.File.stat().

As far as i can tell this doesn't really change things here. Without this patch we hit the same problem:
http://hg.mozilla.org/mozilla-central/annotate/9d650c07b547/toolkit/mozapps/extensions/PluginProvider.jsm#l421

I'm wondering though why we don't store this as a property on the plugin tags - we need to hit the file system
anyway when scanning plugins, so i would rather store it at that point in a follow-up bug.
Does that sound reasonable?

> 1) Why can't we treat plugins specially when iterating over the add-ons from
> the AddonsManager? i.e. what's available in nsIPluginHost that isn't
> reported by the AddonsManager?

This change is per bug 922318, comment 5. AFAIR especially the grouping PluginProvider
does might lead to unexpected results and we can't get to the plugin tags directly via it's interface.
Getting the data directly from the plugin host & tags seems most reliable here.

> 2) Why don't we put add-ons and plugins in separate lists in the FHR data?

Good point, let's do that.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> (In reply to Gregory Szorc [:gps] from comment #5)
> > 2) Why don't we put add-ons and plugins in separate lists in the FHR data?
> 
> Good point, let's do that.

Does this seem reasonable:

"org.mozilla.addons.active": {
  "_v": 2,
  "addons": {
    // ... as before, but without plugins
  },
  "plugins": [
    // ... plugin data list (they don't have unique identifiers)
  ]
}

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Rework plugin & extension data, desktop, v3

This should cover the comments so far.

Comment 9

[Gregory Szorc [:gps]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> (In reply to Gregory Szorc [:gps] from comment #5)
> > @@ +918,5 @@
> > > +
> > > +    function getModifiedDay(provider, tag) {
> > > +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> > > +      file.initWithPath(tag.fullpath);
> > > +      return provider._dateToDays(new Date(file.lastModifiedTime));
> > 
> > This performs main thread I/O and may cause jank. Use OS.File.stat().
> 
> As far as i can tell this doesn't really change things here. Without this
> patch we hit the same problem:
> http://hg.mozilla.org/mozilla-central/annotate/9d650c07b547/toolkit/mozapps/
> extensions/PluginProvider.jsm#l421
> 
> I'm wondering though why we don't store this as a property on the plugin
> tags - we need to hit the file system
> anyway when scanning plugins, so i would rather store it at that point in a
> follow-up bug.
> Does that sound reasonable?

Capturing it as part of plugin scanning in a follow-up sounds reasonable.

The only point I was trying to make is "nsIFile usage from the main thread is considered harmful [most of the time]." Just because nsIPluginHost does synchronous I/O (I assume) doesn't give this code the excuse to do it as well. The rule is no new main thread I/O. There are few exceptions. Lots of old code is grandfathered in. I assume nsIPluginHost hasn't been converted because it's not used frequently enough to cause performance concerns?

> > 1) Why can't we treat plugins specially when iterating over the add-ons from
> > the AddonsManager? i.e. what's available in nsIPluginHost that isn't
> > reported by the AddonsManager?
> 
> This change is per bug 922318, comment 5. AFAIR especially the grouping
> PluginProvider
> does might lead to unexpected results and we can't get to the plugin tags
> directly via it's interface.
> Getting the data directly from the plugin host & tags seems most reliable
> here.

Follow-up: why aren't these things exposed to AddonsManager? They could be, right? Should they be?

I'm not opposed to using nsIPluginHost. Just trying to figure all this out.

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8365121 [details] [diff] [review]
Rework plugin & extension data, desktop, v3

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

::: services/healthreport/providers.jsm
@@ +876,5 @@
>  
>    _createDataStructure: function (addons) {
> +    let data = {
> +      addons: {"extensions": {}, "plugins": []},
> +      counts: {}

<bikeshed> I'd put plugins as a top-level property and leave addons as-is.

@@ +916,5 @@
> +    }
> +
> +    let pluginTags = Cc["@mozilla.org/plugin/host;1"].
> +                       getService(Ci.nsIPluginHost).
> +                       getPluginTags({});

Nit: The style convention is to put the period on the following line.

@@ +929,5 @@
> +        description: tag.description,
> +        appDisabled: tag.blocklisted,
> +        userDisabled: tag.disabled || (tag.clicktoplay && "askToActivate"),
> +        updateDay: getModifiedDay(this, tag),
> +        mimeTypes: tag.getMimeTypes({}),

Now that plugins are a separate list, IMO it doesn't make sense to shoehorn this data structure into what we use for regular add-ons.

I think storing tag.blocklisted as "blocklisted" (not appDisabled) is fine. We should also consider storing tag.disabled and tag.clicktoplay separately and/or as-is. Is there a benefit to that?

Feel free to disagree: I don't have a super strong opinion on this.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #9)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> > As far as i can tell this doesn't really change things here. Without this
> > patch we hit the same problem:
> > http://hg.mozilla.org/mozilla-central/annotate/9d650c07b547/toolkit/mozapps/
> > extensions/PluginProvider.jsm#l421
> > 
> > I'm wondering though why we don't store this as a property on the plugin
> > tags - we need to hit the file system
> > anyway when scanning plugins, so i would rather store it at that point in a
> > follow-up bug.
> > Does that sound reasonable?
> 
> Capturing it as part of plugin scanning in a follow-up sounds reasonable.
> 
> The only point I was trying to make is "nsIFile usage from the main thread
> is considered harmful [most of the time]." Just because nsIPluginHost does
> synchronous I/O (I assume) doesn't give this code the excuse to do it as
> well. The rule is no new main thread I/O. There are few exceptions. Lots of
> old code is grandfathered in. I assume nsIPluginHost hasn't been converted
> because it's not used frequently enough to cause performance concerns?

So, i was just saying that this basically hit the same statements in the linked addon manager parts before and hence not adding new main thread IO. That part just didn't stand out enough before i guess?
I think i can add this on the plugin tag and allow both the addon manager and FHR to use it.

> Follow-up: why aren't these things exposed to AddonsManager? They could be,
> right? Should they be?
> 
> I'm not opposed to using nsIPluginHost. Just trying to figure all this out.

I'm not sure if the plugin tags should be exposed if the addon manager is trying to wrap/abstract from them.
We can & should fix e.g. the grouping in the addon manager and it's caching of data (if that wasn't fixed yet), but that won't happen right away.

On the other hand we can just get the data directly from nsIPluginHost without these concerns and the plugin data differs sufficiently to handle it separately anyway.

Comment 12

[Gregory Szorc [:gps]]

In general, if you have the opportunity to replace synchronous main thread I/O with non-blocking I/O, do it. The main reason we still have main thread I/O is that we don't have the resources to fix all the legacy offenders.

Comment 13

[Benjamin Smedberg]

Since we have to get the plugin data from the plugin host, we don't have that opportunity here. There are several other bugs about making that asynchronous, tied up in web-compat concerns.

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #10)
> <bikeshed> I'd put plugins as a top-level property and leave addons as-is.

Hm, so leave "org.mozilla.addons.active" as is and add plugins as say "org.mozilla.addons.plugins"?

Comment 15

[Richard Newman [:rnewman]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> > (In reply to Gregory Szorc [:gps] from comment #5)
> > > 2) Why don't we put add-ons and plugins in separate lists in the FHR data?
> > 
> > Good point, let's do that.
> 
> Does this seem reasonable:
> 
> "org.mozilla.addons.active": {
>   "_v": 2,
>   "addons": {
>     // ... as before, but without plugins
>   },
>   "plugins": [
>     // ... plugin data list (they don't have unique identifiers)
>   ]
> }

Addons are part of the v3 environment. Speaking solely to that format, not desktop's legacy v2 format: 

* We're going to need to bump the environment version regardless, because the contents of plugin records will change, as will the list of add-ons.

* The plugin list must be consistently sorted (or we must consistently sort it), because these values *must* consistently hash to the same value. That means that either the list we get from storage must be ordered, or there has to be a value in each plugin record that we can use to do that.

* We already have code for handling {} here, not [], so having no consistent identifier for plugins is vaguely annoying (the identifier is how we sort add-ons).

* I would strongly favor creating org.mozilla.plugins.active, rather than nesting in o.m.addons.active. It makes environment handling much more straightforward.

Comment 16

[Georg Fritzsche [:gfritzsche]]

We don't have a unique consistent identifier for plugins like extension ids and we can have multiple versions of the same plugin installed.
Maybe name+description+version would work. Do we have to do this on the clients though?

> * I would strongly favor creating org.mozilla.plugins.active, rather than
> nesting in o.m.addons.active. It makes environment handling much more
> straightforward.

gps, does that also sound good to you?

Comment 17

[Richard Newman [:rnewman]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> We don't have a unique consistent identifier for plugins like extension ids
> and we can have multiple versions of the same plugin installed.
> Maybe name+description+version would work. Do we have to do this on the
> clients though?

The goal is that two identical phones, with identical versions of Fennec installed, and identical plugins, will have an identical environment hash. That work must be done on the client.

That implies that they'll have the same JSON representations of each plugin, sorted in the same order.

A reasonable solution IMO is to hash the plugin, or (as a stand-in) the stable part of the plugin descriptor, to compute a stable ID for the plugin.

(Perhaps that could be part of the plugin record itself, so FHR doesn't have to care? It seems that the current plugin ID isn't really useful.)

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Richard Newman [:rnewman] from comment #17)
> A reasonable solution IMO is to hash the plugin

That would definitely add new I/O.

> or (as a stand-in) the
> stable part of the plugin descriptor, to compute a stable ID for the plugin.

As mentioned, that could be based on name+description+version, so we end up with something identifying a specific version of a plugin. We will submit those fields though, so that wouldn't need to be done on the client.

> (Perhaps that could be part of the plugin record itself, so FHR doesn't have to care? It seems that the current plugin ID isn't really useful.)

I think we'll only use that for FHR, so i don't see a need to add this to the core code.


> 
> (Perhaps that could be part of the plugin record itself, so FHR doesn't have
> to care? It seems that the current plugin ID isn't really useful.)

Comment 19

[Benjamin Smedberg]

> The goal is that two identical phones, with identical versions of Fennec installed, and identical plugins,
> will have an identical environment hash.

What is important about this goal? I think we should just use the plugin name/description/filename/version or somesuch. We certainly don't want to hash the actual plugin .dll: I/O costs are high and value seems low.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> > The goal is that two identical phones, with identical versions of Fennec installed, and identical plugins,
> > will have an identical environment hash.
> 
> What is important about this goal?

It's how the v3 document format works. Same environment => same environment hash.


> We certainly don't want to
> hash the actual plugin .dll: I/O costs are high and value seems low.

Fine by me.


> As mentioned, that could be based on name+description+version, so we end up
> with something identifying a specific version of a plugin. We will submit
> those fields though, so that wouldn't need to be done on the client.

Plugins are part of the environment; changing plugins will affect crashes and performance, for example. That means this needs to be done on the client.


> > (Perhaps that could be part of the plugin record itself, so FHR doesn't have to care? It seems that the current plugin ID isn't really useful.)
> 
> I think we'll only use that for FHR, so i don't see a need to add this to
> the core code.

Just depends on whether you want two different implementations of the identifier computation (one desktop, one Android). No big deal.

Comment 21

[Georg Fritzsche [:gfritzsche]]

(In reply to Richard Newman [:rnewman] from comment #20)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #19)
> > > The goal is that two identical phones, with identical versions of Fennec installed, and identical plugins,
> > > will have an identical environment hash.
> > 
> > What is important about this goal?
> 
> It's how the v3 document format works. Same environment => same environment
> hash.

Can we change it (with version bump or not) to not require hashs for plugins?

Comment 22

[Gregory Szorc [:gps]]

Can we just punt the v3/stable ordering bits until desktop actually uses v3? Although, I think I told Metrics that may not be far off, so maybe that's not such a good idea :(

Comment 23

[Richard Newman [:rnewman]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #21)

> Can we change it (with version bump or not) to not require hashs for plugins?

Either plugins are included in the environment, and are thus longitudinal dividers for events, or they're not included in the environment.

The former is obviously preferred, but requires that plugins have a consistent ordering and can be somehow hashed to a consistent value in order to contribute to the environment's hash.

See <http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/healthreport/EnvironmentV1.java#209> for how this would work.

The latter involves you building out a new type of measurement (daily) for FHR on Android, and renders any kind of plugin-related observation suspect -- environments would no longer be accurate buckets for some kinds of events.

It seems straightforward to me to compute an identifier for a plugin (filename + name + description + version), and contribute all of the fields of each plugin to the environment, just as we do now (by virtue of their presence in the add-ons list). We have all of the fields. So why the pushback?

Comment 24

[Georg Fritzsche [:gfritzsche]]

I was wondering what we need it for and if we need it to pushed along-side possibly the same fields it is based on.
Not so much pushback as trying to understand the needs here.

Comment 25

[Richard Newman [:rnewman]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> I was wondering what we need it for and if we need it to pushed along-side
> possibly the same fields it is based on.
> Not so much pushback as trying to understand the needs here.

OK, great. So to reiterate my proposal:

  // These two continue to live in the v3 environment, or
  // the current place in the v2 payload.
  org.mozilla.addons.active: {
    _v: 2,                        // Bumped because no longer includes plugins.
    ...
  },
  org.mozilla.plugins.active: {
    _v: 1,
    "$computedid1": {        // Computed from stable textual plugin fields.
      description: "...",
      appDisabled: ...,
      ...
    },
    "$computedid2": {
      ...
    }
  }

  ...

  // This is unchanged.
  org.mozilla.addons.counts {
    ...
  }

Comment 26

[Richard Newman [:rnewman]]

And, for plugins that decline to provide textual details, I suggest using the opaque local plugin ID, or something else unique. Multiple anonymous plugins will collide, and we need to avoid that.

Comment 27

[Georg Fritzsche [:gfritzsche]]

Ok,(In reply to Richard Newman [:rnewman] from comment #25)
> OK, great. So to reiterate my proposal:
[...]
>   // This is unchanged.
>   org.mozilla.addons.counts {
>     ...
>   }

Should we still track plugins in this?

> And, for plugins that decline to provide textual details, I suggest using
> the opaque local plugin ID, or something else unique. Multiple anonymous
> plugins will collide, and we need to avoid that.

We don't something like a local plugin id (besides the path, but that is potentially sensitive data).
I'm not sure if we even support for a plugin to provide empty name & description. If we do and there are actually any around i haven't seen it yet.

Comment 28

[Benjamin Smedberg]

Plugins have to have a name and a filename. They don't have to have a description or version but in those cases we just treat it as the empty string. So I don't think we need to generate any kind of ID, just use "<filename>:<name>:<version>:<description>"

Comment 29

[Richard Newman [:rnewman]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #28)
> Plugins have to have a name and a filename.

Great!

(In reply to Georg Fritzsche [:gfritzsche] from comment #27)
> Should we still track plugins in this?

I think so. The counts are broken out by type anyway.

Comment 30

[Georg Fritzsche [:gfritzsche]]

Attachment: Rework plugin & extension data, desktop, v4

Speculatively putting this up assuming that we'll end up with something roughly like the previous comments.

* Moving plugin data reporting out of org.mozilla.addons.active means adding a new provider AFAICT.
* I'm holding back on documentation changes until we have settled for something.
* Counting & reporting plugins in 2 different providers feels weird, but i don't really mind either way.

Comment 31

[Georg Fritzsche [:gfritzsche]]

Attachment: Rework plugin & extension data, desktop, v5

The data format is settled now, so here is the patch including documentation changes.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=931bdacdf1cf

Notes still applying:
* Moving plugin data reporting out of org.mozilla.addons.active means adding a new provider AFAICT.
* Counting & reporting plugins in 2 different providers feels weird, but i don't really mind either way.

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8367505 [details] [diff] [review]
Rework plugin & extension data, desktop, v5

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

Ugh. I'm not a fan of the multiple providers, especially since there's a lot of copied or at least very similar code in here.

Rule of thumb: if you copy more than a line of non-boilerplate code, chances are you should be sharing that code instead.

Options:

1) Refactor the shared bits of the provider into a base type
2) Move the plugins code back into the AddonsProvider as a separate measurement
3) Leave the measurements combined
4) Create a new, refactored and unified provider with both addons and plugins data

I like #2 or #4.

I would strongly prefer we address that before landing. I hate "lowering the bar" to get this in before 29. But I'm prepared to relegate it to a follow-up. But that limits us to #1 unless we want to introduce yet another measurement value for Metrics to deal with. I think it makes sense to minimize measurement churn and do the refactoring now.

::: services/healthreport/docs/dataformat.rst
@@ +660,5 @@
>  
> +org.mozilla.plugins.active
> +-------------------------
> +
> +This measurement contains information about the currently-installed add-ons.

plugins.

@@ +683,5 @@
> +* mimeTypes
> +* updateDay
> +
> +With the exception of *updateDay* and *mimeTypes*, all these properties come
> +directly from nsIPluginTag via nsIPluginHost.

Surround the interface names with `` `` to get <code> treatment.

::: services/healthreport/providers.jsm
@@ +910,5 @@
>  
> +    let pluginTags = Cc["@mozilla.org/plugin/host;1"]
> +                       .getService(Ci.nsIPluginHost)
> +                       .getPluginTags({});
> +    data.counts["plugin"] = (data.counts["plugin"] || 0) + pluginTags.length;

data.counts.plugin should always be undefined, right? Why not assign directly?

@@ +1048,5 @@
> +
> +    function getModifiedDay(provider, tag) {
> +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> +      file.initWithPath(tag.fullpath);
> +      return provider._dateToDays(new Date(file.lastModifiedTime));

No new synchronous I/O!

::: services/healthreport/tests/xpcshell/test_provider_addons.js
@@ +155,5 @@
>  
>    value = data.days.getDay(now);
>    do_check_eq(value.size, 4);
>    do_check_eq(value.get("extension"), 1);
> +  do_check_eq(value.get("plugin"), 2);

Why did this increase?

Comment 33

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #32)
> @@ +1048,5 @@
> > +
> > +    function getModifiedDay(provider, tag) {
> > +      let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
> > +      file.initWithPath(tag.fullpath);
> > +      return provider._dateToDays(new Date(file.lastModifiedTime));
> 
> No new synchronous I/O!

I thought per comment 9 et al we are ok with a follow-up? That is bug 964272.

> 
> ::: services/healthreport/tests/xpcshell/test_provider_addons.js
> @@ +155,5 @@
> >  
> >    value = data.days.getDay(now);
> >    do_check_eq(value.size, 4);
> >    do_check_eq(value.get("extension"), 1);
> > +  do_check_eq(value.get("plugin"), 2);
> 
> Why did this increase?

Because we are counting our 2 test plugins now, not the plugin entry that the test provides.

Comment 34

[Georg Fritzsche [:gfritzsche]]

Attachment: Rework plugin & extension data, desktop, v6

Merged the providers back again per IRC discussion, we now have org.mozilla.addons.addons and .plugins

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8367609 [details] [diff] [review]
Rework plugin & extension data, desktop, v6

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

Looks good to me!

Comment 36

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c78f9d502f0f

Comment 37

[Carsten Book [:Tomcat]]

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

Comment 38

[Guilherme Lima]

On about:healthreport I'm now seeing

Add-ons
enabled 0
disabled 0

Plugins
enabled 0
disabled 0

fallout from this bug?

Comment 39

[Benjamin Smedberg]

Filed bug 966871 on comment 38: that's a server-side change so it doesn't need to block the trains.