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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Will Pittenger]

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
2.
3.
Actual Results:  
You will see a box similar to the attached image

Expected Results:  
One expects options for how to proceed.

Comment 1

[Will Pittenger]

Attachment: This is what you see when you follow the directions I gave

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Do you have an extension which always causing those problems for you or is it more a random behavior? Confirmed based on the screenshot.

Comment 3

[Will Pittenger]

I wouldn't call it either.  It simply doesn't matter which extension you pick.  If the download fails, you get what I showed.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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

[Will Pittenger]

At this point, the betas the only version I can run.  So until Beta 5 comes out...

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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.

Steps:
1. Perform a search, i.e. "feed"
2. Go into the offline mode
3. Click the install button of a search entry.

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v1

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.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 9

[Jennifer Morrow [:Boriss] (UX)]

Attachment: Mockups: Current error handling, proposed error handling styilng

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.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: WiP v2

Updated to fit the mockup. Ended up rewriting half the addon install binding in the process.

Comment 11

[Dave Townsend [:mossop]]

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["@mozilla.org/security/hash;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.

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Strings-only [checked in]

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.

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 475737 [details] [diff] [review]
Strings-only [checked in]

Landed the strings: http://hg.mozilla.org/mozilla-central/rev/bbd9ee911c09

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

Dave: do you think this still blocks b7 now that the strings are landed?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As talked with Blair on IRC we should remove the orphaned restart-install button, which isn't needed anymore:

http://mxr.mozilla.org/mozilla-central/search?string=restart-install&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 17

[Dave Townsend [:mossop]]

The tests here are failing because of bug 599509

Comment 18

[Dave Townsend [:mossop]]

Attachment: backend patch rev 1

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.

Comment 19

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

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["@mozilla.org/security/hash;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["@mozilla.org/network/file-input-stream;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) != this.hash.data) {
>+        WARN("File hash (" + hash + ") did not match provided hash (" +
>+             this.hash.data + ")");
You no longer declare hash :/

Comment 20

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

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_DOWNLOAD_FAILED,
>+  AddonManager.STATE_INSTALL_FAILED,
>+  AddonManager.STATE_CANCELLED
>+];
>+

I'd like to take another quick look at this one

Comment 21

[Dave Townsend [:mossop]]

Attachment: backend patch rev 2

Comment 22

[Dave Townsend [:mossop]]

Landed the backend piece, frontend still needs to be finished up.

http://hg.mozilla.org/mozilla-central/rev/694dbf0d5ad1

Comment 23

[Dave Townsend [:mossop]]

Backed out due to test failures.

Comment 24

[Dave Townsend [:mossop]]

Attachment: test harness fixes

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.

Comment 25

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

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.
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#575

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.

Comment 26

[Dave Townsend [:mossop]]

Landed the backend again: http://hg.mozilla.org/mozilla-central/rev/8b02a41fc140

Comment 27

[Axel Hecht [:Pike]]

(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?

Comment 28

[Dave Townsend [:mossop]]

(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.

Comment 29

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Frontend Patch v1

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?

Comment 30

[Dave Townsend [:mossop]]

Attachment: Fix broken hasResource/getResource on cached installed add-ons

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 31

[Dave Townsend [:mossop]]

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.

Comment 32

[Dave Townsend [:mossop]]

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.

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Attachment: Frontend Patch v1.1

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.

Comment 34

[Rob Campbell [:rc] (:robcee)]

landed fix for hasResource and backend patch:

http://hg.mozilla.org/mozilla-central/rev/d414fbea7ba3
http://hg.mozilla.org/mozilla-central/rev/caac4406a720

Comment 35

[Rob Campbell [:rc] (:robcee)]

correction for comment #34, checked in frontend patch, not backend (which was already landed).

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dave or Blair, is any manual test necessary?

Comment 37

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to comment #36)
> Dave or Blair, is any manual test necessary?

Think the automated test is enough here.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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