Closed
Bug 1264173
Opened 8 years ago
Closed 8 years ago
ProductAddonChecker can't download from servers that don't serve Content-Length headers
Categories
(Firefox :: General, defect, P1)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 47
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 4 obsolete files)
8.51 KB,
patch
|
mconley
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•8 years ago
|
||
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)
Assignee | ||
Comment 2•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a01252d188de
Comment 3•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4749c0eb715
Assignee | ||
Comment 8•8 years ago
|
||
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.
Assignee | ||
Comment 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
(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 13•8 years ago
|
||
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 14•8 years ago
|
||
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+
Comment 15•8 years ago
|
||
(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.
Comment 17•8 years ago
|
||
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 18•8 years ago
|
||
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 19•8 years ago
|
||
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)
Comment 20•8 years ago
|
||
¯\_(ツ)_/¯ Here goes!
Comment 21•8 years ago
|
||
Timeout removed, as discussed.
Attachment #8741119 -
Attachment is obsolete: true
Attachment #8741140 -
Flags: review?(mconley)
Comment 22•8 years ago
|
||
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+
Comment 23•8 years ago
|
||
Attachment #8741140 -
Attachment is obsolete: true
Attachment #8741145 -
Flags: review?(mconley)
Updated•8 years ago
|
Attachment #8741145 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 24•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e45dc599a04eb99f8c8e3f3156878b7e258ccee Bug 1264173 - Change ProductAddonChecker to download with XHR instead of nsIIncrementalDownload. r=mconley,spohl
Comment 25•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0e45dc599a04
Assignee | ||
Updated•8 years ago
|
Blocks: widevine-uplift
Assignee | ||
Comment 26•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8485c5d3d090
status-firefox47:
--- → fixed
Assignee | ||
Comment 27•8 years ago
|
||
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.
Description
•