558134 - Canceling add-on installation leaves behind unwanted (multiple) entries in the extensions list

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Attachment: screenshot

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a4pre) Gecko/20100407 Minefield/3.7a4pre (.NET CLR 3.5.30729)

Canceling the installation of an add-on leaves entries in the extensions pane behind. The same happens when you install the same add-on multiple times.

Steps:
1. Go to https://addons.mozilla.org/en-US/firefox/addon/6622
2. Install the extension but cancel the dialog
3. Install the extension

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

http://hg.mozilla.org/projects/addonsmgr/rev/9678190bc1f8

Comment 3

[Dave Townsend [:mossop]]

Landed on trunk as a part of bug 554007

Comment 4

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

It's still there with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100514 Minefield/3.7a5pre

Just try to install that extension and cancel the installation dialog:
https://addons.mozilla.org/en-US/firefox/addon/26/

Comment 5

[Dave Townsend [:mossop]]

Blair, this is basically what we discussed about hiding STATE_AVAILABLE entries from the list. Ben, did you say you were likely to do this as part of something else anyway?

Comment 6

[Ben Parr [:bparr]]

Hiding non-active entries (state = STATE_AVAILABLE) has mostly been done. When a list is shown, it will not have any non-active entries (part of patch for Bug 572561). However, non-active entries can still appear because the List View has a onNewInstall listener that adds items. So, Henrik's steps do show a non-active install. Reloading the list will hide the non-active install. So, to maintain consistency, we should probably remove the onNewInstall callback, and add onDownloadStarted and onInstallStarted callbacks, or do something similar.

I am able to reproduce the bug using the first link Henrik gave (DOM Inspector), but not the second link (Download Statusbar). The differentiating factor, I think, is that I have DOM inspector already installed, but not Download Statusbar. It seems that the onDownloadCancelled nor onInstallCancelled is fired if an install is canceled for an add-on that is already installed. Note that this was just a quick assessment, and could be wrong.

Comment 7

[Dave Townsend [:mossop]]

Sounds like you know more about what is going on here than anyone else, so it's yours to own ;)

Comment 8

[Dave Townsend [:mossop]]

The problem here is that once gEventManager sees that an install event is for an install of an existing add-on it only sends notifications to listeners registered for that add-on which leaves the install entry out in the cold.

Comment 9

[Dave Townsend [:mossop]]

Attachment: patch rev 1

This relies on the patch from bug 597620 as it builds on top of the test there.

There are a couple of changes here.

The API only sends out onOperationCompleted after it has removed the pendingUpgrade property from the add-on, this allows undoing an upgrade to work in the UI.

All install events are sent out to all install listeners regardless of whether they are for upgrades too. This allos the list view to see that an install has become an upgrade and so to remove the installing entry.

Also a minor fix to stop trying to send onInstallCompleted when the install status is attached to an installed add-on which does not have that method.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 478547 [details] [diff] [review]
patch rev 1

(In reply to comment #9)
> All install events are sent out to all install listeners regardless of whether
> they are for upgrades too. This allos the list view to see that an install has
> become an upgrade and so to remove the installing entry.

Most install listeners only expect events for new installs (they don't filter out upgrades because they assume the event manager did that). So this ends up creating new items in the list view for upgrades.

> Also a minor fix to stop trying to send onInstallCompleted when the install
> status is attached to an installed add-on which does not have that method.

This fixes bug 565562.

Comment 11

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Corrects and tests for that problem. I can't really see an alternative to startingto send all install events out to all install listeners short of having the listview register for addon events for all in the list, but that wouldn't really help any anyway.

Comment 12

[Dave Townsend [:mossop]]

Fixed: http://hg.mozilla.org/mozilla-central/rev/5ba23deb1856

Comment 14

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

Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b8pre) Gecko/20101012 Firefox/4.0b8pre. That only covers the steps from comment 0 with cancel involved. Bug 562922 which covers the multiple installation of the same extension is still valid.