1156566 - Retrigger CDM download and report telemetry if user (or anti-virus software?) deletes or renames the CDM files

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Chris Peterson [:cpeterson]]

John Xu reports that if a user renames or deletes directory for adobe cp modules, Firefox will not download a new module. Playing drm content will show message that it is downloading modules:

Firefox is installing components needed to play the audio or video on this page. Please try again later.

Comment 1

[Chris Peterson [:cpeterson]]

Firefox should download a new CDM, but it doesn't need to block EME MVP.

Comment 2

[Chris Peterson [:cpeterson]]

@Dolske: cpearce will write the Gecko parts. Can you write the Chrome and GMP service parts (for Beta 39)?

* Gecko: on GMP creation, check if CDM exists, remove plugin dir from gmpservice, reset prefs, fire “need to install cdm” message to chrome.

* Chrome: check that GMPs have DLLs, reset the prefs (this means we won’t add the plugin dir to gmpservice), then the CDM should reinstall some time later.
Add telemetry to chrome to report when expected DLL goes missing on startup, and when it does, so we can calculate error rate.

* Add telemetry to report when DLL is correctly installed and on disk.

* Add telemetry to gecko to report when the DLL instantiated

Comment 3

[Chris Peterson [:cpeterson]]

@Dolske: cpearce will write the Gecko parts. Can you write the Chrome and GMP service parts described in comment 2 (for Beta 39)?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

I'll write the Chrome parts too.

We can just add telemetry to report when the Adobe EME DLL is missing on startup of the GMPProvider, we don't need to only do it when an EME session is initiated.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1: Don't add invalid GMP dirs to GMPService

* Check whether GMP dirs contain the GMP DLL/dylib/so and info file before adding to the GMPService in GMPProvider.
* Updated XPCShell test to test that starting up with a GMP dir without lib & info files results in the GMP not being added. And then test that if we create mock plugin lib/info files we do end up adding the GMP.
* I also fixed a bug in the definition of prefs in GMPUtils.jsm.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8604453 [details] [diff] [review]
Patch 1: Don't add invalid GMP dirs to GMPService

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

::: toolkit/modules/GMPUtils.jsm
@@ +88,5 @@
>    KEY_BUILDID:                  "media.gmp-manager.buildID",
>    KEY_CERTS_BRANCH:             "media.gmp-manager.certs.",
>    KEY_PROVIDER_ENABLED:         "media.gmp-provider.enabled",
>    KEY_LOG_BASE:                 "media.gmp.log.",
> +  KEY_LOGGING_LEVEL:            "media.gmp.log.level",

This change fixes the following error during XPCShell tests:

0:02.41 LOG: Thread-1 INFO "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property this.GMPPrefs" {file: "resource://gre/modules/GMPUtils.jsm" line: 120}]"

I think this is the reason why I was having trouble getting GMP browser console logging to work a while back.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Report via telemetry when EME plugin DLL disappears

Add Telementry to report on Firefox startup when the Adobe and OpenH264 GMPs are supposed to be present on disk but are not.

r? spohl for GMPProvider/test changes
r? bsmedberg for telemetry changes

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b6dda44b1d2

Comment 9

[Stephen A Pohl [:spohl]]

Comment on attachment 8604453 [details] [diff] [review]
Patch 1: Don't add invalid GMP dirs to GMPService

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

::: toolkit/modules/GMPUtils.jsm
@@ +88,5 @@
>    KEY_BUILDID:                  "media.gmp-manager.buildID",
>    KEY_CERTS_BRANCH:             "media.gmp-manager.certs.",
>    KEY_PROVIDER_ENABLED:         "media.gmp-provider.enabled",
>    KEY_LOG_BASE:                 "media.gmp.log.",
> +  KEY_LOGGING_LEVEL:            "media.gmp.log.level",

Would dropping the |this.| from |this.KEY_LOG_BASE| fix this too?

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +448,5 @@
>      return this._updateTask;
>    },
> +
> +  _arePluginFilesOnDisk: function () {
> +    let fileExists = function(gmpPath, aFileName, log) {

Remove |log|. Also: s/gmpPath/aGmpPath/

@@ +458,5 @@
> +
> +    // Determine the name of the GMP dynamic library; it differs on every
> +    // platform. Note: we can't use Services.appInfo.OS here, as that's
> +    // "XPCShell" in our tests.
> +    let isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);

nit: over 80 characters

@@ +471,5 @@
> +      libName = "lib" + id.substring(4) + ".dylib";
> +    } else if (isLinux) {
> +      libName = id.substring(4) + ".so";
> +    } else {
> +      this._info.error("_arePluginFilesOnDisk unsupported platform.");

this._log.error("_arePluginFilesOnDisk - unsupported platform.");

@@ +509,5 @@
>                        gmpPath);
>  
>        if (gmpPath && isEnabled) {
> +        if (!wrapper.validate()) {
> +          this._log.info("startup - gmp " + plugin.id + " missing lib and/or info files, uninstalling");

nit: over 80 characters

::: toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js
@@ +8,5 @@
>  
>  XPCOMUtils.defineLazyGetter(this, "pluginsBundle",
>    () => Services.strings.createBundle("chrome://global/locale/plugins.properties"));
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "FileUtils", "resource://gre/modules/FileUtils.jsm");

nit: over 80 characters

@@ +217,5 @@
>      Assert.ok(!gPrefs.prefHasUserValue(autoupdateKey));
>    }
>  });
>  
> +function createMockPluginFilesIfNeeded(file, aPluginId) {

nit: s/file/aFile/

@@ +283,5 @@
>  
> +    // Test that plugin registration fails if the plugin dynamic library and
> +    // info files are not present.
> +    gPrefs.setCharPref(gGetKey(GMPScope.GMPPrefs.KEY_PLUGIN_VERSION, addon.id),
> +                      TEST_VERSION);

nit: one more space for indent

Comment 10

[Stephen A Pohl [:spohl]]

Comment on attachment 8604471 [details] [diff] [review]
Patch 2: Report via telemetry when EME plugin DLL disappears

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

::: toolkit/components/telemetry/Histograms.json
@@ +7523,5 @@
> +    "alert_emails": ["[e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "flag",
> +    "description": "Whether or not the Adobe EME GMP was expected to be resident on disk but mysteriously isn't."
> +  },  

nit: whitespace

@@ +7529,5 @@
> +    "alert_emails": ["[e-mail]", "[e-mail]"],
> +    "expires_in_version": "never",
> +    "kind": "flag",
> +    "description": "Whether or not the OpenH264 GMP was expected to be resident on disk but mysteriously isn't."
> +  },  

nit: whitespace

::: toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js
@@ +277,5 @@
>            removedPaths.push(path);
>          }
>        },
>      };
> +    

nit: whitespace

@@ +279,5 @@
>        },
>      };
> +    
> +    let reportedKeys = [];
> +    

nit: whitespace

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 39+ since that seems to be where we're aiming this.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Stephen Pohl [:spohl] from comment #9)
> Comment on attachment 8604453 [details] [diff] [review]
> Patch 1: Don't add invalid GMP dirs to GMPService
> 
> Review of attachment 8604453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/GMPUtils.jsm
> @@ +88,5 @@
> >    KEY_BUILDID:                  "media.gmp-manager.buildID",
> >    KEY_CERTS_BRANCH:             "media.gmp-manager.certs.",
> >    KEY_PROVIDER_ENABLED:         "media.gmp-provider.enabled",
> >    KEY_LOG_BASE:                 "media.gmp.log.",
> > +  KEY_LOGGING_LEVEL:            "media.gmp.log.level",
> 
> Would dropping the |this.| from |this.KEY_LOG_BASE| fix this too?

No, that doesn't work either.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8604471 [details] [diff] [review]
Patch 2: Report via telemetry when EME plugin DLL disappears

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

Or maybe Ally or Vladan can review/approve adding a Telemetry probe here?

Comment 14

[Vladan Djeric (:vladan)]

Comment on attachment 8604471 [details] [diff] [review]
Patch 2: Report via telemetry when EME plugin DLL disappears

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

Does this need to be an opt-out measurement on release channel?
See "releaseChannelCollection" in https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Declaring_a_Histogram

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #14)
> Comment on attachment 8604471 [details] [diff] [review]
> Patch 2: Report via telemetry when EME plugin DLL disappears
> 
> Review of attachment 8604471 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Does this need to be an opt-out measurement on release channel?
> See "releaseChannelCollection" in
> https://developer.mozilla.org/en-US/docs/Mozilla/Performance/
> Adding_a_new_Telemetry_probe#Declaring_a_Histogram

I think it would be best for it to be opt-out. We think anti-virus is interfering with our EME plugin download and/or install, so collecting data on broad as possible configurations or variants of anti-virus installs is important.

Comment 16

[Vladan Djeric (:vladan)]

Ok, mark it as such in the Histograms.json declaration. With these opt-out histograms, we've been asking authors to set the histogram's expiry_version e.g. 6 months in the future, so that we remember to re-evaluate the usefulness of the probe and the trade-off between the amount of user data collected vs user benefit. We can bump the expiry version or set it to "never" in 6 months

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Ok, I will make those changes. Thanks Vladan!

Comment 18

[Chris Pearce [:cpearce (Not reading bugmail)]]

Must remember to uplift.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b72193f6677
https://hg.mozilla.org/integration/mozilla-inbound/rev/f52c27b87a31

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b72193f6677
https://hg.mozilla.org/mozilla-central/rev/f52c27b87a31

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8604453 [details] [diff] [review]
Patch 1: Don't add invalid GMP dirs to GMPService

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: We believe that anti-virus is deleting Adobe's EME plugin after Firefox downloads it. This patch changes Firefox to check to see if the Adobe EME (and OpenH264) plugins are actually on disk before telling Gecko to use said plugins. Without this patch, users who have anti-virus that is interfering with Firefox's EME plugin install will report to EME sites that they can use EME, but EME will fail. With this patch, we will attempt to re-download the EME plugin, increasing the chance that it'll work.
[Describe test coverage new/current, TreeHerder]: Tests are included to cover this case in the patch.
[Risks and why]: Low; if something goes wrong here, we'll just attempt to re-download the EME plugin, i.e. we'd take an existing code path.
[String/UUID change made/needed]: None.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8604471 [details] [diff] [review]
Patch 2: Report via telemetry when EME plugin DLL disappears

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: We believe that anti-virus is deleting Adobe's EME plugin after Firefox downloads it. This patch changes Firefox to report via opt-out telemetry when this happens, so that we can figure out how widespread the problem is.
[Describe test coverage new/current, TreeHerder]: Tests are included to cover this case in the patch.
[Risks and why]: Low; just adds telemetry
[String/UUID change made/needed]: None.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8604453 [details] [diff] [review]
Patch 1: Don't add invalid GMP dirs to GMPService

Includes test, low-risk patch for telemetry. Approved for uplift to aurora (40) and beta (39).

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8604471 [details] [diff] [review]
Patch 2: Report via telemetry when EME plugin DLL disappears

Includes tests, low-risk patch for telemetry. Approved for uplift to aurora (40) and beta (39).

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/502444490789
https://hg.mozilla.org/releases/mozilla-aurora/rev/cf2860ddc77c

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_gmpProvider.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=837730&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/d00b220688b3

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

JavaScript Error: "1432156808281 addons.manager ERROR Exception calling provider GMPProvider.startup: TypeError: removedPaths.includes is not a function (C:/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_gmpProvider.js:277:13)

Array.prototype.includes is only in Nightly, not Aurora/Beta. Will add a polyfill implementation.

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

This can still make it into 39 beta 1 build 2 if we can land it in beta tomorrow or early Friday.

Comment 29

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a7f3541d133b
https://hg.mozilla.org/releases/mozilla-aurora/rev/dce1bb4fe6d8

Comment 30

[Pulsebot]

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

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

RyanVM pointed out that since Array.includes() is we'll get the same failure when current Nightly merges to Aurora. So I landed the same Array.prototype polyfill fix in Nightly.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4572f6af0d50
https://hg.mozilla.org/releases/mozilla-beta/rev/26f16f564a81

Comment 33

[Carsten Book [:Tomcat]]

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