Closed Bug 1053745 Opened 5 years ago Closed 5 years ago

Include OpenH264 plugin in FHR data

Categories

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

defect
Not set

Tracking

(firefox32 unaffected, firefox33 wontfix, firefox34 verified, firefox35 verified)

VERIFIED FIXED
Firefox 35
Tracking Status
firefox32 --- unaffected
firefox33 --- wontfix
firefox34 --- verified
firefox35 --- verified

People

(Reporter: gfritzsche, Assigned: benjamin, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [openh264-uplift] [lang=js])

Attachments

(2 files, 1 obsolete file)

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
Flags: firefox-backlog+
Should we plan to uplift this to Fx33?
Blocks: OpenH264
Flags: needinfo?(georg.fritzsche)
Yes, I think we should get this into Fx33, per the dev.media discussion.
Flags: needinfo?(georg.fritzsche)
Whiteboard: [openh264-uplift]
QA Whiteboard: [qa?]
QA Whiteboard: [qa?]
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
Gavin, can we get this in the next iteration?
Flags: needinfo?(gavin.sharp)
Mentor: georg.fritzsche
Whiteboard: [openh264-uplift] → [openh264-uplift] [lang=js]
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
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?
Flags: needinfo?(gavin.sharp)
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Iteration: --- → 35.1
Attached patch bug1053745-gmp-fhr (obsolete) — Splinter Review
Boy this is a lot of boilerplate...
Attachment #8483894 - Flags: review?(georg.fritzsche)
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.
Attachment #8483894 - Flags: review?(georg.fritzsche)
Attachment #8483894 - Attachment is obsolete: true
Attachment #8484492 - Flags: review?(georg.fritzsche)
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).
Attachment #8484492 - Flags: review?(georg.fritzsche) → review+
https://hg.mozilla.org/integration/fx-team/rev/25c524c5af2f
Target Milestone: --- → Firefox 35
https://hg.mozilla.org/mozilla-central/rev/25c524c5af2f
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Attachment #8484492 - Flags: approval-mozilla-beta?
Attachment #8484492 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(jjensen)
Flags: needinfo?(benjamin)
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.
Flags: needinfo?(benjamin)
Comment on attachment 8484492 [details] [diff] [review]
bug1053745-gmp-fhr, rev. 2

Thanks for clarifying Benjamin. Approved for Beta and Aurora.
Attachment #8484492 - Flags: approval-mozilla-beta?
Attachment #8484492 - Flags: approval-mozilla-beta+
Attachment #8484492 - Flags: approval-mozilla-aurora?
Attachment #8484492 - Flags: approval-mozilla-aurora+
QA Contact: kamiljoz
I totally missed that this didn't update services/healthreport/docs/dataformat.rst - can we do this in a follow-up?
Flags: needinfo?(benjamin)
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?
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.
Richard, do you know what we can call to trigger FHR recollecting from all providers for manual QA?
Flags: needinfo?(rnewman)
I think the needinfo for jjensen was cleared up by comment 14.
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.
Flags: needinfo?(rnewman)
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".
Yeah ok, this is broken. I don't know why yet, since both the automated tests and my local tests worked :-(
Status: RESOLVED → REOPENED
Flags: needinfo?(benjamin)
Resolution: FIXED → ---
Flags: needinfo?(jjensen)
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 :-(
Flags: needinfo?(benjamin)
Marco no, this is just broken so it doesn't make sense to use a separate bug.
Flags: needinfo?(benjamin)
Iteration: 35.1 → 35.2
Iteration: 35.2 → ---
Attachment #8501754 - Flags: review?(georg.fritzsche)
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.
Attachment #8501754 - Flags: review?(georg.fritzsche) → review+
https://hg.mozilla.org/mozilla-central/rev/51d3f2b58ac1
https://hg.mozilla.org/mozilla-central/rev/0cb0d9df5a8e
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Iteration: --- → 35.3
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 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
Attachment #8501754 - Flags: approval-mozilla-aurora?
Comment on attachment 8501754 [details] [diff] [review]
1053745-openh264-fhr-fixup

Aurora+
Attachment #8501754 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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".
Status: RESOLVED → VERIFIED
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.