1009816 - Firefox desktop: openh264 updates: check, download, install

Status: VERIFIED FIXED

(Firefox :: General, enhancement)

Description

[Benjamin Smedberg]

The Firefox client should check for and install versions of the OpenH264 plugin from Cisco on each install. This currently depends on a decision about where the update manifest is going to live (either in the AUS ping or in a separate ping specifically for openh264).

The client should validate the manifest hash when downloading and save the openh264 plugin into a location in the user profile.

This bug does not cover any UX work; this is entirely about the backend.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

We'll basically get this for free from bug 950933 - I've been working on that with the assumption that it should work with any optionally downloadable media-plugin-thingy that can be advertised from Mozilla servers (ie, push rather than pull).  Only thing that differs should be the UX.

Comment 2

[Benjamin Smedberg]

Let's use this public bug instead.

The openh264 download will not be a Mozilla-specific .xpi: it will probably be a raw DLL download, but may be an archive with very basic metadata. It certainly won't have an install.rdf.

It's unclear yet where which service will be serving install/update manifests for this plugin.

Comment 3

[Benjamin Smedberg]

Requirements:

* We need to be able to verify the bits to a Mozilla-trusted source (Mozilla update server)
* We expect the plugin API to change for a while yet, and so we're going to need to serve different Cisco plugins to different release channels
* It would be nice to be able to issue out-of-band updates to the plugin that didn't require a full Firefox respin. This may not be a 100% requirement, if we're willing to respin a Firefox dot-release.
* After somebody updates Firefox, they may have an "old" OpenH264 plugin and we need to decide whether it's ok to keep using it or not (blocklisting, perhaps a separate bug)

Comment 4

[Benjamin Smedberg]

The manifests will be handled through AUS. Here is the schema that I proposed that we should use a spec or strawman:


<updates>
  <addons>
    <addon id="[e-mail]"
uri="http://download.cdn.cisco.com/openh264/win32/openh264-1.1.zip"
       hash="sha256:ABD123409..."
       version="1.1"/>
  </addons>
</updates>

If there is a Firefox update available, minimum requirement:

<updates>
  <update>...</update>
  <addons>
    <addon id="[e-mail]"
uri="http://download.cdn.cisco.com/openh264/win32/openh264-1.1.zip"
       hash="sha256:ABD123409..."
       version="1.1"/>
  </addons>

If there is a Firefox update available, we could provide an advisory download for the new OpenH264 version. This is NOT a v1 requirement:

<updates>
  <update ...><patch .../>
    <addon id="[e-mail]"
uri="http://download.cdn.cisco.com/openh264/win32/openh264-1.2.zip"
       hash="sha256:1245678..."
       version="1.2"/>
  </update>
  <addons>
    <addon id="[e-mail]"
uri="http://download.cdn.cisco.com/openh264/win32/openh264-1.1.zip"
       hash="sha256:ABD123409..."
       version="1.1"/>
  </addons>
</updates>

rstrong if I'm reading http://hg.mozilla.org/mozilla-central/annotate/668f29cd71b3/toolkit/mozapps/update/nsUpdateService.js#l3323 and 1716 correctly, having the extra toplevel <addons> element will not affect existing clients. Can you confirm?

Comment 5

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

What is the rationale for adding this to app update instead of either plugin finder service or add-ons manager? I am very concerned since adding complexity to app update can cause breakage to app update which is not easy to recover from whereas breaking either the plugin finder service or the add-ons manager can be fixed using app update.

Comment 6

[Benjamin Smedberg]

We investigated adding this to AMO (addon update), blocklist, PFS, AUS, and a separate ping. As it stands, the requirement that this be able to ride trains and be tightly coupled to Firefox releases even though it is a separate product led me to the conclusion that AUS is the right choice. AMO doesn't have the concept of release channels and doesn't really have any active developers. PFS is dead. A separate ping would be ok, but both I and the privacy team would very much like to avoid new pings if they aren't necessary.

It appears to me that the risk of adding this to the AUS ping can be well-contained. We'll just need to code a little defensively.

Comment 7

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

Since this is the first I have heard about it I want to consider this further.

We code very defensively for app update but as you have seen we are loosing around 2% of users per release without a clear reason as to why. Adding this could easily further complicate that even with coding defensively.

A couple of concerns off the top of my head.

We have already moved away from verifying the ssl certificate of AUS on Windows and will be doing the same on Mac and Linux soon. We did this since there are edgecases (you emailed me about the proxy server edgecase) and I do not want to reintroduce that since it prevents people from updating.

The blocklist service understands channels. Also, app update can be disabled with Firefox UI whereas the blocklist can only be disabled via hidden prefs.

There is nothing that app update has for verifying this payload beyond the simple hash check after download which is not robust which is why mar signing has been implemented on Windows and is almost finished for Mac and Linux.

Comment 8

[Brian R. Bondy [:bbondy]]

> the requirement that this be able to ride trains

OK

> and be tightly coupled to Firefox releases even though it is a separate product

I don't want to complicate things if it is not necessary, but I just wanted to mention a possible security consideration relating to this requirement. 

We may want a signature and not just a hash for the hashes which are planned to be in the AUS manifest.  We'd use the per channel private key which are also used for signing MAR files.  This would ensure that a separate product could never be installed with a wrong channel.
We could add a command to the signmar program to take in an arbitrary binary file to produce such a signature pretty easily.

We would also include the file size, channel name, and applicable version in the hash calculation.

Bug 902761 makes it so the public key certs are included as binary blobs as a simple header file, so we could verify what we downloaded either in the updater binary or firefox binary cross platform before it gets installed.

Comment 9

[Andreas Gal :gal]

How are we doing here? Do we have an ETA?

Comment 10

[Brian R. Bondy [:bbondy]]

Talking with bsmedberg and rstrong about implementation details. It's desired to have this for FF33.

Comment 11

[Marco Mucci [:MarcoM]]

Hi Brian, would you recommend this bug be marked as [qa+] or [qa-] for QA verification?

Comment 12

[Brian R. Bondy [:bbondy]]

Once done, it'll involve pretty involved testing and need QA help. So I think qa+.

Comment 13

[Marco Mucci [:MarcoM]]

Thanks Brian.

Comment 14

[Brian R. Bondy [:bbondy]]

So I'm going ahead with this plan, we can change it later if needed depending on bug 1013354.
Does this sound OK?

Expectations on bug 1013354:
- Addons will be similarly specified as in Comment 4 inside the aus update response ping.
- Only addons for the current platform will be returned (The OS info is sent up in the request URL)
- Only the most recent addon will be returned that should be installed.
- If something is returned, it's wanted for it to be installed. And it will be installed if not already installed.

Limited scope of this bug:
- There is no UI for this bug.
- There is no ability to uninstall in this bug.
- There is no ability to detect if it is installed other than looking at spawned processes when using h264.
- There is no conditional installations in this bug, if it's returned, it'll be installed unless already installed for the same version.
- Verifying response addons to be certified-signed by mozilla via using the channel's MAR public key will happen in a follow up bug if we deem it is necessary.
**If any of those are needed, please post follow up bugs, or let me know that you want them and I can post them. **

Plans:
- When checking for updates, if there is one or more addons specified. It'll first check if the matching version is already installed. If not, it will install it as per https://wiki.mozilla.org/GeckoMediaPlugins
- I may need to cache the URL for the addons to be processed on postUpdate to avoid having an extra update ping. 
- Otherwise normal update pings can spur an installation at any time. This covers the case of new installs.

Comment 15

[Benjamin Smedberg]

(In reply to Brian R. Bondy [:bbondy] from comment #14)
> So I'm going ahead with this plan, we can change it later if needed
> depending on bug 1013354.
> Does this sound OK?

Yes, that's right. It would be nice to implement this with mock data before bug 1013354 is done, but in production it depends.

> - If something is returned, it's wanted for it to be installed. And it will
> be installed if not already installed.

This is true of OpenH264 specifically. Other addons will be opt-in by the user. So for now please scope this specifically to the OpenH264 addon.

> Limited scope of this bug:
> - There is no UI for this bug.
> - There is no ability to uninstall in this bug.
> - There is no ability to detect if it is installed other than looking at
> spawned processes when using h264.

The UX is being done in bug 1009909. So the addon manager code is going to need to know the status of whether the OpenH264 plugin is installed from either this code or from the media-plugins code.

> - There is no conditional installations in this bug, if it's returned, it'll
> be installed unless already installed for the same version.

In the UX, the user will have the ability to disable the plugin, which means we should uninstall it and not install new versions. So we do need to be able to do this conditionally.

> - Verifying response addons to be certified-signed by mozilla via using the
> channel's MAR public key will happen in a follow up bug if we deem it is
> necessary.

Agreed.

Comment 16

[Brian R. Bondy [:bbondy]]

Thanks.

> So for now please scope this specifically to the OpenH264 addon.

Will do, I'll only proceed for now if  id==="[e-mail]"

The UX is being done in bug 1009909. So the addon manager code is going to
> need to know the status of whether the OpenH264 plugin is installed from
> either this code or from the media-plugins code.

There are other ways to install not related to the work in this bug. So that work should be done elsewhere. 

> So we do need to be able to do this conditionally.

Since the details aren't worked out yet, I'll have to do that in another bug after we have some kind of signal that it has been uninstalled and should not be re-installed. I mentioned that in the UX bug.

Comment 17

[Brian R. Bondy [:bbondy]]

Attachment: WIP - Add support for updates/addons parsing (untested)

Just uploading work done for update parsing.
Also have some work for zip extraction but still more to do before I upload that one.

Comment 18

[Brian R. Bondy [:bbondy]]

Attachment: WIP - Add support for updates/addons parsing (untested)`

qref'ed

Comment 19

[Brian R. Bondy [:bbondy]]

Attachment: WIP v2

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: update-parsing.diff

v2 from last update:
- added lots of parsing stuff
v3:
- added downloading of GMP addon
- added verifying of GMP addon

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> We investigated adding this to AMO (addon update), blocklist, PFS, AUS, and
> a separate ping. As it stands, the requirement that this be able to ride
> trains and be tightly coupled to Firefox releases even though it is a
> separate product led me to the conclusion that AUS is the right choice. AMO
> doesn't have the concept of release channels and doesn't really have any
> active developers.

*sigh* Wish I'd been talked to about this.

I had wanted this to be included as part of the Add-ons Manager - which basically gives us a lot of things for free, including secure updates. And that doesn't need to involve AMO at all - all it needs is an update.rdf manifest hosted somewhere. If it needs to be different per channel, we put the channel in the URL we request. If it needs to be tied to when the app gets updated, the app update service asks the Add-ons Manager to stage an update (we want to look at doing this for normal add-ons anyway). I don't know why we're writing all this duplicate code, when we already have a well-tested infrastructure that does this.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

This was largely assuming what I said in bug 1009909 comment 20. However, given the immediate plan in bug 1009909 comment 21, it doesn't seem workable. At least, not until we replace that quick hack with a more thorough implementation.

Comment 23

[Brian R. Bondy [:bbondy]]

I'm continuing as normal on this work, but please let me know if Comment 21 and Comment 22 changes anything, and if in that case, Blair would be the new owner.  Just want to avoid the opportunity cost of other undone tasks early, if this won't be used.

Comment 24

[Benjamin Smedberg]

Continue as originally specced. Cisco won't be making .xpis and some of the other issues around build-specific updates would also require additional AOM work, so getting full AOM integration would be too large for this timeframe.

Comment 25

[Brian R. Bondy [:bbondy]]

Attachment: WIPv4

Added installing (extracting of downloaded zip) of GMP plugin to OSX path only so far.

Comment 26

[Georg Fritzsche [:gfritzsche]]

Brian, would this bug result in a way to trigger an update check via nsIUpdateService etc. or shall i file a follow-up for this?

Comment 27

[Brian R. Bondy [:bbondy]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #26)
> Brian, would this bug result in a way to trigger an update check via
> nsIUpdateService etc. or shall i file a follow-up for this?

I can do that but please file a follow up and assign to me. Thanks.

Comment 28

[Brian R. Bondy [:bbondy]]

I spoke with rstrong and he expressed several concerns with this being part of the update service.
For one, checks can be administratively disabled and Linux distros ship without nsUpdateService. There are other concerns too.

My current plan is to move this to an independent JavaScript module which checks the same URL. Would a JSM work for you gfritzsche?

Comment 29

[Georg Fritzsche [:gfritzsche]]

(In reply to Brian R. Bondy [:bbondy] from comment #28)
> My current plan is to move this to an independent JavaScript module which
> checks the same URL. Would a JSM work for you gfritzsche?

Sure, that sounds fine to me.

Comment 30

[Georg Fritzsche [:gfritzsche]]

Per mail we agreed on the updater setting the prefs "media.openh264.path" and "media.openh264.version" after install/update.
I realized today that an additional "media.openh264.lastUpdate" (timestamp?) would be great for the addon provider UI.

Comment 31

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev1

There will be another patch to use the module, but this is the module, tests and registration.

high level overview:

Typical expected AUS response:
<?xml version=\"1.0\"?>
    <updates>
        <addons>
            <addon id="[e-mail]"
                   uri=“http://download-path-here"
                   hash="sha256:5e7356990fd3d5d570c92b0d75115a658f6782f25c6c77883be402a12325386d"
                   version="1.1"/>
      </addons>
    </updates>
Instead of a hash attribute, you can specify hashFunction and hashValue if you prefer.


Import the module:
Cu.import("resource://gre/modules/GMPInstallManager.jsm");

GMPInstallManager
checkForAddons (Returns an array of GMPAddon objects)
installAddon (Takes in a GMPAddon, and calls a callback when done with the list of extracted paths)

GMPAddon
id
uri
version
hashFunction
hashValue

The module exposes these objects/constructors:
["GMPInstallManager", "GMPExtractor", "GMPDownloader",
 "GMPAddon", "GMPPrefs"];

Comment 33

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8451601 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev1

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

::: toolkit/modules/GMPInstallManager.jsm
@@ +33,5 @@
> +});
> +
> +// Helper method so logs only show up when media.gmp-manager.log is true
> +function LOG(module, message) {
> +  if (gLogEnabled) {

What about using Log.jsm?

@@ +50,5 @@
> +   * @param defaultValue The default value if no preference exists
> +   * @return The obtained preference value, or the defaultVlaue if none exists
> +   */
> +  getValue: function(key, addon, defaultValue) {
> +    var func = this.isBoolPref(key) ? "getBoolPref" : "getCharPref";

Why not use Preferences.jsm?

Comment 34

[Georg Fritzsche [:gfritzsche]]

And a more general question: Shouldn't we use promises instead of callbacks for new async APIs?

Comment 35

[David Teller [:Yoric] - still alive but not very active]

I strongly vote for Promise or, even better, Task. We now have infrastructure that ensures that we have much better error reporting and test coverage if we use Promise/Task, plus they handle deferring to the next tick and releasing the stack.

Comment 36

[Brian R. Bondy [:bbondy]]

(In reply to Georg Fritzsche [:gfritzsche] [away July 10-14] from comment #33)
> Comment on attachment 8451601 [details] [diff] [review]
> Base implementation and tests for module to check, download, and install
> openh264 updates. rev1
> 
> Review of attachment 8451601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/modules/GMPInstallManager.jsm
> @@ +33,5 @@
> > +});
> > +
> > +// Helper method so logs only show up when media.gmp-manager.log is true
> > +function LOG(module, message) {
> > +  if (gLogEnabled) {
> 
> What about using Log.jsm?
> 
> @@ +50,5 @@
> > +   * @param defaultValue The default value if no preference exists
> > +   * @return The obtained preference value, or the defaultVlaue if none exists
> > +   */
> > +  getValue: function(key, addon, defaultValue) {
> > +    var func = this.isBoolPref(key) ? "getBoolPref" : "getCharPref";
> 
> Why not use Preferences.jsm?

I have the object to scope prefs in the context of addons easily but I can aimprove that type check thing by using Preferences.jsm.  Will do.

> What about using Log.jsm?

Sure I can update to that. I had it this way because the implementation started inside nsUpdateService.js and that's what it does.

> And a more general question: Shouldn't we use promises instead of callbacks for new async APIs?

If you prefer, sure I can easily update to promises.

> I strongly vote for Promise or, even better, Task. We now have infrastructure that ensures that we 
> have much better error reporting and test coverage if we use Promise/Task, plus they handle deferring 
> to the next tick and releasing the stack.

I haven't used Task.jsm but I'll look into using that. Thanks for the suggestion.

Comment 37

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

(In reply to Brian R. Bondy [:bbondy] from comment #31)
> Created attachment 8451601 [details] [diff] [review]
> Base implementation and tests for module to check, download, and install
> openh264 updates. rev1
> 
> There will be another patch to use the module, but this is the module, tests
> and registration.
> 
> high level overview:
> 
> Typical expected AUS response:
> <?xml version=\"1.0\"?>
>     <updates>
>         <addons>
>             <addon id="[e-mail]"
>                    uri=“http://download-path-here"
>                   
> hash="sha256:
> 5e7356990fd3d5d570c92b0d75115a658f6782f25c6c77883be402a12325386d"
>                    version="1.1"/>
>       </addons>
>     </updates>
> Instead of a hash attribute, you can specify hashFunction and hashValue if
> you prefer.
Since this won't be needed by app update I suggest asking releng to go with 
<?xml version=\"1.0\"?>
  <addons>
    <addon id="[e-mail]"
           uri=“http://download-path-here"
           hash="sha256: 5e7356990fd3d5d570c92b0d75115a658f6782f25c6c77883be402a12325386d"
           version="1.1"/>
  </addons>

and only display updates for update checks and addons for these new checks. This way update checks which are usually empty updates won't receive the extra info that it won't use and with the number of these checks that happen daily that is quite a lot of extra unnecessary traffic. The url would of course need something to differentiate between the two.

Comment 38

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

Comment on attachment 8451601 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev1

There has been some discussion of not using incremental download for app update and I think for this use case it likely makes sense to just download it.

Please use new prefs for this instead of the app.update.* prefs.

Please use a dir other than "updates" so there won't be a chance of it restricting app update changes in the future. I think using "UpdRootD" might be fine though keep in mind that it will also be changing over time so it might be a good thing to just use something else since the requirements are different.

Comment 39

[Brian R. Bondy [:bbondy]]

(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #38)
> Comment on attachment 8451601 [details] [diff] [review]
> Base implementation and tests for module to check, download, and install
> openh264 updates. rev1
> 
> There has been some discussion of not using incremental download for app
> update and I think for this use case it likely makes sense to just download
> it.

All the rest of the stuff sounds good but is there a compelling reason to change from incremental downloader? This code might eventually be moved into addon manager stuff and the current implementation with incremental is already done and tests written.

Comment 40

[Brian R. Bondy [:bbondy]]

> Since this won't be needed by app update I suggest asking releng to go with 

Agreed but I'll wait until they give me the final format to change that. For now I'm just going off the spec on Comment 4.

Comment 41

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

iirc the add-ons mgr doesn't use incremental downloads and the change shouldn't require too much in the way of changes. Take a look at what it would take and we'll decide from there.

I talked with bsmedberg and others about the format and please push for the change or at the very least not sending the extra info in the app update check. If there is resistance please let me know.

Comment 42

[Brian R. Bondy [:bbondy]]

> Since this won't be needed by app update I suggest asking releng to go with

I've heard using a different endpoint+response might require a privacy review.
To my code it doesn't matter though. In any case there will be 2 pings.  If there's a privacy concern, the pings will be made to the same URL. But that might be one reason to keep it as per Comment #4.

Comment 43

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

I would hope since it will be sending the same info or possibly a subset of the same info that a privacy review wouldn't be a problem.

Comment 44

[Brian R. Bondy [:bbondy]]

I'd hope so too but I'm not sure what the policy is involving a different endpoint. So thought I'd mention it.

Comment 45

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev2

Done:
- Updated to promises
- Updated to use Preferences.jsm (still have GMPPrefs for easy add-on scopped set/get)
- Updated to use Log.jsm

TODO:
- rstrong's review comments
- Use Task.jsm

Comment 46

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8452889 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev2

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

I only quickly skimmed through GMPInstallManager.jsm (sorry, not much time today), but the API usage in the test looks good.

::: toolkit/modules/GMPInstallManager.jsm
@@ +137,5 @@
> +    this._request.setRequestHeader("Pragma", "no-cache");
> +
> +    this._request.addEventListener("error", function(event) {
> +      this.onErrorXML(event);
> +    }.bind(this) ,false);

You could just go with:
> event => this.onErrorXML(event)
... or:
> this.onErrorXML.bind(this)

::: toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
@@ +66,5 @@
> + */
> +add_test(function test_checkForAddons_noAddonsElement() {
> +  overrideXHR(200, "<updates></updates>");
> +  var promise = GMPInstallManager.checkForAddons();
> +  promise.then(function(gmpAddons) {

Now, if you switch to add_task() instead of add_test() you can go for:
> let gmpAddons = yield GMPInstallManager.checkForAddons();

@@ +320,5 @@
> +
> +    GMPInstallManager.overrideInstallPath = dir.path;
> +    GMPInstallManager.overrideLeaveDownloadedZip = true;
> +    var installPromise = GMPInstallManager.installAddon(gmpAddon);
> +    installPromise.then(function(extractedPaths) {

> let gmpAddons = yield GMPInstallManager.checkForAddons();
> ...
> let extractedPaths = yield GMPInstallManager.installAddon(gmpAddon);

Comment 47

[Brian R. Bondy [:bbondy]]

sweet, great suggestions, thanks.

Comment 48

[Brian R. Bondy [:bbondy]]

TODOext:
- Add logic on the client side to only install things if they aren't installed for the last installed version.

Comment 49

[[e-mail] (:bhearsum)]

Brian, per our IRC conversation earlier, the h264 update URL should be exactly the same as the Firefox one, except use "h264" (or "H264" - I don't care which) instead "Firefox" in the %PRODUCT% field. Everything else should be the same -- it should include the Firefox version, buildid, platform, etc. I know we talked briefly about putting the h264 version in there, but because we're always returning the latest version, there's no need for that - it's more important than we have the Firefox version/buildid so that we can serve the correct plugin when we need to serve different plugins to different Gecko versions/buildids. So, you probably want:
https://aus4.mozilla.org/update/3/h264/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml

and maybe to factor the "h264" string out somewhere.

Does that sound okay to you?

Comment 50

[Brian R. Bondy [:bbondy]]

Maybe we should scrope the URL to Gecko Media Plugins (GMP) instead? 
I'm just wondering for when we want more than h264. 
https://aus4.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml
You'd only return h264 though.

In the future I don't think we want 1 update ping per media plugin.

Comment 51

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev3

Done in this rev
- Updated to mirrored prefs under media.gmp-manager.* instead of app.update.*
- Updated update url per bhearsum / my discussion in 1009816
- Updated to fix the review comments from gfritzsche (still have a few yields to do)
- Updated code to use URL for parsing instead of uri per schema change
- updated parsing to no longer accept hash parameter
- updated parsing to parse optional size argument
- added code to reject the promise if the sizes don’t match if it was specified in the manifest-

Comment 52

[[e-mail] (:bhearsum)]

(In reply to Brian R. Bondy [:bbondy] from comment #50)
> Maybe we should scrope the URL to Gecko Media Plugins (GMP) instead? 
> I'm just wondering for when we want more than h264. 
> https://aus4.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/
> %LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.
> xml
> You'd only return h264 though.
> 
> In the future I don't think we want 1 update ping per media plugin.

I'll admit I don't quite understand the difference. GMP sounds like it makes more sense, but can we chat on IRC so I can get up to speed on this?

Tangentially related, what format are you expecting the file referenced by the URL part of the response to be in? I got e-mail last night that said there's two files that need to be returned, so we'll need to package them up in some way (tarball? xpi? mar doesn't really make sense here).

Comment 53

[Benjamin Smedberg]

Who sent that email? I've said several times that GMP plugins need to be a zip file with the two files inside, from when we build and hand it to Cisco and in the download.

Comment 54

[Brian R. Bondy [:bbondy]]

sure ping me anytime.

My code extracts a zip file containing any number of files/folders if the hatches match (typically this will be 2 files inside a zip).

Basically openh264 is one gecko media plugin. There could be others in the future.
It's my understanding that we'll be pointing to someone else's site (such as cisco) who will host the actual GMP plugin. We'd always point to the zip containing the files to be extracted. 

More info on gecko media plugins:
https://wiki.mozilla.org/GeckoMediaPlugins

Comment 55

[Brian R. Bondy [:bbondy]]

I filed bug 1037767 for the incremental downloader replacement change. That part can be done eventually if GMPInstallManager is not obsoleted. It's not going to be a high priority though for now. Please speak out if that's a problem :)

Comment 56

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev4

Probably a good time to take another pass.

Done in rev4:
- Use a dir other than updates
- Added isInstalled property which detects if it is installed, and added tests for it.
- var -> let for better scoping 
- Convert computeHash and verifyHash to use promise/deferred 
- Added a test for if file size is specified it works and doesn’t reject, also for no file size specified passes
- Added a test for wrong file size specified vs downloaded rejects
- Cleaned up regular functions w/ bind(this) to arrow functions which have implicit bind(this)
- Converted GMPInstallManager to no longer be a singleton for better usage with tasks.
- Added support for concurrent downloading 2 different GMP plugins
- Got rid of allow override install dir from testing to get closer test coverage to real code
- Now install to profile dir on all platforms

TODO:
- Find a good place to use this component, probably delayed startup in firefox front end code?
- Use OSFile more? I’m a bit constrained here because zip reader requires nsIFile, possibly for hashing though I can use NetUtils read stream async for calculating the downloaded file hash.  Or maybe it doesn’t matter - because we can call things in a task?

Comment 57

[Georg Fritzsche [:gfritzsche]]

I can't really find time to look into it before tuesday, maybe someone from #fx-team can look over it if you need input earlier?

> - Use OSFile more? I’m a bit constrained here because zip reader requires
> nsIFile, possibly for hashing though I can use NetUtils read stream async
> for calculating the downloaded file hash.  Or maybe it doesn’t matter -
> because we can call things in a task?

We should really try to avoid main-thread IO, but a task alone doesn't help here. Maybe Yoric has good suggestions here?

Comment 58

[Brian R. Bondy [:bbondy]]

I think I want toolkit/components/promiseworker/PromiseWorker.jsm for the off the main thread stuff which I think is somewhat like a web worker but with chrome privs.
The interface for my component probably won't change at all though.
That work will be done in the next patch, so shouldn't hold up the current patch review.

Comment 59

[Brian R. Bondy [:bbondy]]

Nevermind it seems like off the main thread xpcom is no longer possible. I think the way to go is to allow for off the mean thread zip reading in a follow up (high priority and for uplifting) and land here with on the main thread IO (but eliminate the other cases of main thread IO where possible in this bug). Will wait for dteller's recommendation though.

I'm working on using the component in the meantime from front end code.

Comment 60

[David Teller [:Yoric] - still alive but not very active]

PromiseWorker is ChromeWorker + tools to handle exceptions and place cross-thread Promise-based function calls. ChromeWorker is Web Worker + js-ctypes. So, while both ChromeWorker and PromiseWorker can be useful for accessing native code off the main thread, as bbondy remarked in comment 59, they do not give access to XPCOM automatically.

However, if necessary, it is quite feasible to wrap a XPCOM API behind some C entry points, then call these C entry points from a ChromeWorker or PromiseWorker using js-ctypes. We do (almost) this for lz4 already.

Comment 61

[Brian R. Bondy [:bbondy]]

Thanks for the info.
I think it would probably be easiest to extend the functionality of the components I need to have async off the main thread APIs.

Comment 62

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev5

OK everything is done that will be done in this bug.  I'll be filing a follow up to get rid of the on the main thread IO cases when updates are actually found. But this is only for when updates are found so the plan is to land like that and fix the on the main thread stuff in a followup and uplift to FF33.

Extra things done in this patch:
- Added front end code to do a check & install of add ons if not already installed and if h264
- Added min time interval for new checks so it doesn’t happen on every restart
- Replace gmp app update vars in URL
- Use update URL and skip cert check if override is specified

Comment 63

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8456619 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev5

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

Overall this looks good!
For the GMPInstallManager, i didn't look to closely at the downloading/DOM parsing/extracting/file handling parts, but rather on the API, structure and async handling.

I think there is a few functions, like verifyDownload(), that could benefit a lot from switching to Task.async()/Task.spawn() and |yield|ing the async things they are doing.
There is also some things, like the XHR request, that could get wrapped & isolated with a promise-returning function.
But given the release-schedule and that this seems fine overall we should not focus on that now.

We should definitely fix hitting early during startup.

Open question: What happens if i hit

::: browser/base/content/browser.js
@@ +9,5 @@
>  Cu.import("resource://gre/modules/XPCOMUtils.jsm");
>  Cu.import("resource://gre/modules/NotificationDB.jsm");
>  Cu.import("resource:///modules/RecentWindow.jsm");
>  Cu.import("resource://gre/modules/WindowsPrefSync.jsm");
> +Cu.import("resource://gre/modules/GMPInstallManager.jsm");

XPCOMUtils.defineLazyModuleGetter() so we don't immediately hit this?

@@ +1220,5 @@
>  
> +    this.gmpInstallManager = new GMPInstallManager();
> +    // We don't really care about the results, if somenoe is interested they
> +    // can check the log.
> +    this.gmpInstallManager.simpleCheckAndInstall();

Talking to Yoric, doing this in _delayedStartup is definitely too early.
You could hang it off the "sessionstore-windows-restored" notification (maybe even delay it a little from there as it doesn't have to load immediately).

::: toolkit/modules/GMPInstallManager.jsm
@@ +117,5 @@
> +};
> +
> +// This is copied directly from nsUpdateService.js
> +// It is used for calculating the URL string w/ var replacement.
> +// TODO: refactor this out somewhere else

Can we file a follow-up bug for these?

@@ +291,5 @@
> +  _getURL: function() {
> +    let log = getScopedLogger("GMPInstallManager._getURL");
> +    // Use the override URL if it is specified.  The override URL is just like
> +    // the normal URL but it does not check the cert.
> +    let url = GMPPrefs.get(GMPPrefs.KEY_URL_OVERRIDE);

Why not just use a hidden boolean pref to disable the cert check?

@@ +306,5 @@
> +         .replace(/%BUILD_ID%/g, Services.appinfo.appBuildID)
> +         .replace(/%BUILD_TARGET%/g, Services.appinfo.OS + "_" + gABI)
> +         .replace(/%OS_VERSION%/g, gOSVersion);
> +    if (/%LOCALE%/.test(url)) {
> +      // TODO: Get the real local, does it actually matter for GMP plugins?

I guess we may need to grab localized info files for name/descriptions later when we support more GMP plugins?
Either way, this doesn't matter for OpenH264 now.

@@ +387,5 @@
> +    let lastCheck = GMPPrefs.get(GMPPrefs.KEY_UPDATE_LAST_CHECK,
> +                                 undefined, 0);
> +    // Handle clock jumps
> +    if (now < lastCheck) {
> +      return now;

Is returning |now| here intentional? If we always want to trigger updates we could use a more explicit value?

@@ +399,5 @@
> +  /**
> +   * Wrapper for checkForAddons and installaddon.
> +   * Will only install if not already installed and will log the results.
> +   */
> +  simpleCheckAndInstall: function() {

Can't this return a promise? You could first do the sync parts, then |return Task.spawn(function*() { ... });| where you |yield| through the async parts.

What happens if i call this function, checkForAddons() and installAddon() while any of them might already be active?
Can we track the ongoing tasks/promises and, at least for checkForAddons(), just return instead of retriggering them?

@@ +409,5 @@
> +   log.info("Last check was: " + secondsSinceLast +
> +            " seconds ago, minimum seconds: " + secondsBetweenChecks);
> +   if (secondsBetweenChecks > secondsSinceLast) {
> +     log.info("Will not check for updates.");
> +     return;

This would just return Promise.resolve().

@@ +413,5 @@
> +     return;
> +   }
> +
> +   let promise = this.checkForAddons();
> +    promise.then(gmpAddons => {

Indentation is off here.

@@ +572,5 @@
> +    }
> +  });
> +  this.size = Number(this.size) || undefined;
> +  // In case we have something like [e-mail], we only want
> +  // the stuff before the @ symbol.

Oh, why? The full id seems fine and would be less likely to hit collisions.

@@ +632,5 @@
> +   * Open H264 has special handling.
> +   * @return true if the plugin is the openh264 plugin
> +   */
> +  get isOpenH264() {
> +    return this.id === "openh264-plugin";

Just use "[e-mail]" and have the string as a constant somewhere?

@@ +756,5 @@
> +               createInstance(Ci.nsICryptoHash);
> +    let hashFunction =
> +      Ci.nsICryptoHash[hashFunctionName.toUpperCase()];
> +    if (!hashFunction) {
> +      throw Cr.NS_ERROR_UNEXPECTED;

|Promise.reject()|?

@@ +767,5 @@
> +    digest = "";
> +  }
> +  fileStream.close();
> +  computeHashDeferred.resolve(digest);
> +  return computeHashDeferred.promise;

Just |return Promise.resolve(digest)| here.

@@ +785,5 @@
> +        target: this,
> +        status: status,
> +        type: "downloaderr"
> +      });
> +      return this._deferred.promise;

No need for the Deferred here yet, just |return Promise.reject(...)|

@@ +865,5 @@
> +    let log = getScopedLogger("GMPDownloader._verifyDownload");
> +    log.info("_verifyDownload called");
> +    if (!this._request) {
> +      verifyDownloadDeferred.reject();
> +      return verifyDownloadDeferred.promise;

Just |return Promise.reject()|.

@@ +878,5 @@
> +      log.warn("Downloader:_verifyDownload downloaded size " +
> +               destination.fileSize + " != expected size " +
> +               this._gmpAddon.size + ".");
> +      verifyDownloadDeferred.reject({});
> +      return verifyDownloadDeferred.promise;

|return Promise.reject()|

Comment 64

[Brian R. Bondy [:bbondy]]

> Is returning |now| here intentional?

Yep it was intentional, addon manager does the same thing, basically a large value.  I'll add a comment to clarify. 

> Just use "[e-mail]" and have the string as a constant somewhere?

I think I can use the whole ID. I wasn't sure if it was OK to put an @ in a pref name or not.  I'll update it to use thew hole ID.

> Why not just use a hidden boolean pref to disable the cert check?

This basically mirrors the same as what app update does. It's handy for quick testing of a local dev server. You only need to set 1 pref instead of having to set the url and the bool pref.

> Can we track the ongoing tasks/promises and, at least for checkForAddons(), just return instead of 
> retriggering them?

I don't see a big benefit in doing that. I think it's fine to just create a new instance of GMPInstallManager and do a new check from addon manager. Let me know if you strongly disagree.

Otherwise everything sounds good. Thanks for the feedback!

Comment 65

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8456619 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev5

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

::: toolkit/modules/tests/xpcshell/test_GMPInstallManager.js
@@ +49,5 @@
> +
> +/**
> + * Tests that no response returned rejects
> + */
> +add_test(function test_checkForAddons_noResponse() {

All tests here could be tasks and |yield|.
To test rejection reasons note this:
"If you specify a promise, the yield operator returns its resolution value as soon as the promise is resolved. If the promise is rejected, its rejection reason is thrown as an exception."

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm#Introduction

Comment 66

[Georg Fritzsche [:gfritzsche]]

... and completely forgot to mention: Generator functions should use the |function* foo() {...}| syntax. The syntax without the * is deprecated and pre-standard.

Comment 67

[Georg Fritzsche [:gfritzsche]]

This causes browser tests to hit the network.

I locally added this to testing/prefs_general.js for mochitest etc.:
> // Make sure GMPInstallerManager won't hit the network.
> user_pref("media.gmp-manager.url", "https://%(server)s/dummy.xml");

Not sure what else needs to be set at the moment, Ryan?

Comment 68

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09d24ac458bb should give you some idea, but it also depends on what platforms GMP is enabled on. Your best bet is to push to Try and see what burns.

Comment 69

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

Comment on attachment 8456619 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev5

I'll try to get this done today... here is what I have so far.

>diff --git a/browser/app/profile/firefox.js b/browser/app/profile/firefox.js
>--- a/browser/app/profile/firefox.js
>+++ b/browser/app/profile/firefox.js
>...
>@@ -1576,16 +1578,41 @@ pref("identity.fxaccounts.settings.uri",
> // On GTK, we now default to showing the menubar only when alt is pressed:
> #ifdef MOZ_WIDGET_GTK
> pref("ui.key.menuAccessKeyFocuses", true);
> #endif
> 
> // Encrypted media extensions.
> pref("media.eme.enabled", false);
> 
>+// GMPInstallManager prefs
>+// See app.update.* pref descriptions for a description of what each pref is
>+// for. The prefs are logically equivalent.
Go ahead and copy the descriptions. The app update certificate prefs will be removed after mar signing is finished on Mac and Linux.

>+pref("media.gmp-manager.log", false);
>+//pref("media.gmp-manager.url.override", "");
>+#ifndef RELEASE_BUILD
>+pref("media.gmp-manager.url", "https://aus4.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml");
>+#else
>+pref("media.gmp-manager.url", "https://aus3.mozilla.org/update/3/GMP/%VERSION%/%BUILD_ID%/%BUILD_TARGET%/%LOCALE%/%CHANNEL%/%OS_VERSION%/%DISTRIBUTION%/%DISTRIBUTION_VERSION%/update.xml");
Was GMP also added to aus3? Ask bhearsum and if not only use aus4.

>+#endif
>+pref("media.gmp-manager.cert.requireBuiltIn", true);
>+pref("media.gmp-manager.cert.checkAttributes", true);
>+pref("media.gmp-manager.cert.maxErrors", 5);
It doesn't look like you use maxErrors. If not, remove this.

>+#ifndef RELEASE_BUILD
>+pref("media.gmp-manager.certs.1.issuerName", "CN=DigiCert Secure Server CA,O=DigiCert Inc,C=US");
>+pref("media.gmp-manager.certs.1.commonName", "aus4.mozilla.org");
>+pref("media.gmp-manager.certs.2.issuerName", "CN=Thawte SSL CA,O=\"Thawte, Inc.\",C=US");
>+pref("media.gmp-manager.certs.2.commonName", "aus4.mozilla.org");
>+#else
>+pref("media.gmp-manager.certs.1.issuerName", "CN=Thawte SSL CA,O=\"Thawte, Inc.\",C=US");
>+pref("media.gmp-manager.certs.1.commonName", "aus3.mozilla.org");
>+pref("media.gmp-manager.certs.2.issuerName", "CN=DigiCert Secure Server CA,O=DigiCert Inc,C=US");
>+pref("media.gmp-manager.certs.2.commonName", "aus3.mozilla.org");
>+#endif
>+

Note: cert pinning will likely replace the cert checks at some point. App update will be the last to use it so using the same host might prevent this from using it sooner.

Comment 70

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

Comment on attachment 8456619 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev5

You'll need to add this to
http://mxr.mozilla.org/mozilla-central/source/browser/installer/package-manifest.in

>diff --git a/toolkit/modules/tests/xpcshell/xpcshell.ini b/toolkit/modules/tests/xpcshell/xpcshell.ini
>--- a/toolkit/modules/tests/xpcshell/xpcshell.ini
>+++ b/toolkit/modules/tests/xpcshell/xpcshell.ini
>@@ -8,16 +8,17 @@ support-files =
>   zips/zen.zip
> 
> [test_AsyncShutdown.js]
> [test_BinarySearch.js]
> [test_DeferredTask.js]
> [test_dict.js]
> [test_DirectoryLinksProvider.js]
> [test_FileUtils.js]
>+[test_GMPInstallManager.js]
GMP will be included with all toolkit apps such as Thunderbird and Seamonkey?

If so, there package manifests will need to be updated as well.

Comment 71

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

Comment on attachment 8456619 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev5

I haven't thoroughly reviewed the tests but the rest looks good. r=me with the above addressed.

Would be a good thing to use the timer manager to check for updates for those users that seldom restart but I'm not going to hold this up for that.

Comment 72

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

Comment on attachment 8456619 [details] [diff] [review]
Base implementation and tests for module to check, download, and install openh264 updates. rev5

>...
>+/**
>+ * Creates a new zip file containing a file with the specified data
>+ * @param zipName The name of the zip file
>+ * @param data The data to go inside the zip for the filename entry1.info
>+ */
>+function createNewZipFile(zipName, data) {
>+   // Create a zip file which will be used for extracting
>+    let stream = Cc["@mozilla.org/io/string-input-stream;1"].
>+                 createInstance(Ci.nsIStringInputStream);
>+    stream.setData(data, data.length);
>+    let zipWriter = Cc["@mozilla.org/zipwriter;1"].
>+                    createInstance(Components.interfaces.nsIZipWriter);
>+    let zipFile = FileUtils.getFile("TmpD", [zipName]);
>+    if (zipFile.exists()) {
>+      zipFile.remove(false);
>+    }
>+    // From prio.h
>+    const PR_RDWR = 0x04;
>+    const PR_CREATE_FILE = 0x08;
>+    const PR_TRUNCATE    = 0x20;
>+    zipWriter.open(zipFile, PR_RDWR | PR_CREATE_FILE | PR_TRUNCATE);
>+    zipWriter.addEntryStream("entry1.info", Date.now(),
>+                             Ci.nsIZipWriter.COMPRESSION_BEST, stream, false);
>+    zipWriter.close();
>+    stream.close()
missing ;

Comment 73

[Brian R. Bondy [:bbondy]]

So all the review comments seem small except for using Task.jsm. I moved that to bug 1039839 so I can implement the rest of the nits and carry forward the r+ to land.

Comment 74

[Georg Fritzsche [:gfritzsche]]

From bug 1039711, comment 1:
> I want us to be very careful about using sessionstore-windows-restored for
> anything important: I'm not sure that it's ever called in some startup
> situations, currently.

What should we ideally hang off here then? Maybe a >=30s delay from "sessionstore-state-finalized" like ExperimentsService.js?

Comment 75

[Benjamin Smedberg]

In general I don't think it's safe to assume that any of the sessionstore notifications are always fired. Certainly the core startup code doesn't guarantee that they are fired. Especially in toolkit code since session restore is Firefox-specific.

Comment 76

[Brian R. Bondy [:bbondy]]

Attachment: 8456619: Base implementation and tests for module to check, download, and install openh264 updates. rev6

Carrying forward r+. Carrying forward r+ since there are neither major changes nor logic changes.

Comment 77

[Brian R. Bondy [:bbondy]]

https://tbpl.mozilla.org/?tree=Try&rev=a31a40344640

Comment 78

[Nils Ohlmeier [:drno]]

So I downloaded the Mac try build for 10.8 and started it on my 10.9 Mac with an empty profile. My understanding is that I do *NOT* have to do anything manually to trigger the download of the plugin, correct?
I don't it in Extension or Plugins showing up. And starting a call on http://mozilla.github.io/webrtc-landing/pc_test_h264.html results in VP8.

So its either broken, or I'm doing something wrong here. Advice, hints?

Comment 79

[Nils Ohlmeier [:drno]]

Same result for 64bit Linux opt on Ubuntu 14.04. No plugin or add-on. No H264 in the SDP of the github test page.

Comment 80

[Brian R. Bondy [:bbondy]]

(In reply to Nils Ohlmeier [:drno] from comment #78)
> So I downloaded the Mac try build for 10.8 and started it on my 10.9 Mac
> with an empty profile. My understanding is that I do *NOT* have to do
> anything manually to trigger the download of the plugin, correct?

Correct.

> I don't it in Extension or Plugins showing up. And starting a call on
> http://mozilla.github.io/webrtc-landing/pc_test_h264.html results in VP8.

It's possible the try build's update url isn't correct. I'm not sure if that can differ.
You can get more information by setting media.gmp-manager.log to true

> So its either broken, or I'm doing something wrong here. Advice, hints?

Possible this, or something else is broken too. There are a lot of parts to this, this bug just being one of them. Let's start by looking at the log.   The log will also tell you where things are installed to or if there's an error.  Search for GMP in the log.

Between tests you should reset media.gmp-manager.lastCheck.  This ensures that at most 1 check per day happens.  Checks happen after 1 minute of starting the browser.

Comment 81

[Nils Ohlmeier [:drno]]

I just verified on my Mac OSX 10.9 machine and found these settings:

[e-mail];1405630376
[e-mail];/Users/nohlmeier/Library/Application Support/Firefox/Profiles/l1atx3ab.h264/[e-mail]
[e-mail];1.0

Comment 82

[Nils Ohlmeier [:drno]]

Confirmed that gmpopenh264.info and libgmpopenh264.dylib are present at the path from the setting.
Never the less the github test call still shows VP8 only.

Comment 83

[Georg Fritzsche [:gfritzsche]]

This presumably depends on bug 1037754, bug 1032814, bug 1009909 and bug 1040048  (not sure if i missed any).
Does the try push include all those?

Comment 84

[Nils Ohlmeier [:drno]]

Unfortunately TBPL is not co-operating today. I can't see what is in the try push.
bbondy: can you confirm what went into that push?

Comment 85

[Nils Ohlmeier [:drno]]

More update: I saw the media.peerconnection.video.h264_enabled in the settings. But it was off. I switched it manually to on. But the test page still shows VP8 only.

Comment 86

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

(In reply to Benjamin Smedberg  [:bsmedberg] Away 19-July through 3-Aug from comment #75)
> In general I don't think it's safe to assume that any of the sessionstore
> notifications are always fired. Certainly the core startup code doesn't
> guarantee that they are fired. Especially in toolkit code since session
> restore is Firefox-specific.

I generally agree, for non-Firefox-specific code, but FWIW Firefox itself already relies on windows-restored always firing in a few different places. Avoiding adding more dependencies on that is a good idea, but I don't know of any cases in Firefox where it doesn't fire.

Comment 87

[Brian R. Bondy [:bbondy]]

Try try push was updated to m-c tip + my patch.
The update to m-c tip was done on 2014-07-17 06:46:01 PDT.

:drno it looks like the pref was set correctly and files installed. That means that this bug is passing.

One of the bugs mentioned in Comment 83 are likely the cause, unless any of those landed really recently.  If one of those landed after the update mentioned above, then we just need a new try push.

Comment 88

[Brian R. Bondy [:bbondy]]

Yep looks like everything is working so far. Bug 1040048 landed 2014-07-17 14:31:18 PDT.  So the try build just doesn't have it because the try build was before that.

Comment 89

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev7

Carrying forward r+.

Just waiting for try to open back up so I can push again.

Done6: 
Added ctypes module include for windows test failure
Added an explicit abort call on the xhr _request object and added a new uninit call


So just to clarify, this won't be in tonight's nightly. I need to push to try and it's been closed for most of the day.   It should land once I can get on try and it passes though. New estimate is landing tomorrow morning.

Comment 90

[Randell Jesup [:jesup] (needinfo me)]

 https://tbpl.mozilla.org/?tree=Try&rev=98fb3c863308

Comment 91

[Brian R. Bondy [:bbondy]]

Attachment: Base implementation and tests for module to check, download, and install openh264 updates. rev8

Fixed a couple of windows xpcshell errors due to not closing the zip reader, causing a file in use error from the tests.  Also made sure test cleans up after itself better.

Verified tests pass on Windows now and that it installs fine too.

Carrying forward r+

Comment 92

[Brian R. Bondy [:bbondy]]

https://tbpl.mozilla.org/?tree=Try&rev=75957b740387

Comment 93

[u279076]

(In reply to Randell Jesup [:jesup] from comment #90)
>  https://tbpl.mozilla.org/?tree=Try&rev=98fb3c863308

Nils, can you please retest this build?

Comment 94

[Nils Ohlmeier [:drno]]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #93)
> (In reply to Randell Jesup [:jesup] from comment #90)
> >  https://tbpl.mozilla.org/?tree=Try&rev=98fb3c863308
> Nils, can you please retest this build?

So I just tested the Mac OS 10.8 opt build from the URL above:

- The plugin downloaded. After that I have these settings:
[e-mail];1405706577
[e-mail];/Users/nohlmeier/Library/Application Support/Firefox/Profiles/7mhjgpou.h265/[e-mail]
[e-mail];1.0
media.openh264.providerEnabled;true
media.peerconnection.video.h264_enabled;false

=> result: no H264 call gets established!

I manually flipped the media.peerconnection.video.h264_enabled to true. And tried again a call on http://mozilla.github.io/webrtc-landing/pc_test_h264.html. No success. Still only VP8 gets offered.

BTW this particular build has a nasty UI problem in it. I'm going to attach a screenshot of it.

Comment 95

[Nils Ohlmeier [:drno]]

Attachment: Screen shot in a call with distorted camera button.

The screen shot shows the camera shared button to the right of the search field getting filled with some ugly characters instead of the proper symbol.

Comment 96

[Brian R. Bondy [:bbondy]]

Since the prefs are set ([e-mail]) and the files exist in the profile path, this bug is working. 

I'd prefer other issues to be tracked in the bugs that are for those issues if possible.
If you'd like 1 place to test overall all of the pieces working together, then one idea is to post a tracking bug for that. Thanks!

Comment 97

[Benjamin Smedberg]

Yes, let's file new bugs on issues found during QA. It appears to me that this is ready to land.

Comment 98

[Brian R. Bondy [:bbondy]]

Just waiting for the windows try tests to finish. Everything else has passed already.  This will likely land in an hour or so when that finishes.

Comment 99

[Maire Reavy [:mreavy]]

I agree with bsmedberg and bbondy.   The test issues drno is seeing are outside bbondy's code.

Comment 100

[Brian R. Bondy [:bbondy]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ada22bb1f40

Comment 101

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/mozilla-central/rev/1f124b3a1355

Comment 102

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8ada22bb1f40

Comment 103

[Brian R. Bondy [:bbondy]]

Information for testing
-----------------------

**Understanding the scope of this bug:**

There are a lot of parts to testing openh264, here's what should happen for this bug:
1) openh264 updates should be checked
2) if some updates are available, they should be downloaded
3) if some updates are downloaded, they should be unzipped in your profile.
4) A pref should be set indicating that the addon is unzipped and where you can find it.

There are other bugzilla bugs relating to getting openh264 to register and show up and work with Firefox completely.
But the testing for those bugs should happen outside of this bug.


**Which platforms should be tested**

At a minimum the tests should be run on Windows, OSX, and Linux.

**What's the best way to see if things are happening**

Open about:config and set:
media.gmp-manager.log -> true.

Also set these prefs:
browser.dom.window.dump.enabled -> true
javascript.options.showInConsole -> true

Logs will show up in the browser console (Tools -> Web Developer -> Browser Console) 1 minute after each Firefox startup.
You can see which logs are related because they will be prefixed with: GMPInstallManager


**When do new addon checks happen**

Exactly 1 minute after each browser startup happens, the pref: media.gmp-manager.lastCheck is checked.
If it doesn't exist, or is 1 day old, it'll do a new check for addons.

You should test 2 things here:
1) to make sure both the no pref exists check starts a check (Remember to go to about:config and reset media.gmp-manager.lastCheck)
2) that the 1 day old check works (remember that the check only happens on startups so you need to open browser after setting your clcok forward a day).

**How do I know if things got installed improperly**

If things got installed properly, you'll see that in the logs. Plus you'll see a new pref set:
media.{0}.path
media.{0}.lastUpdate
media.{0}.version

Where {0} is the openh264 pref name.
To get the full name just open about:config and type openh264 and the pref will show up there if it was set.


**If things aren't working**

Set the logging on, reproduce, and report to this bug.
The logs will be looked at and information will be given back, if it's a bug, or if things for this bug worked correctly.
If there is no log given, you'll just be told to please give me a log by following the steps in this comment.
Please **only** report a problem without a log, if the bug is about logging not working.


**For testing that new versions get installed**

You should also test that newer versions of the openh264 plugin get installed properly over top of the existing one.
Setup a local server:
cd some-new-directory
Create a file in this directory named updates.xml (You can see a similar file like this by looking at the real data served from the real AUS server in the logs).
Adjust that updates.xml file to have a new version number.
python -m SimpleHTTPServer 8011
Set media.gmp-manager.url.override to your local server's updates.xml path.
Reset your environment (reset media.gmp-manager.lastCheck (or set clock forward a day))
Test again.
Check that the media.{0}.version has the new version and that the new zip files get overwritten (You should put a different .zip served from your location so you can tell)
Once you're done remember to reset media.gmp-manager.url.override

**For testing other edge cases**

Things like server being down, no updates being served, ...
Follow the steps in the previous section, and adjust the updates.xml accordingly.
Make sure things work as expected in each case you can think of.

Comment 104

[Ada [:adalucinet]]

(In reply to Brian R. Bondy [:bbondy] from comment #103)
> Information for testing
> -----------------------
 
> **Which platforms should be tested**
> 
> At a minimum the tests should be run on Windows, OSX, and Linux.

Tested with latest Aurora 2014-07-27 on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 13.04 x64;

  
> **How do I know if things got installed improperly**
> 
> If things got installed properly, you'll see that in the logs. Plus you'll
> see a new pref set:
> media.{0}.path
> media.{0}.lastUpdate
> media.{0}.version

The logs are ok, please see https://pastebin.mozilla.org/5611863, and the prefs are correctly set:
media.gmp-gmpopenh264.path: /Users/andreivaida/Library/Application Support/Firefox/Profiles/17we692k.sd/gmp-gmpopenh264
media.gmp-gmpopenh264.lastUpdate: 1406280508
media.gmp-gmpopenh264.version: 1.0

Including after I reset media.gmp-manager.lastCheck or change the clock.

> **For testing that new versions get installed**
> 
> You should also test that newer versions of the openh264 plugin get
> installed properly over top of the existing one.
> Setup a local server:
> cd some-new-directory
> Create a file in this directory named updates.xml (You can see a similar
> file like this by looking at the real data served from the real AUS server
> in the logs).

My updates.xml file contains:
<updates>
   <addons>
        <addon id="gmp-gmpopenh264" URL="http://ciscobinary.openh264.org/openh264-macosx64-v1.1-Firefox33-b4504d4da483d83246633f7ad03b9f60400abaf6.zip" hashFunction="SHA512" hashValue="57c2b23c97e32d21ff9f6f611803cfdf6ba4a97f7b403c031e979a79c1db1f507d9b72749824254892032f119a4d057cec926fbc4efb184c4f543dfd34ad72a2" size="282436" version="2.0"/>
   </addons>
</updates>

Could you please confirm this is correct? Also downloaded the .zip file and changed the URL accordingly.

> Adjust that updates.xml file to have a new version number.
> python -m SimpleHTTPServer 8011
> Set media.gmp-manager.url.override to your local server's updates.xml path.

This is what I get after performing the above steps: https://pastebin.mozilla.org/5673125
I talked with :gfritzsche via IRC:

16:55 gfritzsche oh,you need to disable the builtin check
16:55 gfritzsche are you using the override url?
16:56 gfritzsche if you do then it should skip that check
16:56 gfritzsche media.gmp-manager.url.override
16:56 gfritzsche and don't use file/ urls
16:57 gfritzsche there might be odd/different behaviour there
16:58 gfritzsche if you use that local server on port 8011 it's ""http://localhost:8011/something""

And still couldn't make it work: "1406534723855 GMPInstallManager.parseResponseXML WARN got node name: parsererror, expected: updates" is displayed in terminal. I feel like I'm missing something here. Could you please give me some guidance?

Comment 105

[Georg Fritzsche [:gfritzsche]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #104)
> And still couldn't make it work: "1406534723855
> GMPInstallManager.parseResponseXML WARN got node name: parsererror,
> expected: updates" is displayed in terminal. I feel like I'm missing
> something here. Could you please give me some guidance?

This sounds like either the URL is incorrect (what have you set it to? http://localhost:8011/updates.xml ?) or the contents of the XML are incorrect.

Comment 106

[Brian R. Bondy [:bbondy]]

Sorry for the delay I was away on Monday.

----

For the first log given, it fetched for updates at:
https://aus4.mozilla.org/update/3/GMP/33.0a2/20140723004001/Darwin_x86_64-gcc3-u-i386-x86_64/en-US/aurora/Darwin%2013.3.0/default/default/update.xml

And the log shows that it installed successfully at:
/Users/andreivaida/Library/Application Support/Firefox/Profiles/17we692k.sd/gmp-gmpopenh264

-----

For the problem described after that, you gave a line of the log, but a full log will help diagnose what's being tested wrong. I think as Georg suggested it's just that the path is not filled in correctly or the xml file is wrong though.

Comment 107

[Ada [:adalucinet]]

Finally, I was able to generate installation of new plugin versions - e.g.:
- Logs before openh264 gets installed: https://pastebin.mozilla.org/5803243
- Logs after openh264 gets installed: https://pastebin.mozilla.org/5803172

But, now I'm stuck at "... and that the new zip files get overwritten (You should put a different .zip served from your location so you can tell)" from comment 103: I've put a different .zip file via the updates.xml file's URL, but the zip files don't get overwritten: https://pastebin.mozilla.org/5804178
Any idea why?

Comment 108

[Brian R. Bondy [:bbondy]]

Are you testing via addon manager? I'm seeing more than 1 install in the last 2 links you gave. I'm not sure how that can happen in a single log file unless you are combining 2 logs or using addon manager.

Comment 109

[Ada [:adalucinet]]

Solved the issue described in comment 107 (thanks Georg!).
Verified as fixed with Aurora 33.0a2 (Build ID: 20140807004002) on Windows 7 x64, Mac OS X 10.9.4 and Ubuntu 12.04 x32 with STR from comment 103 and some additional exploratory testing - no new issues encountered.

Comment 110

[silentscream4]

Hello guys 
I have this problem with the H264. I have pasted my log in paste bin and here is the address to view it

http://pastebin.com/cPAKzB4R

My add-on window in the Mozilla Firefox always shows the H264 plugin will be installed shortly message. When i try to update it then two files get downloaded. One is gmp-**.dll and another is a .info file. I don't know what to do with them.

Thanks

Comment 111

[David Teller [:Yoric] - still alive but not very active]

Silentscream4: if you are looking for technical support, you should rather go to http://support.mozilla.com/, where people who actually know answers would be able to help you better than us. If you are here because you have found a new bug, on the other hand, the best way to make sure that it's not lost inside another bug is to https://bugzilla.mozilla.org/enter_bug.cgi to open a new bug.

Comment 112

[hadivoice]

hey silentscream4 you should go to :
C:\Users\...\AppData\Roaming\Mozilla\Firefox\Profiles\
and create these folders :
9rv5ofs4.default\gmp-gmpopenh264\1.1
and then copy those tow files in here!