Failure to download extension causes about:addons to list the addon with no way to restart the download

VERIFIED FIXED in mozilla2.0b10



9 years ago
8 years ago


(Reporter: will.pittenger1+mozbugzilla, Assigned: Unfocused)


Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)


(Whiteboard: [strings][strings pushed on trunk][has patch])


(7 attachments, 4 obsolete attachments)

10.85 KB, image/png
745.99 KB, image/png
1.93 KB, patch
: review+
Details | Diff | Splinter Review
25.63 KB, patch
: review+
Details | Diff | Splinter Review
2.71 KB, patch
: review+
Details | Diff | Splinter Review
10.18 KB, patch
: review+
Details | Diff | Splinter Review
32.28 KB, patch
: review+
Details | Diff | Splinter Review


9 years ago
User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0b4) Gecko/20100818 Firefox/4.0b4

I was using about:addons to search through my installed addons when I saw that doing so also searches available addons.  Nice touch, but about:addons acted up when I attempted to install an extension which AMO failed to serve.

(At least 90% of the time, AMO will list an addon and let you view all the information, but actually download it?  No way.  It just times out.)

What happened was the box listing the addon became yellow.  No buttons were present.  I was hoping for one for Retry.  Cancel might might sense.  Or "Show me this in AMO".  Or a link to the homepage.  Instead, no buttons were visible at all.  There were absolutely no options.

Reproducible: Always

Steps to Reproduce:
1. Attempt to download an addon listed by AMO in the search window that AMO won't serve the actual addon
Actual Results:  
You will see a box similar to the attached image

Expected Results:  
One expects options for how to proceed.
Do you have an extension which always causing those problems for you or is it more a random behavior? Confirmed based on the screenshot.
Ever confirmed: true
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 3

9 years ago
I wouldn't call it either.  It simply doesn't matter which extension you pick.  If the download fails, you get what I showed.
Could you re-test this with a more recent build? There were a bunch of changes to the list view recently, and I can't remember if it handles errors like this better or not.

Comment 5

9 years ago
At this point, the betas the only version I can run.  So until Beta 5 comes out...
Ok, I have better and always reproducible steps now. We definitely have to improve that. Bug 591893 could help a bit by running the search again but keeping the install button on those failed entries would be really handy.

1. Perform a search, i.e. "feed"
2. Go into the offline mode
3. Click the install button of a search entry.
blocking2.0: --- → ?
Keywords: uiwanted
blocking2.0: ? → final+
Depends on: 591893
Posted patch WiP v1 (obsolete) — Splinter Review
Work in progress. It works, but has no tests, and I'm not 100% sure of all the side-effects of doing it this way.
Assignee: nobody → bmcbride
Attachment #475364 - Flags: feedback?(dtownsend)
Also: This adds new strings. Very tempted to just get in the UI part, and wait for the API part (if it ends up requiring too much work).

On the upside, fixing the API part for this should also fix bug 553557.
blocking2.0: final+ → ?
Presumably, there are many ways for a particular download to fail.  It would be great to give a specific description and a way to fix it for each case.  For now, we need a broad enough case to handle any kind of download failure. 

Since this is a warning related to a particular add-on, it should be displayed in the same style that all add-on specific errors are: a slight, striped coloring with a message at the top and, in this case, the ability to retry the download.
Posted patch WiP v2 (obsolete) — Splinter Review
Updated to fit the mockup. Ended up rewriting half the addon install binding in the process.
Attachment #475364 - Attachment is obsolete: true
Attachment #475504 - Flags: feedback?(dtownsend)
Attachment #475364 - Flags: feedback?(dtownsend)
Comment on attachment 475504 [details] [diff] [review]
WiP v2

Looks to be along the right lines to me.

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -4178,16 +4178,17 @@ function AddonInstall(aCallback, aInstal
> }
> AddonInstall.prototype = {
>   installLocation: null,
>   wrapper: null,
>   stream: null,
>   crypto: null,
>   hash: null,
>+  hashAlg: null,
>   loadGroup: null,
>   badCertHandler: null,
>   listeners: null,
>   name: null,
>   type: null,
>   version: null,
>   iconURL: null,
>@@ -4216,16 +4217,26 @@ AddonInstall.prototype = {
>   install: function AI_install() {
>     switch (this.state) {
>     case AddonManager.STATE_AVAILABLE:
>       this.startDownload();
>       break;
>     case AddonManager.STATE_DOWNLOADED:
>       this.startInstall();
>       break;
>+    case AddonManager.STATE_DOWNLOAD_FAILED:
>+    case AddonManager.STATE_INSTALL_FAILED:
>+    case AddonManager.STATE_CANCELLED:
>+      this.state = AddonManager.STATE_AVAILABLE;
>+      this.error = 0;
>+      this.progress = 0;
>+      this.maxProgress = -1;
>+      XPIProvider.installs.push(this);
>+      this.startDownload();
>+      break;
>     case AddonManager.STATE_DOWNLOADING:
>     case AddonManager.STATE_CHECKING:
>     case AddonManager.STATE_INSTALLING:
>       // Installation is already running
>       return;
>     default:
>       throw new Error("Cannot start installing from this state");
>     }
>@@ -4622,23 +4633,24 @@ AddonInstall.prototype = {
>    * This is the first chance to get at real headers on the channel.
>    *
>    * @see nsIStreamListener
>    */
>   onStartRequest: function AI_onStartRequest(aRequest, aContext) {
>     this.crypto = Cc[";1"].
>                   createInstance(Ci.nsICryptoHash);
>     if (this.hash) {
>-      [alg, this.hash] = this.hash.split(":", 2);
>+      if (!this.hashAlg)
>+        [this.hashAlg, this.hash] = this.hash.split(":", 2);

One concern here is that if we are restarting the download then we want to throw away any hash that was provided by the HTTPS header, but not ay hash provided by InstallTrigger.install. That probably means stashing the latter somewhere safe.
Attachment #475504 - Flags: feedback?(dtownsend) → feedback+
Whiteboard: [strings]
blocking2.0: ? → beta7+
If we're pre-landing strings, please add a localization note that points to a mockup so that the localization community knows what they're doing.
Having a bunch of trouble making any progress on understanding test failures and fixing them (and writing new API tests).

So here's a strings-only patch.
Attachment #475737 - Flags: review?(dtownsend)
Keywords: uiwanted
Attachment #475737 - Flags: review?(dtownsend) → review+
Comment on attachment 475737 [details] [diff] [review]
Strings-only [checked in]

Landed the strings:
Attachment #475737 - Attachment description: Strings-only → Strings-only [checked in]
Dave: do you think this still blocks b7 now that the strings are landed?
blocking2.0: beta7+ → betaN+
Whiteboard: [strings] → [strings][strings landed]


9 years ago
Whiteboard: [strings][strings landed] → [strings][strings pushed on trunk]
As talked with Blair on IRC we should remove the orphaned restart-install button, which isn't needed anymore:^[^\0]*%24&hitlimit=&tree=mozilla-central
The tests here are failing because of bug 599509
Depends on: 599509
Posted patch backend patch rev 1 (obsolete) — Splinter Review
Pulled the backend parts out here, added some fixes and test so it can unblock some other bugs. This makes restarting failed or cancelled installs possible and verifies that it works, also fixes and tests bug 602332 as I was messing around with the hashes at the same time.
Attachment #487075 - Flags: review?(robert.bugzilla)
Comment on attachment 487075 [details] [diff] [review]
backend patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -4440,17 +4450,24 @@ var XPIDatabase = {
>  */
> function AddonInstall(aCallback, aInstallLocation, aUrl, aHash, aName, aType,
>                       aIconURL, aVersion, aReleaseNotesURI, aExistingAddon,
>                       aLoadGroup) {
>   this.wrapper = new AddonInstallWrapper(this);
>   this.installLocation = aInstallLocation;
>   this.sourceURI = aUrl;
>   this.releaseNotesURI = aReleaseNotesURI;
>-  this.hash = aHash;
>+  if (aHash) {
>+    let hashSplit = aHash.split(":");
perhaps just make it all lowercase here aHash.toLowerCase().split(":");

>@@ -4469,23 +4486,34 @@ function AddonInstall(aCallback, aInstal
>     this.state = AddonManager.STATE_DOWNLOADED;
>     this.progress = this.file.fileSize;
>     this.maxProgress = this.file.fileSize;
>     if (this.hash) {
>       let crypto = Cc[";1"].
>                    createInstance(Ci.nsICryptoHash);
>+      try {
>+        crypto.initWithString(this.hash.algorithm);
>+      }
>+      catch (e) {
>+        WARN("Unknown hash algorithm " + this.hash.algorithm);
>+        this.state = AddonManager.STATE_DOWNLOAD_FAILED;
>+        this.error = AddonManager.ERROR_INCORRECT_HASH;
>+        aCallback(this);
>+        return;
>+      }
>       let fis = Cc[";1"].
>                 createInstance(Ci.nsIFileInputStream);
>       fis.init(this.file, -1, -1, false);
>       crypto.updateFromStream(fis, this.file.fileSize);
>-      let hash = crypto.finish(true);
>-      if (hash != this.hash) {
>-        WARN("Hash mismatch");
>+      if (getHashStringForCrypto(crypto) != {
>+        WARN("File hash (" + hash + ") did not match provided hash (" +
>+    + ")");
You no longer declare hash :/
Comment on attachment 487075 [details] [diff] [review]
backend patch rev 1

>diff --git a/toolkit/mozapps/extensions/amWebInstallListener.js b/toolkit/mozapps/extensions/amWebInstallListener.js
>--- a/toolkit/mozapps/extensions/amWebInstallListener.js
>+++ b/toolkit/mozapps/extensions/amWebInstallListener.js
>@@ -47,16 +47,24 @@
> const Cc = Components.classes;
> const Ci = Components.interfaces;
> const Cr = Components.results;
> Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> Components.utils.import("resource://gre/modules/AddonManager.jsm");
> Components.utils.import("resource://gre/modules/Services.jsm");
>+// The states that are ready to start installing
badly worded... perhaps?
The states available when an add-on is ready to start installin

>+const READY_STATES = [
>+  AddonManager.STATE_AVAILABLE,
>+  AddonManager.STATE_CANCELLED

I'd like to take another quick look at this one
Attachment #487075 - Flags: review?(robert.bugzilla) → review-
Attachment #487075 - Attachment is obsolete: true
Attachment #488538 - Flags: review?(robert.bugzilla)
Landed the backend piece, frontend still needs to be finished up.
Last Resolved: 9 years ago
Resolution: --- → FIXED
Backed out due to test failures.
Resolution: FIXED → ---
Not sure why I didn't see this before but this fixes the bustage by correcting the test harness. Not all tests end up going through endTest so instead only add the window listener once and remove it in the cleanup function. Also moved the check for remaining downloads to the cleanup function and made it log and cancel each one to stop failures in later tests.
Attachment #488925 - Flags: review?(robert.bugzilla)
Comment on attachment 488925 [details] [diff] [review]
test harness fixes

The following is outside the scope of this bug

I'm concerned about getAllInstalls being sync while stating it is async. Please file a bug to sort that out.

Restartless add-ons that are installed by tests aren't cleaned up on test time out, etc. Would be a good thing to file a bug for that as well.
Attachment #488925 - Flags: review?(robert.bugzilla) → review+
(In reply to comment #22)
> Landed the backend piece, frontend still needs to be finished up.

Dave, how close are we to put the pre-landed strings into action here?
(In reply to comment #27)
> (In reply to comment #22)
> > Landed the backend piece, frontend still needs to be finished up.
> Dave, how close are we to put the pre-landed strings into action here?

Not sure, Blair is on point for the frontend parts here and he's been tied up with bug 601022 for a while now.
Posted patch Frontend Patch v1 (obsolete) — Splinter Review
Stuck trying to hunt down weird test failures, which have sucked up a lot of time :( I think it's probably something small I'm missing, and just need a different pair of eyes on it - Dave, would you mind having a look?
Attachment #475504 - Attachment is obsolete: true
Attachment #501566 - Flags: feedback?(dtownsend)
Your patch is revealing a small issue with retrieving resources for bootstrapped add-ons that have been recently installed.

Basically when the add-on to install is downloaded we create an Addon instance to represent it. The info is loaded straight out of the install.rdf and keeps a reference to the local XPI. hasResource/getResource both look into that XPI.

For bootstrapped add-ons when install completes their XPI gets copied/extracted to the profile and then deleted and a new Addon instance is created from the new info in the database. hasResource/getResource both look at the new XPI/directory.

However if any callers have kept a reference to the original Addon instance they will find that hasResource/getResource will throw an exception because the XPI they are looking for has been deleted. This patch fixes that by updating the _sourceBundle property on the original instance. It also stops us recreating the _sourceBundle reference everytime and instead just sets it when the info is first pulled from the database. Spinning this through the tryserver now to verify it.

Blair this was causing problems for your patch because now in onInstallEnded you are calling refreshInfo on the add-on UI which still had a reference to the original Addon instance and was trying to get the icon from it. This patch will make that not throw but I think you also want to make sure that the UI gets updated with a reference to the new Addon instance otherwise it will not be able to disable/enable the add-on properly I think (it's possible it actually ends up getting the new instance through a different path, I didn't look too hard yet).
Comment on attachment 501851 [details] [diff] [review]
Fix broken hasResource/getResource on cached installed add-ons

This passed the try runs, comment 30 explains what is going on here and what this patch solves.
Attachment #501851 - Flags: review?(robert.bugzilla)
Comment on attachment 501566 [details] [diff] [review]
Frontend Patch v1

Gave some notes previously, this is looking fine just check on whether we're correctly using the new Addon instance after install.
Attachment #501566 - Flags: feedback?(dtownsend) → feedback+
Only real difference is that its been rebased. 

I double checked, and it does already use the new Addon instance in the main addon bindings.
Attachment #501566 - Attachment is obsolete: true
Attachment #502419 - Flags: review?(dtownsend)
Whiteboard: [strings][strings pushed on trunk] → [strings][strings pushed on trunk][has patch][needs review Mossop]
Attachment #502419 - Flags: review?(dtownsend) → review+
Whiteboard: [strings][strings pushed on trunk][has patch][needs review Mossop] → [strings][strings pushed on trunk][has patch]
Keywords: checkin-needed
Whiteboard: [strings][strings pushed on trunk][has patch] → [strings][strings pushed on trunk][has patch][needs landing]
Whiteboard: [strings][strings pushed on trunk][has patch][needs landing] → [strings][strings pushed on trunk][has patch][needs landing][needs rebasing]
Whiteboard: [strings][strings pushed on trunk][has patch][needs landing][needs rebasing] → [strings][strings pushed on trunk][has patch][needs landing]
Keywords: checkin-needed
Whiteboard: [strings][strings pushed on trunk][has patch][needs landing] → [strings][strings pushed on trunk][has patch]
correction for comment #34, checked in frontend patch, not backend (which was already landed).
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
Dave or Blair, is any manual test necessary?
Flags: in-testsuite+
Flags: in-litmus?
Target Milestone: --- → mozilla2.0b10
(In reply to comment #36)
> Dave or Blair, is any manual test necessary?

Think the automated test is enough here.
Flags: in-litmus? → in-litmus-
Except bug 626373 we should be fine here now. Users now have the chance to re-try a failed download. Marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110114 Firefox/4.0b10pre ID:20110114030359
You need to log in before you can comment on or make changes to this bug.