Widget is not removed on disable/uninstall/upgrade

RESOLVED FIXED in 1.0

Status

Add-on SDK
General
P1
normal
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jeremy Cutler, Assigned: ochameau)

Tracking

unspecified
x86
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Using 1.0b5, when I create a widget and then disable, uninstall or upgrade/downgrade the add-on, the widget remains. When I re-enable, install again or the upgrade/downgrade replacement finishes in the same browser session (no restarting Firefox), an additional widget is created. In fact, every time you do this another one is created. The correct number of widgets remain when you restart the browser.

Reproducible: Always

Steps to Reproduce:
1. Create a widget using the widget API
2. Load Firefox
3. Disable the add-on
4. Enable the add-on

Actual Results:  
Every time you repeat the disabling and re-enabling, one more widget appears and the old ones are not removed.

Expected Results:  
When disabling or uninstalling, I would expect the widget to disappear.

It is possible to remove the old widget correctly like so:

=============

/*** Create widget ***/
var widget;

widget = widgets.Widget({
   label: "myLabel",
   id: "myId",
   tooltip: "My Tooltip",
   contentURL: data.url("icon32.png"),
   onClick: function() {
      openSettings();
   }
});

/*** Remove old widget on disable/uninstall/upgrade ***/
require("unload").when(function(reason){
   if (widget) {
      widget.destroy();
   }
});

=============

This certainly works, but I don't think it is ideal. I think that this behaviour should be the default. Also, the unload module is not part of the addon-kit API, but the rather less accessible api-utils. It was certainly not obvious to me how to implement this behaviour without digging through the api-utils reference documentation.
Irakli: can you take a look at this?  It sounds like a very bad regression.
Priority: -- → P1
Target Milestone: --- → 1.0

Comment 2

7 years ago
I also have this problem。
use addon SDK 1.0b5
Assignee: nobody → rFobic
Fallout from making widgets able to show different content in different windows, perhaps?
Blocks: 660286
(Assignee)

Comment 4

7 years ago
Created attachment 536094 [details] [diff] [review]
Remove widget on module unload

Thanks for your report Jeremy!

This is a regression from my big patch against widget that added WidgetView stuff.
It's impressive we let such bug get out without noticing it!

I've added one unit test that verify widget removal on module unload.
It would be great to review our codebase to ensure that we have such test for every API!
Assignee: rFobic → poirot.alex
Attachment #536094 - Flags: review?(rFobic)
(Assignee)

Updated

7 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 536094 [details] [diff] [review]
Remove widget on module unload

Review of attachment 536094 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!
Attachment #536094 - Flags: review?(rFobic) → review+
(Reporter)

Comment 6

7 years ago
No problem, guys. Keep up the good work and I'll be back if I find anything else you might want to know about. Do you know if a fix for this will be in beta 6 or a later release?
(Assignee)

Comment 7

7 years ago
Jeremy: It is going to be included in the next release, ie 1.0rc1 that is going to be released ASAP.

Landed:
https://github.com/mozilla/addon-sdk/commit/41241578a9c525be47d7dba5c236e631b6823bc4
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.