Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Description • 15 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 1 • 15 years ago

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?

	Blair McBride [:Unfocused] (UNAVAILABLE) Reporter
	Comment 2 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 3 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 4 • 14 years ago

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.

	Dave Townsend [:mossop] Assignee
	Comment 5 • 14 years ago

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

	Robert Strong (they/them - no direct email)
	Comment 7 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 8 • 14 years ago

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

	Dave Townsend [:mossop] Assignee
	Comment 9 • 14 years ago

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

	Henrik Skupin [:whimboo][⌚️UTC+2]
	Comment 10 • 14 years ago

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