1137961 - [EME] Update of GMP doesn't happen soon after app update

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

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

Several people have complained that flipping the pref to enable automatic downloading of the Adobe EME GMP takes too long to start and too long to finish. 

Why does the download not start sooner and take so long? At the least we need a way for users to initiate a CDM download, and get feedback on its progress.

Comment 2

[Stephen A Pohl [:spohl]]

The instructions to turn on the downloader may not have been clear, but I don't believe this is a valid bug. Here's how to enable the download of Adobe EME:

1. Set "media.gmp-eme-adobe.hidden" to false in about:config.
2. Reset "media.gmp-manager.lastcheck" (should read '0').
3. Restart the browser promptly after step 2.

You should see that Adobe EME is downloaded after roughly one minute after browser restart.

Note that the "hidden" pref is only used until we officially land Adobe EME in one of our releases and is not exposed via any UI, i.e. an end-user will never run into this issue.

CC-ing Syd and Maja to make sure that they have the proper instructions as well.

Comment 3

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

We talked about this in the EME Standup. We're still concerned about this issue. Consider this case:

1. User starts Firefox.
2. Firefox checks for GMP updates, records its update time.
3. Firefox updates, pulls down the update that enables EME and Adobe CDM.
4. User goes to their favourite DRM streaming site that uses Adobe CDM, doesn't work or they get an error (or a drop down bar in their UI) as the CDM won't download for another 24 hours.

So I think we can make this problem much less bad by simply resetting the media.gmp-manager.lastcheck when Firefox applies an update. This should be a simple fix that we should put into our first release.

Comment 4

[Stephen A Pohl [:spohl]]

Okay, much narrower use case affecting significantly fewer users. But yes, this is a valid scenario that could be resolved by resetting the media.gmp-manager.lastcheck pref.

Comment 5

[Stephen A Pohl [:spohl]]

I thought about this some more and I think the better thing to do here is to check during Firefox startup if any of the expected & enabled GMPs aren't installed yet. In this case, we should just reset media.gmp-manager.lastcheck unconditionally, which will lead to a check 1 minute after startup and install any available GMPs. If there are no objections, I'll go ahead and get started on a patch for this.

Comment 6

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

I think that'll do fine.

Comment 7

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

Actually, I'm not so sure.

Consider this case:

1. User starts Firefox.
2. Firefox checks for GMP updates, say CDM is version X, Firefox records update time.
3. Firefox updates from version N to N+1, pulls down an update that changes the binary ABI for GMPs. 

Now Firefox has a CDM that's for an incompatible ABI, and assuming AUS's update.xml has an entry for Firefox N+1 -> CDM version X+1, we it won't be able to get a the required CDM version X+1 until the next GMP update happens in 24 hours.

So I do think we need to reset media.gmp-manager.lastcheck on update due to this case. Sorry, should have thought of it earlier.

Comment 8

[Stephen A Pohl [:spohl]]

I don't think this is an issue, but I'm not 100% sure. See questions below:

(In reply to Chris Pearce (:cpearce) from comment #7)
> 3. Firefox updates from version N to N+1, pulls down an update that changes
> the binary ABI for GMPs. 

Can you give some more detail here? When Firefox updates, it will have to be restarted to apply the update. It will then check whether or not all known, expected GMPs are installed (these are embedded in the updated Firefox, not the XML). What does ABI mean here, and in what context would that be an issue?

> Now Firefox has a CDM that's for an incompatible ABI, and assuming AUS's
> update.xml has an entry for Firefox N+1 -> CDM version X+1, we it won't be
> able to get a the required CDM version X+1 until the next GMP update happens
> in 24 hours.
> 
> So I do think we need to reset media.gmp-manager.lastcheck on update due to
> this case. Sorry, should have thought of it earlier.

I don't see how resetting media.gmp-manager.lastcheck during an update (I'm assuming you're referring to the installer doing this) would result in different behavior than to reset lastcheck, or skip the lastcheck check, during startup after an update has been applied. Could you explain?

Comment 9

[Stephen A Pohl [:spohl]]

Ah, I think I see what you're saying. I'm assuming that what you're referring to is that although Firefox N+1 may have a GMP installed, the update.xml for Firefox N+1 may have a different GMP version than the update.xml for Firefox N. Is that correct?

Different question then: Would there be a huge amount of pushback if we simply always checked for GMP updates on Firefox startup, and then every 24 hours as long as the browser is running?

Comment 10

[Benjamin Smedberg]

I think we're solving the wrong problem here. In the original spec for GMP installs, was that when we downloaded a new app, we would check for matching new GMPs and download them before we applied the update.

I thought we had a bug for this, but I can't find it.

We shouldn't check for GMPs on every launch, just as we don't check for app update on every launch to avoid killing AUS. Instead we should either/both:

A. Download the new GMP before the update happens and use it immediately on update
B. Check for GMP updates immediately on launch only after an update has just happened.

A is preferable but is more work. B sounds like a pretty quick fix.

Comment 11

[Stephen A Pohl [:spohl]]

Makes sense. Thanks, Benjamin.

Robert, do we already reset prefs during updates? Could we add media.gmp-manager.lastcheck to the list of prefs to reset? If not, do you have a preferred place for me to add this?

Comment 12

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

(In reply to Stephen Pohl [:spohl] (ooo rest of day 3/3/2015) from comment #9)
> Ah, I think I see what you're saying. I'm assuming that what you're
> referring to is that although Firefox N+1 may have a GMP installed, the
> update.xml for Firefox N+1 may have a different GMP version than the
> update.xml for Firefox N. Is that correct?

Yes, that's correct.


> Different question then: Would there be a huge amount of pushback if we
> simply always checked for GMP updates on Firefox startup, and then every 24
> hours as long as the browser is running?

I agree with bsmedberg that we don't want to always check on startup, as that would kill AUS, just check after Firefox update.

Comment 13

[Robert Strong (they/them - no direct email)]

You should add this to nsBrowserContentHandler.js

You can also detect a version change when an install occurs in this code to handle the non app update case. You might want to consider just performing the check instead of clearing the pref.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The goal here is to eliminate the situation where the video won't play without user intervention, such as requiring a restart. We check at start up once a day to minimise the amount of times a user has to sit and wait for the CDM to download.

Consider the situation where someone gives up on another browser in disgust. Switching to Firefox and pasting in the link needs to work seamlessly without any silly messages or user intervention. This should be the end-game and I'm not saying everything needs to be done in v1.

Comment 15

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

We'll needs this for Firefox 37 on Beta and later.

Comment 16

[Stephen A Pohl [:spohl]]

Attachment: Patch

Robert, is this what you had in mind? I could add this to the switch statement just below if desired. I kept this separate because the switch statement is regarding overridePage and I'm writing fewer lines of code, but I'm not set on this.

Gavin, would you like me to request an additional review from someone on your team for this?

Comment 17

[Robert Strong (they/them - no direct email)]

Comment on attachment 8573638 [details] [diff] [review]
Patch

>From: Stephen Pohl <[e-mail]>
>
>Bug 1137961: Check for new GMPs as soon as possible after an update to Firefox. r=rstrong
>
>diff --git a/browser/components/nsBrowserContentHandler.js b/browser/components/nsBrowserContentHandler.js
>--- a/browser/components/nsBrowserContentHandler.js
>+++ b/browser/components/nsBrowserContentHandler.js
>@@ -488,16 +488,22 @@ nsBrowserContentHandler.prototype = {
>       // URL if we do end up showing an overridePage. This makes it possible
>       // to have the overridePage's content vary depending on the version we're
>       // upgrading from.
>       let old_mstone = "unknown";
>       try {
>         old_mstone = Services.prefs.getCharPref("browser.startup.homepage_override.mstone");
>       } catch (ex) {}
>       let override = needHomepageOverride(prefb);
>+      if (override == OVERRIDE_NEW_MSTONE ||
>+          override == OVERRIDE_NEW_BUILD_ID) {
>+        // When an update occurred we should check for new GMPs as soon as
>+        // possible.
This applies to version changes and not just updates...  the comment should reflect that. How about:
Clearing the following preference when the application version changes will cause a check for GMPs as soon as possible.

Extra points for making clear what "as soon as possible" is.

>+        Services.prefs.clearUserPref("media.gmp-manager.lastCheck");
>+      }
>       if (override != OVERRIDE_NONE) {
>         switch (override) {
>           case OVERRIDE_NEW_PROFILE:
>             // New profile.
>             overridePage = Services.urlFormatter.formatURLPref("startup.homepage_welcome_url");
>             break;
>           case OVERRIDE_NEW_MSTONE:
>             // Check whether we will restore a session. If we will, we assume

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8573638 [details] [diff] [review]
Patch

Ideally this code would live in GMP-related code (GMPInstallManager.jsm or GMPProvider.jsm?) and use its own pref to track build IDs/mstones, rather than just piggy backing on the existing mstone stuff in nsBrowserContentHandler. Can you make that change?

Comment 19

[Stephen A Pohl [:spohl]]

Attachment: Patch

Like so?

Comment 20

[Stephen A Pohl [:spohl]]

Comment on attachment 8573676 [details] [diff] [review]
Patch

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

If this is the right approach I'll upload a new patch with the following addressed:

::: toolkit/modules/GMPInstallManager.jsm
@@ +122,5 @@
>    KEY_UPDATE_SECONDS_BETWEEN_CHECKS: "media.gmp-manager.secondsBetweenChecks",
>    KEY_APP_DISTRIBUTION: "distribution.id",
>    KEY_APP_DISTRIBUTION_VERSION: "distribution.version",
> +  KEY_MSTONE: "media.gmp-manager.mstone",
> +  KEY_BUILDID: "media.gmp-manager.buildid",

Will change this to "buildID".

@@ +428,5 @@
>      let now = Math.round(Date.now() / 1000);
>      GMPPrefs.set(GMPPrefs.KEY_UPDATE_LAST_CHECK, now);
>    },
> +  _versionchangeOccurred: function() {
> +    let savedmstone = GMPPrefs.get("media.gmp-manager.mstone", null);

Will change this to KEY_MSTONE.

Comment 21

[Stephen A Pohl [:spohl]]

Regardless of the actual implementation I would like to have QA on this.

Comment 22

[:Gijs (he/him)]

Why the FirefoxProfileMigrator change? This is used in the case of Firefox Refresh/Reset. I don't think we need to copy over the mstone information at that point (in fact, it would make sense to me that because of the reset (basically, new profile with bare necessities copied over from the old profile), we want to check for a new update ASAP).

Comment 23

[Stephen A Pohl [:spohl]]

(In reply to :Gijs Kruitbosch from comment #22)
> Why the FirefoxProfileMigrator change? This is used in the case of Firefox
> Refresh/Reset. I don't think we need to copy over the mstone information at
> that point (in fact, it would make sense to me that because of the reset
> (basically, new profile with bare necessities copied over from the old
> profile), we want to check for a new update ASAP).

From what I could tell, this is where the mstone and buildID pref were being set (see the lines just above my change). Hence, I inferred that we must be using FirefoxProfileMigrator during updates. nsBrowserContentHandler.js will reset the existing mstone and buildID pref immediately after startup, so once we ran a GMP check (60 seconds later) we would have been unable to tell based on these prefs that there was an update and fail to reset the media.gmp-manager.lastCheck pref. We could then be in a situation where we wait up to 24 hours before doing another GMP check. The change to FirefoxProfileMigrator was essentially to duplicate the mstone and buildID prefs for GMP purposes.

Comment 24

[Stephen A Pohl [:spohl]]

Attachment: Patch

Comment 25

[Stephen A Pohl [:spohl]]

(In reply to Stephen Pohl [:spohl] from comment #23)
> nsBrowserContentHandler.js will
> reset the existing mstone and buildID pref immediately after startup, so
> once we ran a GMP check (60 seconds later) we would have been unable to tell
> based on these prefs that there was an update and fail to reset the
> media.gmp-manager.lastCheck pref.

I meant: "...and fail to perform a GMP check immediately regardless of the value of the media.gmp-manager.lastCheck pref (see the changes to simpleCheckAndInstall() in GMPInstallManager.jsm)".

Comment 26

[:Gijs (he/him)]

(In reply to Stephen Pohl [:spohl] from comment #23)
> (In reply to :Gijs Kruitbosch from comment #22)
> > Why the FirefoxProfileMigrator change? This is used in the case of Firefox
> > Refresh/Reset. I don't think we need to copy over the mstone information at
> > that point (in fact, it would make sense to me that because of the reset
> > (basically, new profile with bare necessities copied over from the old
> > profile), we want to check for a new update ASAP).
> 
> From what I could tell, this is where the mstone and buildID pref were being
> set (see the lines just above my change). Hence, I inferred that we must be
> using FirefoxProfileMigrator during updates.

We don't, we only use it for Firefox reset. The reason this code sets the mstone prefs as well is to avoid the "what's new" page popping up, because the new profile won't have any mstone pref set at all (without this code), and it's odd to get a "hey, you just updated, here's what's new in this version of Firefox" page when all you've done is resetting your profile.

I don't think that file needs touching for this patch.

The upgrade path sets the mstone things here:

http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js?mark=120-136#92

AFAICT.

Comment 27

[Stephen A Pohl [:spohl]]

Hmm, so are you saying we should set the media.gmp-manager.buildID and media.gmp-manager.mstone prefs to savedBuildID and savedmstone in nsBrowserContentHandler.js before we change the browser.startup.homepage_override.buildID and browser.startup.homepage_override.mstone prefs to the actual values of the current browser? That should work, just wanted to confirm first.

Comment 28

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8574026 [details] [diff] [review]
Patch

>diff --git a/browser/components/migration/FirefoxProfileMigrator.js b/browser/components/migration/FirefoxProfileMigrator.js

>           // Reset the homepage_override prefs so that the browser doesn't override our
>           // session with the "what's new" page:
>           Services.prefs.setCharPref("browser.startup.homepage_override.mstone", mstone);
>           Services.prefs.setCharPref("browser.startup.homepage_override.buildID", buildID);
>+          // We need to track mstone and buildID for GMPs separately to allow
>+          // for immediate GMP checks after version changes.
>+          Services.prefs.setCharPref("media.gmp-manager.mstone", mstone);
>+          Services.prefs.setCharPref("media.gmp-manager.buildID", buildID);

This code is effectively only used for the "Firefox Refresh" feature. The homepage_override prefs are being set here because the affect whether a "whatsnew" or "firstrun" web page appears on startup. We don't want to have those appear after a refresh.

For GMP, it's not a big deal if refreshing Firefox triggers an immediate GMP check (and it might actually be a good thing, as Gijs says), so I would just omit this change.

>diff --git a/toolkit/modules/GMPInstallManager.jsm b/toolkit/modules/GMPInstallManager.jsm

>+  KEY_MSTONE: "media.gmp-manager.mstone",
>+  KEY_BUILDID: "media.gmp-manager.buildID",

The buildID changes more frequently than the milestone in all cases, so it's redundant to watch both.

Release: buildID/milestone effectively always change at the same time (after update)
Beta/Aurora/Nightly: milestone changes once every 6 weeks, buildID changes for each update (per the channel's update frequency)

>+  _versionchangeOccurred: function() {

Assuming you watch only buildID, then you should be able to simplify this significantly. Along the lines of:

let savedBuildID = GMPPrefs.get(GMPPrefs.KEY_BUILDID, null);
let buildID = Services.appinfo.platformBuildID;
if (savedBuildID == buildID) {
  return false;
}
GMPPrefs.set(GMPPrefs.KEY_BUILDID, buildID);
return true;

Note that this has different behavior than your patch the first time this is run (assume a version change occurred), but I think that's fine.

Are we confident that the GMP update server will actually be able to deal with the volume of requests this will cause? Every Firefox client getting an update will ping the server immediately - that seems a bit risky.

Comment 29

[:Gavin Sharp [email: [e-mail]]]

Sorry, guess I missed some comments - Gijs already covered what I said.

(In reply to Stephen Pohl [:spohl] from comment #27)
> Hmm, so are you saying we should set the media.gmp-manager.buildID and
> media.gmp-manager.mstone prefs to savedBuildID and savedmstone in
> nsBrowserContentHandler.js before we change the
> browser.startup.homepage_override.buildID and
> browser.startup.homepage_override.mstone prefs to the actual values of the
> current browser? That should work, just wanted to confirm first.

No - the two code paths are not related at all. GMP can just do its own thing and ignore the homepage_override/nsBrowserContentHandler logic.

Comment 30

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks, Gavin!

(In reply to :Gavin Sharp [email: [e-mail]] from comment #28)
> >diff --git a/toolkit/modules/GMPInstallManager.jsm b/toolkit/modules/GMPInstallManager.jsm
> 
> Note that this has different behavior than your patch the first time this is
> run (assume a version change occurred), but I think that's fine.

Agreed, this is fine. On first run, media.gmp-manager.lastCheck will not be present and we'd be doing an immediate GMP check anyway.

> Are we confident that the GMP update server will actually be able to deal
> with the volume of requests this will cause? Every Firefox client getting an
> update will ping the server immediately - that seems a bit risky.

I don't know the answer to "can we handle that", but note that this code will only run after an update has been *applied*. So, although many users may be downloading the update at the same time, we shouldn't see a flood of simultaneous GMP checks since people apply the update at different times.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8574054 [details] [diff] [review]
Patch

I guess this patch assumes that simpleCheckAndInstall() is guaranteed to be called on every startup, and I haven't verified that this is the case. Assuming you have, r=me.

(aside: I really hate the GMPPrefs.KEY_* constant mapping - seems like unnecessary obfuscation)

Comment 32

[Stephen A Pohl [:spohl]]

Thanks, Gavin! I appreciate it.

(In reply to :Gavin Sharp [email: [e-mail]] from comment #31)
> Comment on attachment 8574054 [details] [diff] [review]
> Patch
> 
> I guess this patch assumes that simpleCheckAndInstall() is guaranteed to be
> called on every startup, and I haven't verified that this is the case.
> Assuming you have, r=me.

Yes, this is guaranteed[1]. It will run a first time 60 seconds after startup.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1371

Comment 33

[Stephen A Pohl [:spohl]]

Comment on attachment 8574054 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Adobe EME
[User impact if declined]: Without this patch, once a user receives an update that turns on Adobe EME, he may be forced to wait up to 24 hours before we actually download Adobe EME. This contradicts the messaging in the addons manager which says that the CDM "will be installed shortly".
[Describe test coverage new/current, TreeHerder]: Local testing only.
[Risks and why]: The risk should be minimal. The worst I can imagine is for the patch to not work, which would result in the same behavior as described under [User impact if declined].
[String/UUID change made/needed]: none

Comment 34

[Stephen A Pohl [:spohl]]

inbound and fx-team are closed at the moment.

Comment 35

[Carsten Book [:Tomcat]]

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

Comment 36

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

Comment on attachment 8574054 [details] [diff] [review]
Patch

EME will remain disabled in 37. Beta-

Comment 37

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

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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4efbd4921753

Comment 39

[Ada [:adalucinet]]

Stephen, could you please give some instructions on how to reliably verify this fix?
Although, for now, I verified that:
1. Launching a profile with an already installed CDM plugin version < 8 and updating Firefox (Nightly and 38.0 beta), also generates a CDM update in a few seconds.
2. Installing CDM version 3 (via a local server) and updating Nightly and 38.0 beta, also generates a CDM update (to version 8) in a few seconds.

Comment 40

[Stephen A Pohl [:spohl]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #39)
> Stephen, could you please give some instructions on how to reliably verify
> this fix?
> Although, for now, I verified that:
> 1. Launching a profile with an already installed CDM plugin version < 8 and
> updating Firefox (Nightly and 38.0 beta), also generates a CDM update in a
> few seconds.
> 2. Installing CDM version 3 (via a local server) and updating Nightly and
> 38.0 beta, also generates a CDM update (to version 8) in a few seconds.

These steps seem perfect. Just want to clarify a few things to make sure we have the same understanding of these steps:

1. Launch an old build of Firefox with a new profile.
2. Let the old build of Firefox install an old CDM (verify via about:addons). This will set the lastCheck pref, which means that Firefox would usually check for an update again after 24 hours (unless Firefox is updated, which is what we're discussing here).
3. Update the CDM on the server.
4. Update Firefox, restart Firefox to apply update.
5. Ensure that the new CDM from step 3 is applied after ~60 seconds.

Thanks, Alexandra!

Comment 41

[Ada [:adalucinet]]

Yes, indeed! Verified as fixed with updates to latest Nightly 40.0a1 (2015-04-22) and 38 beta 6 (Build ID: 20150420134330) on Vista x32, Windows 7 x64 and Windows 8.1 x64 - the new CMD version (8) is properly applied. Marking accordingly.

Comment 42

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Removing qe-flag as verification on Beta 38 should suffice.