Closed Bug 553022 Opened 13 years ago Closed 12 years ago

AddonInstall instances are being held onto

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.3a5

People

(Reporter: Unfocused, Assigned: mossop)

References

Details

(Whiteboard: [rewrite])

Attachments

(1 file)

AddonInstall instances are being held onto, after they are canceled - and get returned by AddonManager.getInstalls()
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?
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()
This is causing tests to fail so we need to do /something/ here.
Blocks: 461973
Priority: -- → P1
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.
Status: NEW → ASSIGNED
Whiteboard: [rewrite] → [rewrite][fixed-in-addonsmgr][needs-review]
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
Attached patch patch rev 1Splinter Review
Attachment #435784 - Flags: review?(robert.bugzilla)
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
Attachment #435784 - Flags: review?(robert.bugzilla) → review+
(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.
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-review] → [rewrite][fixed-in-addonsmgr][needs-landing]
http://hg.mozilla.org/mozilla-central/rev/fb65c6d92505
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [rewrite][fixed-in-addonsmgr][needs-landing] → [rewrite]
Target Milestone: --- → mozilla1.9.3a5
Verified fixed based on check-in and passing automated tests.
Status: RESOLVED → VERIFIED
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.