1053745 - Include OpenH264 plugin in FHR data

Status: VERIFIED FIXED

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

Description

[Georg Fritzsche [:gfritzsche]]

We should include the OpenH264 plugin data in FHR.

This would go into the AddonsProvider. Ideally we just add it to "org.mozilla.addons.plugins" with an empty "mimeTypes" field so we don't need to change the data format.

http://hg.mozilla.org/mozilla-central/annotate/d7e78f0c1465/services/healthreport/providers.jsm#l937

Comment 1

[Maire Reavy [:mreavy]]

Should we plan to uplift this to Fx33?

Comment 2

[Benjamin Smedberg]

Yes, I think we should get this into Fx33, per the dev.media discussion.

Comment 3

[Georg Fritzsche [:gfritzsche]]

Gavin, can we get this in the next iteration?

Comment 4

[Georg Fritzsche [:gfritzsche]]

The straight-forward way would be to add this to the plugins data we already collect around here:
http://hg.mozilla.org/mozilla-central/annotate/738469449872/services/healthreport/providers.jsm#l973

We can just grab the data from the |addons| list, the id we are looking for is found here:
http://hg.mozilla.org/mozilla-central/annotate/738469449872/toolkit/mozapps/extensions/internal/OpenH264Provider.jsm#l29

Comment 5

[Marco Mucci [:MarcoM]]

Hi Georg, this bug is currently part of the IT 35.1 priority sheet.

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Gavin, can we get this in the next iteration?

Comment 6

[Benjamin Smedberg]

Attachment: bug1053745-gmp-fhr

Boy this is a lot of boilerplate...

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8483894 [details] [diff] [review]
bug1053745-gmp-fhr

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

::: services/healthreport/providers.jsm
@@ +987,5 @@
>      for (let addon of addons) {
>        let type = addon.type;
>  
>        // We count plugins separately below.
> +      if (addon.type == "plugin" && addon.pluginMimeTypes !== undefined) {

Looking at the pluginMimeTypes getter in OpenH264Provider, this would always make us skip reporting the OpenH264 plugin, no?
What do we actually need this check for, can't we just |continue| when type=="plugin" && !addon.id.startsWith("gmp-")?

@@ +988,5 @@
>        let type = addon.type;
>  
>        // We count plugins separately below.
> +      if (addon.type == "plugin" && addon.pluginMimeTypes !== undefined) {
> +        dump("It's a plugin, skipping\n");

Let's kill this dump() here or make it this._log.info().

@@ +993,4 @@
>          continue;
> +      }
> +      if (addon.type == "plugin" && addon.id.startsWith("gmp-")) {
> +        dump("It's a GMP plugin\n");

Another dump() to get rid of.

Comment 8

[Benjamin Smedberg]

Attachment: bug1053745-gmp-fhr, rev. 2

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8484492 [details] [diff] [review]
bug1053745-gmp-fhr, rev. 2

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

Looks good with the comments below addressed.

::: services/healthreport/providers.jsm
@@ +937,5 @@
>              return plugins.setLastText("plugins", pluginsField).then(
> +              function onSuccess() {
> +                return gmpPlugins.setLastText("gmp-plugins", gmpPluginsField).then(
> +                  function onSuccess() {
> +                    deferred.resolve();

Yikes, quite some nesting. We should consider switching to a Task at some point.

@@ +988,5 @@
>        let type = addon.type;
>  
>        // We count plugins separately below.
> +      if (addon.type == "plugin") {
> +        if (addon.gmPlugin) {

For the data here you call them gmpPlugins/gmp-plugins/GMPPlugins, the addon property |gmPlugin| (with just the gm prefix) - can we stick with one prefix, "gmp"?

@@ +991,5 @@
> +      if (addon.type == "plugin") {
> +        if (addon.gmPlugin) {
> +          data.gmpPlugins[addon.id] = {
> +            version: addon.version,
> +            userDisabled: addon.userDisabled

Are we potentially interested in |applyBackgroundUpdates| as well?

::: toolkit/mozapps/extensions/internal/OpenH264Provider.jsm
@@ +86,5 @@
>    optionsURL: OPENH264_OPTIONS_URL,
>  
>    get id() { return OPENH264_PLUGIN_ID; },
>    get type() { return "plugin"; },
> +  get gmPlugin() { return true; },

Going with the naming schemes here, we should make it e.g. |isGMPlugin| (or rather |isGMPPlugin| per the providers.jsm comment).

Comment 10

[Benjamin Smedberg]

https://hg.mozilla.org/integration/fx-team/rev/25c524c5af2f

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/25c524c5af2f

Comment 12

[Benjamin Smedberg]

Comment on attachment 8484492 [details] [diff] [review]
bug1053745-gmp-fhr, rev. 2

Approval Request Comment
[Feature/regressing bug #]: OpenH264 feature
[User impact if declined]: we won't know whether users have OpenH264 functioning properly for WebRTC interop
[Describe test coverage new/current, TBPL]: manual and automated testing
[Risks and why]: Small risk, primarily to the FHR data-collection system
[String/UUID change made/needed]: none

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm still not clear on the review process for new FHR probes. I don't think that this specific case is a problem but would like to ensure that we're gating FHR probes correctly. 

bsmedberg/jjensen - Is review required before adding a probe like this? Can you explain the process so that I'm clear for future FHR requests?

Comment 14

[Benjamin Smedberg]

For a while now I've been the decision maker on probes, at least for desktop. I'll bring in legal/privacy if needed, but in most cases that's not necessary.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8484492 [details] [diff] [review]
bug1053745-gmp-fhr, rev. 2

Thanks for clarifying Benjamin. Approved for Beta and Aurora.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/112c18bd9406
https://hg.mozilla.org/releases/mozilla-beta/rev/02474d192901

Comment 17

[Georg Fritzsche [:gfritzsche]]

I totally missed that this didn't update services/healthreport/docs/dataformat.rst - can we do this in a follow-up?

Comment 18

[Kamil Jozwiak [:kjozwiak]]

Win 8.1 x64 (VM):
- m-c [205443:3b7921328fc1 - BuildID: 20140916030204]
- aurora [217547:e2a4798c3f91 - BuildID: 20140916004001]
- beta [216754:c69dd3a1d850 - BuildID: 20140915204924]

Ubuntu 13.10 x64 (VM):
- m-c [205443:3b7921328fc1 - BuildID: 20140916030204]
- aurora [217547:e2a4798c3f91 - BuildID: 20140916004001]
- beta [216754:c69dd3a1d850 BuildID: 20140915204924]

Test Cases used:

- installed the latest build and ensured that "org.mozilla.addons.gm-plugins" is appearing under about:healthreport
- disabled the plugin and checked to see if the "disabled" field is being added/changed under about:healthreport
- moved the machines date ahead by several days and ensured that the data is still present and correct

Georg/Benjamin, the only information that's currently being displayed under about:healthreport -> Raw Data is:

"org.mozilla.addons.gm-plugins": {
  "_v": 1
},

The above is always static even when disabling the plugin via about:addons. There's no "name", "description" or "disabled" fields being listed. I've disabled/enabled the openh264 plugin several times without anything being changed or added into about:healthreport. Georg mentioned on IRC that there should be a disabled field but I've never seen one being created under "org.mozilla.addons.gm-plugins".

Is this intended or is this a bug or perhaps I'm doing something incorrectly?

Comment 19

[Georg Fritzsche [:gfritzsche]]

Hm, the OpenH264 plugin should be listed in there with its id as the key ("gmp-gmpopenh264") and values for version, userDisabled, applyBackgroundUpdates.

Given that the automated test is fine and things seem to be basically working, i could imagine that the measurement was taken before the OpenH264 plugin was installed.
I'm not sure on a good way to trigger FHR recollection right now.

Comment 20

[Georg Fritzsche [:gfritzsche]]

Richard, do you know what we can call to trigger FHR recollecting from all providers for manual QA?

Comment 21

[Georg Fritzsche [:gfritzsche]]

I think the needinfo for jjensen was cleared up by comment 14.

Comment 22

[Richard Newman [:rnewman]]

If visiting about:healthreport isn't enough, then call .collectMeasurements() on the active HealthReporter instance:

    let reporter = Cc["@mozilla.org/datareporting/service;1"]
                     .getService()
                     .wrappedJSObject
                     .healthReporter;
    reporter.collectMeasurements();

You can use collectAndObtainJSONPayload() to get the payload.

Comment 23

[Kamil Jozwiak [:kjozwiak]]

So I inserted the above code from comment # 22 into the Browser Console and dumped the payload into the Browser Console using:

reporter.collectAndObtainJSONPayload(true).then(
  function onJSON(data) {
    console.log(JSON.stringify(data, null, 2));
  }
);

Once the payload was dumped into the Browser Console, the same data was being displayed. No other information on the h264 plugin is visible.

The strange part is that when I disable other plugins via about:addons, the "disabled": field under the FHR Raw Data display's the correct value so it does look like it's being correctly updated. For some reason, I don't see any other information on the h264 plugin other than:

"org.mozilla.addons.gm-plugins": {
  "_v": 1
}

I also had someone else try this on their local machine that had the latest Nightly and had the same results. Didn't see any plugin information other than "org.mozilla.addons.gm-plugins".

Comment 24

[Benjamin Smedberg]

Yeah ok, this is broken. I don't know why yet, since both the automated tests and my local tests worked :-(

Comment 25

[Marco Mucci [:MarcoM]]

Hi Benjamin, can you resolve this and file a follow up bug that can be worked on it IT 35.2

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #24)
> Yeah ok, this is broken. I don't know why yet, since both the automated
> tests and my local tests worked :-(

Comment 26

[Benjamin Smedberg]

Marco no, this is just broken so it doesn't make sense to use a separate bug.

Comment 27

[Benjamin Smedberg]

Attachment: 1053745-openh264-fhr-fixup

Comment 28

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8501754 [details] [diff] [review]
1053745-openh264-fhr-fixup

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

You need to fix test_provider_addons.js accordingly as well.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/51d3f2b58ac1
https://hg.mozilla.org/mozilla-central/rev/0cb0d9df5a8e

Comment 30

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-10-03-02-01-mozilla-central/

Ensured once the h264 plugin is downloaded/installed, the correct information is being added into FHR:

"org.mozilla.addons.gm-plugins": {
  "gmp-gmpopenh264": {
  "version": "1.1",
  "userDisabled": false,
  "applyBackgroundUpdates": 1
},

- changed "Automatic Updates" to "On" and ensured "applyBackgroundUpdates": was set to 2
- changed "Automatic Updates" to "Off" and ensured "applyBackgroundUpdates": was set to 0
- changed "Automatic Updates" to "Default" and ensured "applyBackgroundUpdates": was set to 1
- ensured that the version under about:addons -> plugins matches "version": under about:healthreport -> Raw Data
- ensured that "userDisabled": appears as false when the plugin is enabled
- selected "Never Activate" and ensured "userDisabled": is set as true
- ensured that the "version": was filled in correctly once the h264 plugin as been downloaded/installed

Went through the following OS's:

- Win 8.1 x64 (VM) - PASSED
- Ubuntu 14.0.4.1 x64 (VM) - PASSED
- OSX 10.9.5 x64 (VM) - PASSED

Benjamin, let me know if you see any cases that I missed and should be tested.

Comment 31

[Benjamin Smedberg]

Comment on attachment 8501754 [details] [diff] [review]
1053745-openh264-fhr-fixup

Approval Request Comment
[Feature/regressing bug #]: OpenH264 feature
[User impact if declined]: Can't measure effective installation of OpenH264 or user-disable rate
[Describe test coverage new/current, TBPL]: Automated tests and manual verification
[Risks and why]: Fairly low risk
[String/UUID change made/needed]: none

Comment 32

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8501754 [details] [diff] [review]
1053745-openh264-fhr-fixup

Aurora+

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e5b1ec4a370
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a81731c2735

Comment 34

[Kamil Jozwiak [:kjozwiak]]

Went through verification using the following build:
- https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-10-14-00-40-01-mozilla-aurora/

Used the same test cases outlined in comment #30 without any issues.

- Win 8.1 x64 (VM) - PASSED
- Ubuntu 14.0.4.1 x64 (VM) - PASSED
- OSX 10.9.5 x64 (VM) - PASSED

Marking as verified as fx33 is set as "wontfix".