Closed Bug 1264173 Opened 4 years ago Closed 4 years ago

ProductAddonChecker can't download from servers that don't serve Content-Length headers

Categories

(Firefox :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 4 obsolete files)

The ProductAddonChecker uses nsIIncrementalDownload, which can't handle downloading from web servers that don't serve Content-Length headers.

The GMPInstallManager uses the ProductAddonChecker to download. The Widevine Content Decryption Module is downloaded via the GMPInstallManager, and Google's update servers don't serve Content-Length headers.

So we need to make ProductAddonChecker able to download files from servers which aren't serving Content-Lenght headers.

The easiest way to do that is to change ProductAddonChecker to use XHR to download.

Note: This bug is blocking deployment of Widevine EME, which we're planning to uplift to Firefox 47.
Some addons (namely the Google Widevine EME Content Decryption Module) are
served from web servers that don't send Content-Length headers, and
nsIIncrementalDownload can't handle such downloads.

MozReview-Commit-ID: 3lBvl1fZwIS
Attachment #8740777 - Flags: review?(spohl.mozilla.bugs)
Comment on attachment 8740777 [details] [diff] [review]
Change ProductAddonChecker to download with XHR instead of nsIIncrementalDownload

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

This looks good to me and given the urgency, I'll r+ assuming try comes back green. Setting f? for Georg in case he has any comments about this change.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
@@ +185,5 @@
> +    let xhr = new XMLHttpRequest();
> +    xhr.onload = function(response) {
> +      logger.info("downloadXHR File download. status=" + xhr.status);
> +      if (xhr.status != 200 && xhr.status != 206) {
> +        reject("Failed to download");

Is there a particular reason why you don't reject with a |Components.Exception| here anymore?

@@ +191,5 @@
>        }
> +      Task.spawn(function* () {
> +        let f = yield OS.File.openUnique(OS.Path.join(OS.Constants.Path.tmpDir, "tmpaddon"));
> +        let path = f.path;
> +        yield f.file.close(); 

nit: trailing whitespace
Attachment #8740777 - Flags: review?(spohl.mozilla.bugs)
Attachment #8740777 - Flags: review+
Attachment #8740777 - Flags: feedback?(gfritzsche)
(In reply to Stephen A Pohl [:spohl] from comment #3)
> Comment on attachment 8740777 [details] [diff] [review]
> Change ProductAddonChecker to download with XHR instead of
> nsIIncrementalDownload
> 
> Review of attachment 8740777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good to me and given the urgency, I'll r+ assuming try comes back
> green. Setting f? for Georg in case he has any comments about this change.
> 
> ::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
> @@ +185,5 @@
> > +    let xhr = new XMLHttpRequest();
> > +    xhr.onload = function(response) {
> > +      logger.info("downloadXHR File download. status=" + xhr.status);
> > +      if (xhr.status != 200 && xhr.status != 206) {
> > +        reject("Failed to download");
> 
> Is there a particular reason why you don't reject with a
> |Components.Exception| here anymore?

Good catch!

Thanks for the quick review.
Gah! Test failures caused by test_GMPInstallManager.js overriding XHR requests, and the downloader now uses XHR to download GMPs, so the download is failing.
This ensures that test_GMPInstallManager can change the XHR response for the
update.xml requests without affecting the XHR requests to download the test
GMP.

Also add more error handlers on the addon download XHR.

MozReview-Commit-ID: 5IWRUm3dHcW
We can fix the test failures by doing the same trick used in the GMPProvider test to create a mock GMPService; we can add a helper function used by the code to download the XML document with the list of addons to creates its XmlHTTPRequest, and then in the test we can change that help helper function to give the behaviour we need.
Comment on attachment 8740777 [details] [diff] [review]
Change ProductAddonChecker to download with XHR instead of nsIIncrementalDownload

Dave: Can you look at this and the next patch in the series? We need this fix to make the Widevine GMP downloader work, but I'm unsure as to whether we need the cache control headers and the BadCertHandler (as used in the other XHR in ProductAddonChecker.jsm).

We want to land this ASAP, so that Amazon can start testing Widevine support.
Attachment #8740777 - Flags: feedback?(dtownsend)
Comment on attachment 8740818 [details] [diff] [review]
Patch 2- Change ProductAddonChecker to create XHRs used to download XML through a layer of indirection. r?

Mossop: Can you check this? Adds more error checking to new XHR in ProductAddonChecker.jsm, and makes GMP install manager tests pass.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad045ef3a36e
Attachment #8740818 - Attachment description: Change ProductAddonChecker to create XHRs used to download XML through a layer of indirection. r? → Patch 2- Change ProductAddonChecker to create XHRs used to download XML through a layer of indirection. r?
Attachment #8740818 - Flags: review?(dtownsend)
Comment on attachment 8740777 [details] [diff] [review]
Change ProductAddonChecker to download with XHR instead of nsIIncrementalDownload

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

I see that this is used from the AddonManager, which has a lot more potential edge cases than just the GMP downloads.
Mossop, i think it's better if you take a look in this case.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
@@ +184,5 @@
>    return new Promise((resolve, reject) => {
> +    let xhr = new XMLHttpRequest();
> +    xhr.onload = function(response) {
> +      logger.info("downloadXHR File download. status=" + xhr.status);
> +      if (xhr.status != 200 && xhr.status != 206) {

We should log some more details for failure scenarios here and below.

@@ +192,5 @@
> +      Task.spawn(function* () {
> +        let f = yield OS.File.openUnique(OS.Path.join(OS.Constants.Path.tmpDir, "tmpaddon"));
> +        let path = f.path;
> +        yield f.file.close(); 
> +        yield OS.File.writeAtomic(path, new Uint8Array(xhr.response));

Do we know how big those files are? This could have quite some memory impact.

@@ +199,2 @@
>      };
> +    xhr.onerror = function(ex) {

Also onabort and ontimeout:
https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/experiments/Experiments.jsm#961

@@ -203,5 @@
> -    let tmpFile = FileUtils.getFile("TmpD", ["tmpaddon"]);
> -    tmpFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> -
> -    logger.info("Downloading from " + uri.spec + " to " + tmpFile.path);
> -    request.init(uri, tmpFile, DOWNLOAD_CHUNK_BYTES_SIZE, DOWNLOAD_INTERVAL);

Could we have any impact on bandwidth with moving away from chunked downloads?
Attachment #8740777 - Flags: feedback?(gfritzsche) → feedback?(dtownsend)
(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> Comment on attachment 8740777 [details] [diff] [review]
> Change ProductAddonChecker to download with XHR instead of
> nsIIncrementalDownload
> 
> Review of attachment 8740777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I see that this is used from the AddonManager, which has a lot more
> potential edge cases than just the GMP downloads.
> Mossop, i think it's better if you take a look in this case.
> 
> ::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
> @@ +184,5 @@
> >    return new Promise((resolve, reject) => {
> > +    let xhr = new XMLHttpRequest();
> > +    xhr.onload = function(response) {
> > +      logger.info("downloadXHR File download. status=" + xhr.status);
> > +      if (xhr.status != 200 && xhr.status != 206) {
> 
> We should log some more details for failure scenarios here and below.
> 
> @@ +192,5 @@
> > +      Task.spawn(function* () {
> > +        let f = yield OS.File.openUnique(OS.Path.join(OS.Constants.Path.tmpDir, "tmpaddon"));
> > +        let path = f.path;
> > +        yield f.file.close(); 
> > +        yield OS.File.writeAtomic(path, new Uint8Array(xhr.response));
> 
> Do we know how big those files are? This could have quite some memory impact.


GMP downloads are between 3 and 5 MB. I don't know how big other addons are.


> 
> @@ +199,2 @@
> >      };
> > +    xhr.onerror = function(ex) {
> 
> Also onabort and ontimeout:
> https://dxr.mozilla.org/mozilla-central/rev/
> 21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/experiments/Experiments.
> jsm#961
> 
> @@ -203,5 @@
> > -    let tmpFile = FileUtils.getFile("TmpD", ["tmpaddon"]);
> > -    tmpFile.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> > -
> > -    logger.info("Downloading from " + uri.spec + " to " + tmpFile.path);
> > -    request.init(uri, tmpFile, DOWNLOAD_CHUNK_BYTES_SIZE, DOWNLOAD_INTERVAL);
> 
> Could we have any impact on bandwidth with moving away from chunked
> downloads?

Note, the existing code using nsIIncrementalDownloader had a chunk timeout of 0, so we'd effectively be downloading as fast as possible with the old code too.
Comment on attachment 8740777 [details] [diff] [review]
Change ProductAddonChecker to download with XHR instead of nsIIncrementalDownload

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

Tentative r=me - I do have some questions I'd like answered though - see below.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
@@ +181,5 @@
>   *         with a JS exception in case of error.
>   */
>  function downloadFile(url) {
>    return new Promise((resolve, reject) => {
> +    let xhr = new XMLHttpRequest();

I'm actually surprised this works. Where is XMLHttpRequest coming from? Traditionally speaking, some import-fu is usually required to make it available in a JSM - example:

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/modules/DirectoryLinksProvider.jsm#14

or constructing the XPCOM variant:

https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/extensions/pdfjs/content/PdfStreamConverter.jsm#607

So where is this coming from? Are we certain this works?

@@ +185,5 @@
> +    let xhr = new XMLHttpRequest();
> +    xhr.onload = function(response) {
> +      logger.info("downloadXHR File download. status=" + xhr.status);
> +      if (xhr.status != 200 && xhr.status != 206) {
> +        reject("Failed to download");

I concur with spohl - rejecting with an Error or Components.Exception is certainly better than rejecting with a string.

@@ +200,5 @@
> +    xhr.onerror = function(ex) {
> +      reject(ex);
> +    }
> +    xhr.responseType = "arraybuffer";
> +    xhr.timeout = 10000;

Are we certain a 10s download is realistic for all connection qualities? Where is this magic number coming from?

@@ +202,5 @@
> +    }
> +    xhr.responseType = "arraybuffer";
> +    xhr.timeout = 10000;
> +    try {
> +      xhr.open("GET", url, true);

Third argument about being async, is optional and defaults to true, and can be dropped safely.
Attachment #8740777 - Flags: review+
Comment on attachment 8740818 [details] [diff] [review]
Patch 2- Change ProductAddonChecker to create XHRs used to download XML through a layer of indirection. r?

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

LGTM.

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
@@ +215,3 @@
>        reject(ex);
> +    };
> +    xhr.addEventListener("error", fail, false);

Third argument "capturing", defaults to false on addEventListener, and is optional, so is unnecessary.
Attachment #8740818 - Flags: review?(dtownsend) → review+
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #13)
> Comment on attachment 8740777 [details] [diff] [review]
> Change ProductAddonChecker to download with XHR instead of
> nsIIncrementalDownload
> 
> Review of attachment 8740777 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Tentative r=me - I do have some questions I'd like answered though - see
> below.
> 
> ::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
> @@ +181,5 @@
> >   *         with a JS exception in case of error.
> >   */
> >  function downloadFile(url) {
> >    return new Promise((resolve, reject) => {
> > +    let xhr = new XMLHttpRequest();
> 
> I'm actually surprised this works. Where is XMLHttpRequest coming from?
> Traditionally speaking, some import-fu is usually required to make it
> available in a JSM - example:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/modules/
> DirectoryLinksProvider.jsm#14
> 
> or constructing the XPCOM variant:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/browser/extensions/pdfjs/content/
> PdfStreamConverter.jsm#607
> 
> So where is this coming from? Are we certain this works?
> 

Note that this is fixed in Patch 2.
I believe all of the issues mentioned have been addressed in this patch except one: I still do not know why the 10 second timeout. Since this is urgent, however, I am uploading the patch anyways.

It is still running through try (see above).
Attachment #8740777 - Attachment is obsolete: true
Attachment #8740818 - Attachment is obsolete: true
Attachment #8740777 - Flags: feedback?(dtownsend)
Attachment #8741119 - Flags: review?(spohl.mozilla.bugs)
Attachment #8741119 - Flags: review?(mconley)
Comment on attachment 8741119 [details] [diff] [review]
Patch: Change ProductAddonChecker to download with XHR

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

Alrighty, let's put the spaghetti in the machine, as they say.
Attachment #8741119 - Flags: review?(mconley) → review+
Comment on attachment 8741119 [details] [diff] [review]
Patch: Change ProductAddonChecker to download with XHR

I defer to mconley's review.
Attachment #8741119 - Flags: review?(spohl.mozilla.bugs)
¯\_(ツ)_/¯

Here goes!
Timeout removed, as discussed.
Attachment #8741119 - Attachment is obsolete: true
Attachment #8741140 - Flags: review?(mconley)
Comment on attachment 8741140 [details] [diff] [review]
Patch: Change ProductAddonChecker to download with XHR

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

::: toolkit/mozapps/extensions/internal/ProductAddonChecker.jsm
@@ +216,5 @@
> +      reject(ex);
> +    };
> +    xhr.addEventListener("error", fail);
> +    xhr.addEventListener("abort", fail);
> +    xhr.addEventListener("timeout", fail);

You can remove this event listener now, since timing out isn't possible anymore.
Attachment #8741140 - Flags: review?(mconley) → review+
Attachment #8741145 - Flags: review?(mconley) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e45dc599a04eb99f8c8e3f3156878b7e258ccee
Bug 1264173 - Change ProductAddonChecker to download with XHR instead of nsIIncrementalDownload. r=mconley,spohl
https://hg.mozilla.org/mozilla-central/rev/0e45dc599a04
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Comment on attachment 8741145 [details] [diff] [review]
Patch: Change ProductAddonChecker to download with XHR

Requesting uplift for Firefox 47.

Approval Request Comment
[Feature/regressing bug #]: Widevine EME support
[User impact if declined]: No Widevine CDM downloaded, so no Widevine DRM video playback.
[Describe test coverage new/current, TreeHerder]: We have test coverage of the GMP downloader code.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8741145 - Flags: approval-mozilla-aurora?
Comment on attachment 8741145 [details] [diff] [review]
Patch: Change ProductAddonChecker to download with XHR

Widevine related uplifts were pre-approved.
Attachment #8741145 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.