757287 - Telemetry Data Point Request - Flash Plugin Version

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[Michael Coates [:mcoates] (acct no longer active)]

Outdated plugins are one of the most common ways our users are compromised.  We will be mounting targeted campaigns to educate users and help guide them to update their plugins.

However, we will not be able to determine if these efforts are successful or if the problem is subsiding unless we are able to measure the total count of flash by version number.

To simplify the data gathering it is sufficient to only apply this data point to flash.

Comment 1

[(dormant account)]

Are you going to attach a patch, we do not have bandwidth for this in the next couple of weeks.

Comment 2

[Justin Lebar (not reading bugmail)]

Be sure to collect at least a week of data before mounting any campaigns.  Telemetry data can be noisy.

Comment 3

[Michael Coates [:mcoates] (acct no longer active)]

(In reply to Taras Glek (:taras) from comment #1)
> Are you going to attach a patch, we do not have bandwidth for this in the
> next couple of weeks.

Understood. I'm getting the bug filed and working to get resources to complete the patch.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Michael, do we need that telemetry long-term or will it be only in for a limited time?

Comment 5

[Benjamin Smedberg]

Yes, we should plan on keeping this enabled "forever". If it's not a privacy problem, we probably want to submit all plugin versions, otherwise just Flash.

Comment 6

[Georg Fritzsche [:gfritzsche]]

As a privacy review in the bug might be sufficient, i am adding the requested data.

Life-time of the probe: 
We want this to stay in with no time limit.

Examples of data being collected:
If it is no privacy concern, we want to collect the names and versions of all installed plugins, e.g.:
 * "Shockwave Flash", "11.2.202.235" 
 * "Java(TM) Platform SE 7 U4", "10.4.1.255"
 * "Adobe Acrobat", "9.5.1.283"
If collecting the names and versions of all installed plugins is a privacy problem, we want to collect only the data for the installed Flash plugin, e.g.
 * "flashVersion", "11.2.202.235"

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Add plugin versions to telemetry ping

This is currently blocked by bug 767077 as plugin names may contain special characters.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #6)
> As a privacy review in the bug might be sufficient, i am adding the
> requested data.

Thanks, Georg.  

We should focus just on Flash for now.  Collecting info about all plug-ins has more privacy implications (and is broader than this bug).  See: https://wiki.mozilla.org/Privacy/Reviews/Telemetry/Encountered_Plugin_Types

Your comment 6 the first question, "precisely what data will we collect?"
For completeness, please verify you agree with my answers to these questions:

1. What problem are we trying to solve with this data?  We want to identify whether or not users are more rapidly upgrading off buggy versions of flash.

2. What benefit will a solution provide to our users?  We can focus our engagement efforts on those that work best (show good results in telmetry data) to keep users upgrading to safe versions of flash.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Sid Stamm [:geekboy] from comment #8)
> We should focus just on Flash for now.  Collecting info about all plug-ins
> has more privacy implications (and is broader than this bug).  See:
> https://wiki.mozilla.org/Privacy/Reviews/Telemetry/Encountered_Plugin_Types

Ah, i see - so we limit this bug to Flash.
 
> Your comment 6 the first question, "precisely what data will we collect?"
> For completeness, please verify you agree with my answers to these questions:
> 
> 1. What problem are we trying to solve with this data?  We want to identify
> whether or not users are more rapidly upgrading off buggy versions of flash.
> 
> 2. What benefit will a solution provide to our users?  We can focus our
> engagement efforts on those that work best (show good results in telmetry
> data) to keep users upgrading to safe versions of flash.

Agreed on both of these as per comment 1.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Submit flash version from telemetry ping

Taras, can you review this?

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 637510 [details] [diff] [review]
Submit flash version from telemetry ping

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +346,5 @@
>  
>      if (this._addons)
>        ret.addons = this._addons;
>  
> +    ret.flashVersion = this.getFlashVersion();

I'd make this conditional on actually having Flash, just like we do for addons, persona, etc.

Do we have a good way of providing a fake plugin for testing purposes?  Surely other tests have addressed this problem.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +134,5 @@
>  
>    do_check_eq(payload.info.reason, reason);
>    do_check_true("appUpdateChannel" in payload.info);
>    do_check_true("locale" in payload.info);
> +  do_check_eq(payload.info.flashVersion, getFlashVersion());

Once we make setting flashVersion conditional, I think it will be sufficient to check for the existence of flashVersion and not a specific value (unless it's a test-specific value, not something returned by getFlashVersion).

Comment 12

[Georg Fritzsche [:gfritzsche]]

Attachment: Submit flash version from telemetry ping, v2

Addressed previous review comments.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 638138 [details] [diff] [review]
Submit flash version from telemetry ping, v2

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

Looks good to me, just a couple minor changes.

::: toolkit/components/telemetry/tests/unit/test_TelemetryPing.js
@@ +125,5 @@
>      appVersion: "1", 
>      appName: "XPCShell", 
>      appBuildID: "2007010101",
> +    platformBuildID: "2007010101",
> +    flashVersion: "1.1.1.1"

Please pull "1.1.1.1" out into a const FLASH_VERSION at the top of the file and use FLASH_VERSION in the definition of PluginHost.

@@ +135,5 @@
>  
>    do_check_eq(payload.info.reason, reason);
>    do_check_true("appUpdateChannel" in payload.info);
>    do_check_true("locale" in payload.info);
> +  

Nit: unnecessary whitespace change.

@@ +336,5 @@
> +  },
> +
> +  QueryInterface: function(iid) {
> +    if (iid.equals(Components.interfaces.nsIPluginHost)
> +     || iid.equals(Components.interfaces.nsISupports))

Nit: please use Ci. instead.

@@ +355,5 @@
> +const PLUGINHOST_CONTRACTID = "@mozilla.org/plugin/host;1";
> +const PLUGINHOST_CID = Components.ID("{2329e6ea-1f15-4cbe-9ded-6e98e842de0e}");
> +
> +function registerFakePluginHost() {
> +  var registrar = Components.manager.QueryInterface(Components.interfaces.nsIComponentRegistrar);

Nit: Ci.nsIComponentRegistrar.

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Submit flash version from telemetry ping, v3

Adressed review comments.

Comment 15

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 638425 [details] [diff] [review]
Submit flash version from telemetry ping, v3

Remove failed attempt to carry review+ over.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/936ee90e6e54

Please remember to include all of the necessary commit information in the patches you submit. It makes life much easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/936ee90e6e54