553022 - AddonInstall instances are being held onto

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

AddonInstall instances are being held onto, after they are canceled - and get returned by AddonManager.getInstalls()

Comment 1

[Dave Townsend [:mossop]]

I mulled over this a bit. Do you agree that AddonInstalls should no longer be returned by getInstalls after they have either failed or cancelled, successful and pending installs should stay permanently?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Thinking about this more, there are cases where keeping them around might actually be a good idea (regardless of their state). See also bug 553499.

Instead, could we have an API that needs to be explicitly called to forget about an AddonInstall. eg: AddonInstall.purge()

Comment 3

[Dave Townsend [:mossop]]

This is causing tests to fail so we need to do /something/ here.

Comment 4

[Dave Townsend [:mossop]]

http://hg.mozilla.org/projects/addonsmgr/rev/2a18f6eb0eea removes AddonInstalls when they are cancelled or failed. Let's just do it that way for now and see what other cases come up.

Comment 5

[Dave Townsend [:mossop]]

Also a follow-up. Should remove the install from the list before notifying listeners and fix the PFS tests: http://hg.mozilla.org/projects/addonsmgr/rev/5ff859837f75

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Comment 7

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

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

>diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_startup.js b/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
>--- a/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
>+++ b/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
>@@ -172,7 +172,10 @@ function run_test_1() {
> 
>     AddonManager.getAddonsByTypes(["extension"], function(extensionAddons) {
>       do_check_eq(extensionAddons.length, 3);
>-      run_test_2();
>+
>+      // Make sure the next test actually changes the modification time of the
>+      // add-on.
>+      do_timeout(1000, run_test_2);
bah...

There must be a better way but that can be a followup bug

Comment 8

[Dave Townsend [:mossop]]

(In reply to comment #7)
> (From update of attachment 435784 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_startup.js b/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
> >--- a/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
> >+++ b/toolkit/mozapps/extensions/test/xpcshell/test_startup.js
> >@@ -172,7 +172,10 @@ function run_test_1() {
> > 
> >     AddonManager.getAddonsByTypes(["extension"], function(extensionAddons) {
> >       do_check_eq(extensionAddons.length, 3);
> >-      run_test_2();
> >+
> >+      // Make sure the next test actually changes the modification time of the
> >+      // add-on.
> >+      do_timeout(1000, run_test_2);
> bah...
> 
> There must be a better way but that can be a followup bug

Removed that since the changes from bug 555083 make it unnecessary.

Comment 9

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/rev/fb65c6d92505

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 02/17 - 02/21)]

Verified fixed based on check-in and passing automated tests.