Closed Bug 1156566 Opened 5 years ago Closed 5 years ago

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

Categories

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

x86
Windows Vista
defect

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox37 --- unaffected
firefox38 --- wontfix
firefox39 + fixed
firefox38.0.5 --- wontfix
firefox40 + fixed
firefox41 + fixed

People

(Reporter: cpeterson, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Firefox should download a new CDM, but it doesn't need to block EME MVP.
Priority: -- → P2
@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
Assignee: nobody → cpearce
Summary: Firefox will not redownload CDM if user deletes or renames the CDM files → Retrigger CDM download and report telemetry if user (or anti-virus software?) deletes or renames the CDM files
@Dolske: cpearce will write the Gecko parts. Can you write the Chrome and GMP service parts described in comment 2 (for Beta 39)?
Flags: needinfo?(dolske)
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.
Flags: needinfo?(dolske)
* 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.
Attachment #8604453 - Flags: review?(spohl.mozilla.bugs)
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.
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
Attachment #8604471 - Flags: review?(spohl.mozilla.bugs)
Attachment #8604471 - Flags: review?(benjamin)
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
Attachment #8604453 - Flags: review?(spohl.mozilla.bugs) → review+
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": ["cpearce@mozilla.com"],
> +    "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": ["cpearce@mozilla.com", "rjesup@mozilla.com"],
> +    "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
Attachment #8604471 - Flags: review?(spohl.mozilla.bugs) → review+
Tracking for 39+ since that seems to be where we're aiming this.
(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 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?
Attachment #8604471 - Flags: review?(vdjeric)
Attachment #8604471 - Flags: review?(ally)
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
Attachment #8604471 - Flags: review?(vdjeric)
Attachment #8604471 - Flags: review+
Attachment #8604471 - Flags: review?(ally)
Blocks: 1164738
(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.
Flags: needinfo?(vdjeric)
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
Flags: needinfo?(vdjeric)
Ok, I will make those changes. Thanks Vladan!
Attachment #8604471 - Flags: review?(benjamin)
Must remember to uplift.
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/1b72193f6677
https://hg.mozilla.org/mozilla-central/rev/f52c27b87a31
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
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.
Attachment #8604453 - Flags: approval-mozilla-beta?
Attachment #8604453 - Flags: approval-mozilla-aurora?
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.
Attachment #8604471 - Flags: approval-mozilla-beta?
Attachment #8604471 - Flags: approval-mozilla-aurora?
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).
Attachment #8604453 - Flags: approval-mozilla-beta?
Attachment #8604453 - Flags: approval-mozilla-beta+
Attachment #8604453 - Flags: approval-mozilla-aurora?
Attachment #8604453 - Flags: approval-mozilla-aurora+
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).
Attachment #8604471 - Flags: approval-mozilla-beta?
Attachment #8604471 - Flags: approval-mozilla-beta+
Attachment #8604471 - Flags: approval-mozilla-aurora?
Attachment #8604471 - Flags: approval-mozilla-aurora+
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.
This can still make it into 39 beta 1 build 2 if we can land it in beta tomorrow or early Friday.
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.
Blocks: 1159673
You need to log in before you can comment on or make changes to this bug.