1192924 - Find and install new sets of system add-ons

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Periodically check for a new known-good set of system add-ons, download any new add-ons to the system add-on location. Once all of the add-ons are available switch to the new known-good set.

Comment 1

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

Bug 1192924: Expose the update URL formatting code in the app update service and switch the GMP install manager to use it. r?rstrong

The GMP manager uses a copy of the update service's url formatting code and has
since fallen out of sync. We'll also want to use the same formatting code for
the system add-on update checks so this just exposes it in the update service
component where anyone can call it.

Comment 2

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r?spohl

The system add-on update checks will use the same update.xml format as GMP so
this splits out the code for parsing and downloading files into a standalone
module that both can reuse.

Comment 3

[Dave Townsend [:mossop]]

Attachment: MozReview Request: Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

Bug 1192924: Check for updated system add-ons and download and install them. r?rhelmer

This performs the update check for system add-ons. It runs as part of the daily
add-on update checks similar to hotfix checks. Currently no URL is set so builds
won't actually start checking yet.

I've taken a few shortcuts here by only staging updates and needing a restart to
install as well as always downloading updates rather than using existing local
copies. At least the latter probably needs fixing before turning this on but
it makes more sense to iterate on those in tree.

Comment 4

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

Dave, I think this would be cleaner to just move into UpdateChannel.jsm. Perhaps rename the jsm as well. Make sense?

Comment 5

[Dave Townsend [:mossop]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #4)
> Dave, I think this would be cleaner to just move into UpdateChannel.jsm.
> Perhaps rename the jsm as well. Make sense?

I think so but the issue I ran into was that we use the gOSVersion and gABI values to control whether updates are enabled and to send some telemetry. I could expose those properties from the JSM as well if that makes sense?

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8660998 [details]
MozReview Request: Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

https://reviewboard.mozilla.org/r/19243/#review17263

::: testing/xpcshell/head.js:1508
(Diff revision 1)
> +    prefs.setCharPref("extensions.systemAddon.update.url", "http://%(server)s/dummy-system-addons.xml");

Why http not https? Does this whole pref get overridden anyway?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2850
(Diff revision 1)
> +            logger.warn(`Failed to remove temporary file ${item.path}.`);

Any reason not to log the exception too?

Comment 7

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

(In reply to Dave Townsend [:mossop] from comment #5)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #4)
> > Dave, I think this would be cleaner to just move into UpdateChannel.jsm.
> > Perhaps rename the jsm as well. Make sense?
> 
> I think so but the issue I ran into was that we use the gOSVersion and gABI
> values to control whether updates are enabled and to send some telemetry. I
> could expose those properties from the JSM as well if that makes sense?
I think it makes sense to do that. It would be a good thing to make the blocklist use it as well if possible.

Comment 8

[Robert Helmer [:rhelmer]]

Comment on attachment 8660998 [details]
MozReview Request: Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

https://reviewboard.mozilla.org/r/19243/#review17435

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 8660996 [details]
MozReview Request: Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

The GMP manager uses a copy of the update service's url formatting code and has
since fallen out of sync. We'll also want to use the same formatting code for
the system add-on update checks so this just exposes it in a shared API.

I've moved the contents of UpdateChannel.jsm to UpdateUtils.jsm and exposed
formatUpdateURL there as well as a few properties that the update service still
needs access to.

UpdateUtils.UpdateChannel is intended to be a lazy getter but isn't for now
since tests expect to be able to change the update channel at runtime.

Comment 10

[Dave Townsend [:mossop]]

Comment on attachment 8660997 [details]
MozReview Request: Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r?spohl

The system add-on update checks will use the same update.xml format as GMP so
this splits out the code for parsing and downloading files into a standalone
module that both can reuse.

Comment 11

[Dave Townsend [:mossop]]

Comment on attachment 8660998 [details]
MozReview Request: Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

This performs the update check for system add-ons. It runs as part of the daily
add-on update checks similar to hotfix checks. Currently no URL is set so builds
won't actually start checking yet.

I've taken a few shortcuts here by only staging updates and needing a restart to
install as well as always downloading updates rather than using existing local
copies. At least the latter probably needs fixing before turning this on but
it makes more sense to iterate on those in tree.

Comment 12

[Stephen A Pohl [:spohl]]

Comment on attachment 8660997 [details]
MozReview Request: Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

https://reviewboard.mozilla.org/r/19241/#review17851

Sorry about the delay! A lot of code that moved, with some dramatic code improvements that I had to wrap my head around first. :-) Only a few minor nits and questions. Thanks!

::: toolkit/modules/GMPInstallManager.jsm:102
(Diff revision 2)
> -      this._request = this._request.wrappedJSObject;
> +      certs = gCertUtils.readCertPrefs(GMPPrefs.KEY_CERTS_BRANCH);

|certs| is now set regardless of whether or not |GMPPrefs.get(GMPPrefs.KEY_CERT_CHECKATTRS, true)| returns true. Is this intentional? If yes, let's remove the pref from GMPPrefs and avoid setting it in test_GMPInstallManager.js.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:37
(Diff revision 2)
> + * Gets the status of an XMLHttpRequest either directory or from its underlying

nit: s/directory/directly/

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:56
(Diff revision 2)
> +  return request.channel.QueryInterface(Ci.nsIRequest).status;

This is existing behavior, but wanted to double-check: |request.channel| is guaranteed to be created here (i.e. != null), correct?

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:150
(Diff revision 2)
> +  let addons = document.querySelector("updates:root > addons:last-child");

I realize that |addons:last-child| is existing behavior, but is there any concern about the possibility of multiple "addons" nodes? We're currently selecting the last one regardless of how many are in the document. I get the impression that the original implementation probably meant to add all the addon elements contained in "addons" nodes and accidentally used |gmpResults = GMPAddon.parseGMPAddonsNode(updatesChildElement);| instead of |gmpResults += GMPAddon.parseGMPAddonsNode(updatesChildElement);| (or similar) in parseResponseXML().

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:151
(Diff revision 2)
> +  if (!addons)

nit: please use curly braces around return statement.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:205
(Diff revision 2)
> +                       DOWNLOAD_INTERVAL);

nit: fix indentation or move to previous line

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:218
(Diff revision 2)
> +      hex = "0" + hex;

nit: Let's add curly braces around this too, even though it was just a copy/paste.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm:231
(Diff revision 2)
> + * @return a promise that resolves to hash of the file or rejects with a JS

nit: let's add "the" before "hash" in the comment for @return

::: toolkit/mozapps/extensions/test/xpcshell/test_ProductAddonChecker.js:46
(Diff revision 2)
> +  // Check the returned size versus the expected size.

nit: This (previously existing) comment does not seem to apply anymore. Let's remove it.

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8660996 [details]
MozReview Request: Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

The GMP manager uses a copy of the update service's url formatting code and has
since fallen out of sync. We'll also want to use the same formatting code for
the system add-on update checks so this just exposes it in a shared API.

I've moved the contents of UpdateChannel.jsm to UpdateUtils.jsm and exposed
formatUpdateURL there as well as a few properties that the update service still
needs access to.

UpdateUtils.UpdateChannel is intended to be a lazy getter but isn't for now
since tests expect to be able to change the update channel at runtime.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 8660997 [details]
MozReview Request: Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

The system add-on update checks will use the same update.xml format as GMP so
this splits out the code for parsing and downloading files into a standalone
module that both can reuse.

Comment 15

[Dave Townsend [:mossop]]

Comment on attachment 8660998 [details]
MozReview Request: Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

This performs the update check for system add-ons. It runs as part of the daily
add-on update checks similar to hotfix checks. Currently no URL is set so builds
won't actually start checking yet.

I've taken a few shortcuts here by only staging updates and needing a restart to
install as well as always downloading updates rather than using existing local
copies. At least the latter probably needs fixing before turning this on but
it makes more sense to iterate on those in tree.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/10e1fa2cc23a
https://hg.mozilla.org/integration/fx-team/rev/a6860f880e01
https://hg.mozilla.org/integration/fx-team/rev/30d612836685

Comment 17

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

Backed out for android bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=4777582&repo=fx-team


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

Comment 18

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

Looks like b2g xpcshell is broken, too: https://treeherder.mozilla.org/logviewer.html#?job_id=4779059&repo=fx-team

Comment 19

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

Was comm-central also patched?

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 8660996 [details]
MozReview Request: Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

The GMP manager uses a copy of the update service's url formatting code and has
since fallen out of sync. We'll also want to use the same formatting code for
the system add-on update checks so this just exposes it in a shared API.

I've moved the contents of UpdateChannel.jsm to UpdateUtils.jsm and exposed
formatUpdateURL there as well as a few properties that the update service still
needs access to.

UpdateUtils.UpdateChannel is intended to be a lazy getter but isn't for now
since tests expect to be able to change the update channel at runtime.

Comment 21

[Dave Townsend [:mossop]]

Comment on attachment 8660997 [details]
MozReview Request: Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

Bug 1192924: Split out add-on update.xml parsing code from GMP modules. r=spohl

The system add-on update checks will use the same update.xml format as GMP so
this splits out the code for parsing and downloading files into a standalone
module that both can reuse.

Comment 22

[Dave Townsend [:mossop]]

Comment on attachment 8660998 [details]
MozReview Request: Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

Bug 1192924: Check for updated system add-ons and download and install them. r=rhelmer

This performs the update check for system add-ons. It runs as part of the daily
add-on update checks similar to hotfix checks. Currently no URL is set so builds
won't actually start checking yet.

I've taken a few shortcuts here by only staging updates and needing a restart to
install as well as always downloading updates rather than using existing local
copies. At least the latter probably needs fixing before turning this on but
it makes more sense to iterate on those in tree.

Comment 23

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

Comment on attachment 8660996 [details]
MozReview Request: Bug 1192924: Expose the update URL formatting code a new UpdateUtils module. r?rstrong

Changes look fine. Would be a good thing to send to try.

Comment 24

[Dave Townsend [:mossop]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #23)
> Comment on attachment 8660996 [details]
> MozReview Request: Bug 1192924: Expose the update URL formatting code a new
> UpdateUtils module. r?rstrong
> 
> Changes look fine. Would be a good thing to send to try.

Already there: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12dfb5182fc9

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9e57875f3f6c
https://hg.mozilla.org/integration/fx-team/rev/be03da790173
https://hg.mozilla.org/integration/fx-team/rev/57858515d1ab

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9e57875f3f6c
https://hg.mozilla.org/mozilla-central/rev/be03da790173
https://hg.mozilla.org/mozilla-central/rev/57858515d1ab

Comment 27

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

On 64bit Windows running 32bit Firefox the patches in this bug changed %BUILD_TARGET% to be "WINNT_x86-msvc-x64". Previously it was "WINNT_x86-msvc". Making the build target for a 32bit build include "x64" seems wrong. 

This has broken both OpenH264 and Adobe Primetime GMP updates, as they use the %BUILD_TARGET% as part of their URL to their AUS update.xml file. Curiously this only breaks updates in newly created profiles, not in existing profiles.

Dave: can you fix this regression, otherwise we'll have to back this out as it's breaking Adobe Primetime/Netflix and OpenH264.

STR:
1. Create a new profile on x64 Win 7 or later in 32bit Firefox.
2. Set prefs media.gmp.log.dump=true, media.gmp.log.level=0
3. Open Add-ons manager > Plugins
4. Observe the "Primetime Content Decryption Module ... will be installed shortly" being shown for the Adobe Primetime plugin's entry.
5. Right click on "Primetime Content Decryption Module" > Find updates.
6. Observe that "Primetime Content Decryption Module ... will be installed shortly" never goes away.

If you compare the GMP installer logs in the browser console with an existing profile, you'll see that the %BUILD_TARGET% has an "-x64" appended to it.

Comment 28

[Dave Townsend [:mossop]]

(In reply to Chris Pearce (:cpearce) from comment #27)
> On 64bit Windows running 32bit Firefox the patches in this bug changed
> %BUILD_TARGET% to be "WINNT_x86-msvc-x64". Previously it was
> "WINNT_x86-msvc". Making the build target for a 32bit build include "x64"
> seems wrong. 
> 
> This has broken both OpenH264 and Adobe Primetime GMP updates, as they use
> the %BUILD_TARGET% as part of their URL to their AUS update.xml file.
> Curiously this only breaks updates in newly created profiles, not in
> existing profiles.
> 
> Dave: can you fix this regression, otherwise we'll have to back this out as
> it's breaking Adobe Primetime/Netflix and OpenH264.

*sigh*. Looks like bug 1183077 added the OS architecture for app update BUILD_TARGET but not GMP updates. Stephen, was that intentional? I'd really like it if we could unify all of these place where we generate URLs so we don't have to fix the same bugs in many places. Can we update Balrog's rules to handle this or do we have to make the GMP update URLs unique again?

Comment 29

[[e-mail] (:bhearsum)]

(In reply to Chris Pearce (:cpearce) from comment #27)
> On 64bit Windows running 32bit Firefox the patches in this bug changed
> %BUILD_TARGET% to be "WINNT_x86-msvc-x64". Previously it was
> "WINNT_x86-msvc". Making the build target for a 32bit build include "x64"
> seems wrong. 

I can see why this seems wrong, but it's actually very important: The first arch is the target architecture of the build, the second is what the machine is actually running. This let's us distinguish between 64-bit builds running on 32-bit machines, and 64-bit builds running on 64-bit machines, which will eventually allow us to upgrade users who are on a 64-bit system to 64-bit Firefox, while keeping users with 32-bit systems on 32-bit. The reason this spilled over to GMP is beacuse we share an update URL format between Firefox updates, GMP, and some other things.

> This has broken both OpenH264 and Adobe Primetime GMP updates, as they use
> the %BUILD_TARGET% as part of their URL to their AUS update.xml file.
> Curiously this only breaks updates in newly created profiles, not in
> existing profiles.

This is an easy fix on the update server - we can alias the new build targets to the old ones. My bad for missing this when we changed the build targets. I'll fix this shortly.

Comment 30

[Stephen A Pohl [:spohl]]

(In reply to Dave Townsend [:mossop] from comment #28)
> *sigh*. Looks like bug 1183077 added the OS architecture for app update
> BUILD_TARGET but not GMP updates. Stephen, was that intentional? I'd really
> like it if we could unify all of these place where we generate URLs so we
> don't have to fix the same bugs in many places.

No, that was not intentional and should have been changed for GMP updates as well. I didn't see that attachment 8660996 [details] was touching this. Good to hear that this is an easy fix on the update server side.

Comment 31

[[e-mail] (:bhearsum)]

(In reply to Stephen A Pohl [:spohl] from comment #30)
> (In reply to Dave Townsend [:mossop] from comment #28)
> > *sigh*. Looks like bug 1183077 added the OS architecture for app update
> > BUILD_TARGET but not GMP updates. Stephen, was that intentional? I'd really
> > like it if we could unify all of these place where we generate URLs so we
> > don't have to fix the same bugs in many places.
> 
> No, that was not intentional and should have been changed for GMP updates as
> well. I didn't see that attachment 8660996 [details] was touching this. Good
> to hear that this is an easy fix on the update server side.

Oh sorry - I think I misunderstood the issue. I thought we *were* including arch in these URLs, and the problem was that the update server wasn't returning anything for them. I actually went ahead updated the server such that URLs like https://aus5.mozilla.org/update/3/GMP/38.0a1/20120222174716/WINNT_x86-msvc-x64/en-US/nightly/default/default/default/update.xml return the appropriate plugins.

If the GMP URLs are using different build targets than app update, I think that should be fixed. This is what happens when we fork our own update code :(.

Comment 32

[Dave Townsend [:mossop]]

(In reply to Ben Hearsum (:bhearsum) from comment #31)
> (In reply to Stephen A Pohl [:spohl] from comment #30)
> > (In reply to Dave Townsend [:mossop] from comment #28)
> > > *sigh*. Looks like bug 1183077 added the OS architecture for app update
> > > BUILD_TARGET but not GMP updates. Stephen, was that intentional? I'd really
> > > like it if we could unify all of these place where we generate URLs so we
> > > don't have to fix the same bugs in many places.
> > 
> > No, that was not intentional and should have been changed for GMP updates as
> > well. I didn't see that attachment 8660996 [details] was touching this. Good
> > to hear that this is an easy fix on the update server side.
> 
> Oh sorry - I think I misunderstood the issue. I thought we *were* including
> arch in these URLs, and the problem was that the update server wasn't
> returning anything for them. I actually went ahead updated the server such
> that URLs like
> https://aus5.mozilla.org/update/3/GMP/38.0a1/20120222174716/WINNT_x86-msvc-
> x64/en-US/nightly/default/default/default/update.xml return the appropriate
> plugins.
> 
> If the GMP URLs are using different build targets than app update, I think
> that should be fixed. This is what happens when we fork our own update code
> :(.

They were using different build targets and this bug fixed that. Sounds like we're all good here now then.

Comment 33

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

On Win10 x64 with 32bit Firefox Nightly I'm getting an AUS URL of:

https://aus5.mozilla.org/update/3/GMP/44.0a1/20150925030206/WINNT_x86-msvc-x64/en-US/nightly/Windows_NT%2010.0.0.0%20(x64)/default/default/update.xml

This doesn't have an Adobe Primetime entry. So we're not done yet.

Comment 34

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

https://reviewboard.mozilla.org/r/19239/#review18373

Comment 35

[[e-mail] (:bhearsum)]

(In reply to Chris Pearce (:cpearce) from comment #33)
> On Win10 x64 with 32bit Firefox Nightly I'm getting an AUS URL of:
> 
> https://aus5.mozilla.org/update/3/GMP/44.0a1/20150925030206/WINNT_x86-msvc-
> x64/en-US/nightly/Windows_NT%2010.0.0.0%20(x64)/default/default/update.xml
> 
> This doesn't have an Adobe Primetime entry. So we're not done yet.

Sorry about that - I only added the alias' for OpenH264. Should be fixed now.

Comment 36

[[e-mail] (:bhearsum)]

Removing needinfo because I think the issue with GMP updates is resolved. Please let me know if it isn't.