Closed Bug 640775 Opened 14 years ago Closed 5 years ago

Allow a form of uninstalling add-ons installed globally by third-party applications or system administrators

Categories

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

Product:
Toolkit
Component:
Add-ons Manager
Type:
defect

Tracking

()

Status:
RESOLVED WONTFIX

People

(Reporter: mossop, Unassigned)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

patch (6)
49.65 KB, patch
mossop
: review-
Details | Diff | Splinter Review
While we still don't want to actually remove these from the system we do want the user to be able to remove them from Firefox in some way. Details to be specced out but basic idea is that the remove button should work, the add-on should become invisible but be visible in "Available Add-ons" when searched for.
Wow, "third-party add-ons", no mention of "Firefox 4".... the title is so obscure... No wonder why I didn't find this bug using bugzilla search. I'm not sure what's meant by "third-party add-ons"... all my add-ons are found in Mozilla Add-on official website. Except "Firefox Sync", all are developed by "third-party" person. I'm not sure the title of this bug really makes sense.
It means add-ons that are installed outside of Firefox by copying to any of the system install locations or installing through the registry. Basically any of the cases where users cannot currently use the remove button.
Summary: Allow a form of uninstalling third-party add-ons → Allow a form of uninstalling add-ons installed by third-party applications
OK, I see. Could you add the following keywords to make this bug easier to be found, please? global-install system-wide FF4
Summary: Allow a form of uninstalling add-ons installed by third-party applications → Allow a form of uninstalling add-ons installed globally by third-party applications or system administrators
Is there any hope of a rapid solution ? In particular for old spell checkers that are no more supported...
Or at the very least REQUIRE the REMOVE button on ALL add-ons and extensions. I just spent almost an hour going through all the extensions.* files + prefs.js just to get rid of an obsolete DigitalPersona extension which did not have the Remove option. It was originally installed by Windows software. I still use DigitalPersona for other apps, including (ironically) logging into Firefox master pwd. ... thanks!
(In reply to Dave from comment #11) > Or at the very least REQUIRE the REMOVE button on ALL add-ons and extensions. This is a very good rule for the future. This should probably be standard in the kits proposed to extension authors. But this does not solve the present problem of old extensions that do not have the remove button. Editing "all the extensions.* files + prefs.js just to get rid of an obsolete ... extension" cannot be asked to a normal user (only to a geek). Firefox extension manager should have a button to force corresponding actions once the extension is selected...
I think we have three options. Either we fake the uninstall ourselves. This is safest in that we control it all but since we won't be removing the add-on's files it feels a little sneaky. But the add-on will be gone from the UI. The problem with this approach is that we don't have reliable means to detect if the add-on got "re-installed" in some way except maybe by mtime, but that has caused us problems in the past (and we don't want these things re-appearing randomly). Alternatively we let developers give us some code to run (a binary maybe) to do the uninstall. That'd allow the developer to do whatever necessary, maybe uninstalling the files, requesting elevation, whatever integration their app needs. It's harder to co-ordinate though, we'd be relying on the binary to tell use whether the uninstall was successful and whether a Firefox restart would be necessary to complete it. Talking to executables is painful right now, all we have support for is return codes in nsIProcess which is sort of clunky. I guess we could also used shared libraries and ctypes to do it a little more cleanly though. Hopefully that'd mean the files went away too, but it also gives the developer a way to avoid the uninstall by being a bad actor when asked. The final option is to abandon our existing policy of not making filesystem/registry changes and just do that. We'd need to ship a binary with Firefox that could be called, requested elevated privileges and then made the changes. That wouldn't work in all cases, but it might be the simplest to implement. The latter two options probably don't work well for non-restartless add-ons where we could only let the files go away when Firefox is quit (or currently when it next starts!) since that'd be when we needed to request elevation and that would probably be quite confusing to the user.
I am an user (curious, loving to use my file manager to look at what is in my PC) and not a developer. It seems to me cleaner to erase the files and registery key(s) of the add-on I wish to kill. And so, I am sure that it will not reinstall by error... Normally add-on are in the profile=user space and it has right to erase. If they are in common space, I need to use my admin account (XP) or elevated privileges (Vista) to uninstall but this is not a problem for me. As long as it said in a message, I have no objection that I need to close and restart FF or TB before the uninstall is effective : this is common practice. Please clarify "non-restartless add-ons" . Does this means add-ons that need restart ?
(In reply to Jean-Marie COUPRIE from comment #15) > Please clarify "non-restartless add-ons" . Does this means add-ons that need > restart ? Yes
Assignee: nobody → sachinhosmani2
(In reply to Dave Townsend (:Mossop) from comment #14) > Either we fake the uninstall ourselves. This is safest in that we control it > all but since we won't be removing the add-on's files it feels a little > sneaky. But the add-on will be gone from the UI. The problem with this > approach is that we don't have reliable means to detect if the add-on got > "re-installed" in some way except maybe by mtime, but that has caused us > problems in the past (and we don't want these things re-appearing randomly). I don't understand what you mean by "re-install". Since uninstallation of a global add-on is not possible, how does a re-installation happen? If the add-on files were removed manually (not via Firefox interface) and the add-on were again installed, even in the profile where the add-on was once removed (actually just hidden from the UI), an opt-in screen would show and confirm the user's interest in the add-on. Could you please explain what you mean by things re-appearing randomly? > The final option is to abandon our existing policy of not making > filesystem/registry changes and just do that. Is there such a policy? We ask vendors to make changes in the registry and file system. Are you referring to making Firefox undo those changes itself when a removal is needed? > The latter two options probably don't work well for non-restartless add-ons > where we could only let the files go away when Firefox is quit (or currently > when it next starts!) since that'd be when we needed to request elevation > and that would probably be quite confusing to the user. Couldn't we make Firefox remove the files while quitting (instead of next start, in which case it would be confusing)? This way we could tell the user that he would need to restart Firefox to remove the add-on and would also need to allow Firefox to do the same (permission-wise).
(In reply to Sachin Hosmani from comment #18) > (In reply to Dave Townsend (:Mossop) from comment #14) > > Either we fake the uninstall ourselves. This is safest in that we control it > > all but since we won't be removing the add-on's files it feels a little > > sneaky. But the add-on will be gone from the UI. The problem with this > > approach is that we don't have reliable means to detect if the add-on got > > "re-installed" in some way except maybe by mtime, but that has caused us > > problems in the past (and we don't want these things re-appearing randomly). > > I don't understand what you mean by "re-install". Since uninstallation of a > global add-on is not possible, how does a re-installation happen? > If the add-on files were removed manually (not via Firefox interface) and > the add-on were again installed, even in the profile where the add-on was > once removed (actually just hidden from the UI), an opt-in screen would show > and confirm the user's interest in the add-on. > Could you please explain what you mean by things re-appearing randomly? I'm running firefox containing an add-on installed by another application on my system. I "uninstall" it and it has gone away. Later I change my mind and I want the add-on back. How do I get it? The add-on is only available as part of the application. Maybe if I reinstall that application it will come back? So I close Firefox, uninstall and reinstall the app and start Firefox again. All Firefox sees though is that the add-on's files are still where they were, maybe their modification time has changed. We could choose to show the opt-in screen again but then we'd probably do that whenever the other application was merely upgraded, not just reinstalled. > > The final option is to abandon our existing policy of not making > > filesystem/registry changes and just do that. > > Is there such a policy? We ask vendors to make changes in the registry and > file system. Are you referring to making Firefox undo those changes itself > when a removal is needed? Yes, we've long said that Firefox won't make changes to files and registry entries installed by other applications. Worst case it could actually break the other application. > > The latter two options probably don't work well for non-restartless add-ons > > where we could only let the files go away when Firefox is quit (or currently > > when it next starts!) since that'd be when we needed to request elevation > > and that would probably be quite confusing to the user. > > Couldn't we make Firefox remove the files while quitting (instead of next > start, in which case it would be confusing)? > This way we could tell the user that he would need to restart Firefox to > remove the add-on and would also need to allow Firefox to do the same > (permission-wise). We could, I still think that if you choose to uninstall an add-on and then continue browsing (maybe for days) and then when you next close Firefox it asks for elevated privileges. Unless we can communicate in that dialog why we need them (I have no idea if we can) then the user will have no idea what is going on. Mind you users generally just accept that dialog anyway.
(In reply to Dave Townsend (:Mossop) from comment #19) > > Couldn't we make Firefox remove the files while quitting (instead of next > > start, in which case it would be confusing)? > > This way we could tell the user that he would need to restart Firefox to > > remove the add-on and would also need to allow Firefox to do the same > > (permission-wise). > > We could, I still think that if you choose to uninstall an add-on and then > continue browsing (maybe for days) and then when you next close Firefox it > asks for elevated privileges. Unless we can communicate in that dialog why > we need them (I have no idea if we can) then the user will have no idea what > is going on. Mind you users generally just accept that dialog anyway. Yes, that would be a problem. But I don't see another option since removing is not allowed while Firefox is running. Maybe we could show a message when Firefox is closed telling the user that he will need to approve the removal, but I don't know if it deserves that much attention. There are add-ons with scope SCOPE_USER which are installed in all Firefox profiles of the current logged-in system user. (Where should I put an add-on for it to get installed with this scope? I tried ~/.mozilla/extensions but Firefox didn't recognize it.) These add-ons wouldn't need elevated privileges for removal. So, are they completely removable from the UI?
(In reply to Sachin Hosmani from comment #20) > (In reply to Dave Townsend (:Mossop) from comment #19) > > > > Couldn't we make Firefox remove the files while quitting (instead of next > > > start, in which case it would be confusing)? > > > This way we could tell the user that he would need to restart Firefox to > > > remove the add-on and would also need to allow Firefox to do the same > > > (permission-wise). > > > > We could, I still think that if you choose to uninstall an add-on and then > > continue browsing (maybe for days) and then when you next close Firefox it > > asks for elevated privileges. Unless we can communicate in that dialog why > > we need them (I have no idea if we can) then the user will have no idea what > > is going on. Mind you users generally just accept that dialog anyway. > > Yes, that would be a problem. But I don't see another option since removing > is not allowed while Firefox is running. Maybe we could show a message when > Firefox is closed telling the user that he will need to approve the removal, > but I don't know if it deserves that much attention. > > There are add-ons with scope SCOPE_USER which are installed in all Firefox > profiles of the current logged-in system user. (Where should I put an add-on > for it to get installed with this scope? I tried ~/.mozilla/extensions but > Firefox didn't recognize it.) See https://developer.mozilla.org/en-US/docs/Installing_extensions for the locations to install add-ons > These add-ons wouldn't need elevated privileges for removal. So, are they > completely removable from the UI? Assuming the user has write access there then yes
I asked Jorge on IRC. I'll be using the 1st option Dave suggested: hiding the add-on from the UI.
I'm a little late to the party, but: Yep, that sounds best to me.
Status: NEW → ASSIGNED
For the removal of a global addon to be remembered across restarts (since the files aren't actually removed), I'm finding a need to introduce a new property to the addon class (say, "globalAndRemoved"), that will tell if the addon is a global one and has been removed by the user. I've added a filter here: https://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/content/extensions.js#1461 that will leave out the removed global addons provided such a property exists. Could there be a better method to accomplish the same?
And of course, the property would need to be stored in the db along with the other properties.
The remove button is enabled for global add-ons. A new property called "globalAndRemoved" is created in the Addon class which gets stored in the database along with the other properties so that the removal is remembered across restarts. When removed they are filtered out based on this property and are hidden from the list.
Attachment #761112 - Flags: review?(jorge)
Comment on attachment 761112 [details] [diff] [review] Allows uninstallion of add-ons installed globally from the list and detail views >+ if (val && !this.isGlobal()) >+ return; Shouldn't we log an error in the console, at least? > this.__defineSetter__("userDisabled", function AddonWrapper_userDisabledSetter(val) { > if (val == this.userDisabled) > return val; > > if (aAddon instanceof DBAddonInternal) { > if (aAddon.type == "theme" && val) { > if (aAddon.internalName == XPIProvider.defaultSkin) > throw new Error("Cannot disable the default theme"); >@@ -6291,16 +6315,22 @@ > throw new Error("Add-on is not marked to be uninstalled"); > XPIProvider.cancelUninstallAddon(aAddon); > }; > > this.findUpdates = function AddonWrapper_findUpdates(aListener, aReason, aAppVersion, aPlatformVersion) { > new UpdateChecker(aAddon, aListener, aReason, aAppVersion, aPlatformVersion); > }; > >+ this.isGlobal = function AddonWrapper_isGlobal() { >+ return (this.scope === AddonManager.SCOPE_ALL || >+ this.scope === AddonManager.SCOPE_SYSTEM || >+ this.scope === AddonManager.SCOPE_APPLICATION); >+ }; >+ > this.hasResource = function AddonWrapper_hasResource(aPath) { > if (aAddon._hasResourceCache.has(aPath)) > return aAddon._hasResourceCache.get(aPath); > > let bundle = aAddon._sourceBundle.clone(); > > // Bundle may not exist any more if the addon has just been uninstalled, > // but explicitly first checking .exists() results in unneeded file I/O. >@@ -6332,17 +6362,17 @@ > } > catch (e) { > aAddon._hasResourceCache.set(aPath, false); > return false; > } > finally { > zipReader.close(); > } >- }, >+ }; What is this about? Random syntax fix? Passing on to Blair, who knows this code better than anyone else.
Attachment #761112 - Flags: review?(jorge)
Attachment #761112 - Flags: review?(bmcbride)
Attachment #761112 - Flags: review+
Comment on attachment 761112 [details] [diff] [review] Allows uninstallion of add-ons installed globally from the list and detail views Review of attachment 761112 [details] [diff] [review]: ----------------------------------------------------------------- Unfortunately, if uninstalled add-ons are returned together with normal add-ons by the existing AddonManager APIs, everything that uses those APIs will need to be updated to check the globalAndRemoved property - since it won't be expecting to see uninstalled add-ons. To solve that, uninstalled add-ons need to never be returned by getAllAddons, getAddonsByTypes, etc. Instead, we should introduce a new API that only returns uninstalled add-ons (similar to getAddonsWithPendingOperations). To ensure such add-ons are never normally returned, they should probably be marked as not visible (the database field 'visible' set to 0) as well - that way all the existing code should just work automatically. I should note that since this deals with the database, it's touching code that's currently being rewritten in bug 853388 (I've been busy reviewing that, which is why I took so long to respond here - sorry!). You might want to get in touch with Irving to work out what's the best way to handle that conflict, as that's quite a big project.
Attachment #761112 - Flags: review?(bmcbride) → review-
I have a few thoughts and questions.. - Does this issue apply only to XPI extensions (add-ons, themes, dictionaries) or does it also apply to plugins? - As Blair points out, lots of overlap with the work in Bug 853388. It would save me a few hours if this change could depend on bug 853388 and land later, but that means waiting for a couple of weeks or tracking my WIP patches, which would be a ton of work for Sachin. - I agree with Blair's approach of using the XPI Provider's 'visible' flag to control whether the addon is returned from any of the XPI Provider's list APIs. We just need to be careful to skip uninstalled add-ons at the places in XPIProvider and XPIProviderUtils where we adjust the visible flag. - I'd prefer a name something like "uninstalled" for the field, rather than globalAndRemoved - We already have an interface to tell whether an add-on can be physically removed, can we use that? (aAddon._installLocation.locked - see for example XPI_uninstallAddon() at https://hg.mozilla.org/mozilla-central/file/834c8941ae24/toolkit/mozapps/extensions/XPIProvider.jsm#l4140) - In terms of APIs for listing removed add-ons, I'd prefer that we decide on the UX for removing and restoring these add-ons before we implement the back end, to make sure we don't make things difficult for ourselves.
(In reply to :Irving Reid from comment #29) > - Does this issue apply only to XPI extensions (add-ons, themes, > dictionaries) or does it also apply to plugins? Only XPI extensions, I think. Plugins haven't been a problem in this area so far, and I assume a solution that includes plugins would make things much more complicated. > - As Blair points out, lots of overlap with the work in Bug 853388. It would > save me a few hours if this change could depend on bug 853388 and land > later, but that means waiting for a couple of weeks or tracking my WIP > patches, which would be a ton of work for Sachin. This is part of a GSOC project, so we could hold off on this for a couple of weeks and have Sachin move to other tasks in the project. How many weeks would you project this would take? > - In terms of APIs for listing removed add-ons, I'd prefer that we decide on > the UX for removing and restoring these add-ons before we implement the back > end, to make sure we don't make things difficult for ourselves. The approach we decide on is the first one on comment #14. It's less than ideal, but at least moves things forward and addresses the main problem.
(In reply to Jorge Villalobos [:jorgev] from comment #30) > (In reply to :Irving Reid from comment #29) > > - As Blair points out, lots of overlap with the work in Bug 853388. It would > > save me a few hours if this change could depend on bug 853388 and land > > later, but that means waiting for a couple of weeks or tracking my WIP > > patches, which would be a ton of work for Sachin. > > This is part of a GSOC project, so we could hold off on this for a couple of > weeks and have Sachin move to other tasks in the project. How many weeks > would you project this would take? We're hoping to land right after the version 25 merge next week, unless a big problem shows up in late reviews or Try builds. > > - In terms of APIs for listing removed add-ons, I'd prefer that we decide on > > the UX for removing and restoring these add-ons before we implement the back > > end, to make sure we don't make things difficult for ourselves. > > The approach we decide on is the first one on comment #14. It's less than > ideal, but at least moves things forward and addresses the main problem. I'm concerned about the issue Dave raises in comment #19 - what happens when someone uninstalls an add-on this way, and then decides they want to install it again?
(In reply to :Irving Reid from comment #31) > (In reply to Jorge Villalobos [:jorgev] from comment #30) > > The approach we decide on is the first one on comment #14. It's less than > > ideal, but at least moves things forward and addresses the main problem. > > I'm concerned about the issue Dave raises in comment #19 - what happens when > someone uninstalls an add-on this way, and then decides they want to install > it again? I think that's a rare edge case, given that these add-ons are unwanted for the most part. That's not to dismiss the issue entirely, but I think we can file a follow-up bug to deal with this. One way I think we can resolve this is also part of Sachin's project, which is a way for external processes to notify Firefox about global add-on installation. Any add-on being installed or reinstalled this way would be known by Firefox and then we can do the right thing.
(In reply to Jorge Villalobos [:jorgev] from comment #32) > One way I think we can resolve this is also part of Sachin's project, which > is a way for external processes to notify Firefox about global add-on > installation. Any add-on being installed or reinstalled this way would be > known by Firefox and then we can do the right thing. That's bug 879480, by the way.
(In reply to :Irving Reid from comment #29) > I have a few thoughts and questions.. > > - I'd prefer a name something like "uninstalled" for the field, rather than > globalAndRemoved But one might think all uninstalled add-ons still exist in the db, but actually only global ones do. Non-global add-ons get wiped off the db when removed, right? > - We already have an interface to tell whether an add-on can be physically > removed, can we use that? (aAddon._installLocation.locked - see for example > XPI_uninstallAddon() at > https://hg.mozilla.org/mozilla-central/file/834c8941ae24/toolkit/mozapps/ > extensions/XPIProvider.jsm#l4140) Yes, there is a way to know that. But the problem arises because both the "disable" and "remove" buttons must be active. A global add-on can be just disabled in which case it must be grayed out. But when removed, it must be hidden from the list every time. So, I found the need to introduce a new property that stores this extra information. But now, as you all suggest setting the "visible" property, I'd like to know the role of it. Leaving this whole feature out, what add-ons have this property set to false? Physically removable add-ons are wiped off the db when removed and the others can only be disabled in which case they are still visible but grayed out. So, what role does this play, currently?
(In reply to Sachin Hosmani from comment #34) > (In reply to :Irving Reid from comment #29) > > I have a few thoughts and questions.. > > > > - I'd prefer a name something like "uninstalled" for the field, rather than > > globalAndRemoved > > But one might think all uninstalled add-ons still exist in the db, but > actually only global ones do. Non-global add-ons get wiped off the db when > removed, right? I find "global" a bit misleading, because elsewhere in the code we decide based on a "locked" location (one that we can't remove). Yes, we completely remove uninstalled add-ons from the database now. As far as I'm concerned we can explain the difference with a good comment on the declaration of this field, something like /* * User has uninstalled this add-on, but it is in a locked location so it cannot * be removed from the install location */ I don't want to spend too much time discussing the name, as long as it's not completely misleading and the code and comments make it clear what it's for. > > - We already have an interface to tell whether an add-on can be physically > > removed, can we use that? (aAddon._installLocation.locked - see for example > > XPI_uninstallAddon() at > > https://hg.mozilla.org/mozilla-central/file/834c8941ae24/toolkit/mozapps/ > > extensions/XPIProvider.jsm#l4140) > > Yes, there is a way to know that. But the problem arises because both the > "disable" and "remove" buttons must be active. A global add-on can be just > disabled in which case it must be grayed out. But when removed, it must be > hidden from the list every time. > So, I found the need to introduce a new property that stores this extra > information. I meant that you could use addon._installLocation.locked instead of your current addon.isGlobal property (perhaps by defining a getter to retrieve _installLocation.locked). > But now, as you all suggest setting the "visible" property, I'd like to know > the role of it. > Leaving this whole feature out, what add-ons have this property set to false? > Physically removable add-ons are wiped off the db when removed and the > others can only be disabled in which case they are still visible but grayed > out. So, what role does this play, currently? The "visible" property is used for XPI add-ons to keep track, when versions of the same add-on are installed in more than one location - for example, my Firefox could come with v1.1 of Adblock installed in SCOPE_APPLICATION, and then I could install v1.2 in SCOPE_PROFILE. Both versions are recorded in the database; the version in SCOPE_PROFILE has addon.visible = true and the version in SCOPE_APPLICATION has visible=false. Most of the XPI Provider APIs filter out the non-visible add-ons. I agree that you still need to add a property to keep track of "locked but uninstalled" add-ons; my suggestion is that you also use the value of that property as part of the calculation that sets the 'visible' property. That way the APIs that filter based on visible will return a filtered list without needing to know about the new property.
Depends on: 853388
I'll wait for bug 853388 to land.
(In reply to Jorge Villalobos [:jorgev] from comment #32) > > I'm concerned about the issue Dave raises in comment #19 - what happens when > > someone uninstalls an add-on this way, and then decides they want to install > > it again? > > I think that's a rare edge case, given that these add-ons are unwanted for > the most part. That's not to dismiss the issue entirely, but I think we can > file a follow-up bug to deal with this. Yes, I think that'd be a good followup. It is a problem I'd like to see solved - via presenting a "available add-ons" UI, as well as bug 879480. (In reply to :Irving Reid from comment #35) > I find "global" a bit misleading, because elsewhere in the code we decide > based on a "locked" location (one that we can't remove). +1 - I'd prefer to stay away from "global". > I meant that you could use addon._installLocation.locked instead of your > current addon.isGlobal property (perhaps by defining a getter to retrieve > _installLocation.locked). Why do we need a property for this? Just to know when to show the uninstall button? That's what the permissions property is for. > I agree that you still need to add a property to keep track of "locked but > uninstalled" add-ons; my suggestion is that you also use the value of that > property as part of the calculation that sets the 'visible' property. That > way the APIs that filter based on visible will return a filtered list > without needing to know about the new property. +1
This just adds the 'uninstalled' field to the list we store in XPIProviderUtils.js "extensions.json" database, so that I can land the new field as soon as possible after bug 853388 and avoid having to increment the DB version again for this patch.
Attachment #787761 - Flags: review?(bmcbride)
Comment on attachment 787761 [details] [diff] [review] Add "uninstalled" field to the XPI JSON data Review of attachment 787761 [details] [diff] [review]: ----------------------------------------------------------------- As discussed on IRC, this bug won't require a DB revision change. And I'm not concerned at all about people going back and forth between builds that include this fix and those that don't - as it will only affect Nightly.
Attachment #787761 - Flags: review?(bmcbride) → review-
Attached patch patch (2) (obsolete) — — Splinter Review
There is a new property called "uninstalled" that is set to true when a global add-on is removed. The add-on is marked not visible so that the existing API doesn't return it in its various methods. There is now a method called "getUninstalledAddons" that returns all such add-ons. We are in need of a UI that will show these "available" add-ons to the user.
Attachment #761112 - Attachment is obsolete: true
Attachment #791381 - Flags: review?(bmcbride)
Blocks: 879480
I realize I have a few things to fix in that patch. Blair, don't review it yet. I'll submit another one real soon.
Hopefully whoever finds time might review this.
Attachment #791381 - Attachment is obsolete: true
Attachment #791381 - Flags: review?(bmcbride)
Attachment #795007 - Flags: review?(dtownsend+bugmail)
Attachment #795007 - Flags: review?(bmcbride)
Comment on attachment 795007 [details] [diff] [review] Fixed some defects in the previous patch. Review of attachment 795007 [details] [diff] [review]: ----------------------------------------------------------------- Getting there! :) ::: toolkit/mozapps/extensions/XPIProvider.jsm @@ +3458,5 @@ > + * > + * @param aCallback > + * A callback to pass an array of AddonWrappers to. > + */ > + getUninstalledAddons: Since we can have multiple copies of an add-on (same ID) installed in different locations, normally the 'active' property makes it so only one copy of the addon is returned via these APIs (it's only ever set to true for one copy of an addon of a given ID). Since you're making it possible for it to be set to false for all add-ons of a given ID, this method can end up returning multiple copies of the same add-on. The install locations are in a prioritized list - probably makes the most sense to only return the addon in the highest priority location. @@ +6317,5 @@ > + // Since userDisabled changes as a side-effect of this property, this > + // property must be changed first so that userDisabled listeners can > + // tell the difference. > + XPIDatabase.setAddonProperties(aAddon, { > + uninstalled: true This is another issues with multiple copies of an add-on in different locations. Will need to set this on all copies. Otherwise we'll get bugs like when an add-on is removed from disk in one location, a copy in another location will get enabled - instead of the add-on staying "uninstalled". ::: toolkit/mozapps/extensions/content/extensions.js @@ +1034,5 @@ > + // Permissions must exist for uninstallation > + // If not, it is a global add-on and must have permissions for disabling > + // or must already be inactive. > + return hasPermission(aAddon, "uninstall") || > + hasPermission(aAddon, "disable") || !aAddon.active; Shouldn't be special-casing this - that the addon is in a locked location is an implementation detail. If we're allowing uninstall (even if not technically the same type of uninstall), the addon's permission property should include PERM_CAN_UNINSTALL. Similarly, when uninstalling such an addon it should sent the same of events as a normal uninstall (e, it shouldn't send onDisabled). Fixing those two things should mean the UI won't need any changes.
Attachment #795007 - Flags: review?(dtownsend+bugmail)
Attachment #795007 - Flags: review?(bmcbride)
Attachment #795007 - Flags: review-
(In reply to Blair McBride [:Unfocused] from comment #43) > Since we can have multiple copies of an add-on (same ID) installed in > different locations, normally the 'active' property makes it so only one > copy of the addon is returned via these APIs (it's only ever set to true for > one copy of an addon of a given ID). Since you're making it possible for it > to be set to false for all add-ons of a given ID, this method can end up > returning multiple copies of the same add-on. > > The install locations are in a prioritized list - probably makes the most > sense to only return the addon in the highest priority location. If we have an uninstalled addon in a locked location, and a visible addon in a higher priority location, I'm wondering if we even want to show the uninstalled one - because even if a user "re-installs" it by using UI to remove the "uninstalled" flag, they still won't see that version of the addon in any other UI because it should still be not visible (hidden by the version in the higher priority location). > @@ +6317,5 @@ > > + // Since userDisabled changes as a side-effect of this property, this > > + // property must be changed first so that userDisabled listeners can > > + // tell the difference. > > + XPIDatabase.setAddonProperties(aAddon, { > > + uninstalled: true > > This is another issues with multiple copies of an add-on in different > locations. Will need to set this on all copies. Otherwise we'll get bugs > like when an add-on is removed from disk in one location, a copy in another > location will get enabled - instead of the add-on staying "uninstalled". I fear it's going to be very difficult to explain to end users what is happening in these circumstances. It's already a bit difficult with the existing visibility rules, but this is going to make it even more so. My first guess would be to have the behaviour *only* affect the visibility of the addon, so in the situation: Location 1 (high priority but locked): Addon-1 version 1.3 Location 2 (lower priority): Addon-1 version 1.2 The location 1 copy is visible, and may or may not be enabled; the location 2 copy is not visible. If the user uninstalls the copy from location 1, my idea is that the copy in location 2 becomes visible but disabled. If we could always enforce the rule that the locked locations are lower priority (so there would be no user-installed copies of the addon hidden behind locked copies), then it might make sense to set the uninstall flag on *all* the locked copies. This is a pretty rare scenario, I would guess; it requires something like a Firefox distribution that has the addon bundled in the distribution, installed on an OS that also has a copy of that addon in a system location. I could see an IT department trying to standardize their Firefox environment getting themselves into a situation like that, but I'm not sure we need to spend too much time trying to figure out what the perfect user experience is in that situation. > ::: toolkit/mozapps/extensions/content/extensions.js > @@ +1034,5 @@ > > + // Permissions must exist for uninstallation > > + // If not, it is a global add-on and must have permissions for disabling > > + // or must already be inactive. > > + return hasPermission(aAddon, "uninstall") || > > + hasPermission(aAddon, "disable") || !aAddon.active; > > Shouldn't be special-casing this - that the addon is in a locked location is > an implementation detail. If we're allowing uninstall (even if not > technically the same type of uninstall), the addon's permission property > should include PERM_CAN_UNINSTALL. > > Similarly, when uninstalling such an addon it should sent the same of events > as a normal uninstall (e, it shouldn't send onDisabled). > > Fixing those two things should mean the UI won't need any changes.
(In reply to Blair McBride [:Unfocused] from comment #43) > Comment on attachment 795007 [details] [diff] [review] > Fixed some defects in the previous patch. > > Review of attachment 795007 [details] [diff] [review]: > ----------------------------------------------------------------- > This is another issues with multiple copies of an add-on in different > locations. Will need to set this on all copies. Otherwise we'll get bugs > like when an add-on is removed from disk in one location, a copy in another > location will get enabled - instead of the add-on staying "uninstalled". I'm sorry, I don't understand what you mean. Can you please tell me of a use case where this happens? And I've added a check in the makeAddonVisible method that prevents "uninstalled" add-ons from becoming visible. > Fixing those two things should mean the UI won't need any changes. Which UI do you mean? I thought a UI change was in store for sure. We need to create one that shows uninstalled add-ons right? (Bug 891405) (In reply to :Irving Reid from comment #44) > My first guess would be to have the behaviour *only* affect the visibility > of the addon, so in the situation: > > Location 1 (high priority but locked): Addon-1 version 1.3 > Location 2 (lower priority): Addon-1 version 1.2 > > The location 1 copy is visible, and may or may not be enabled; the location > 2 copy is not visible. > > If the user uninstalls the copy from location 1, my idea is that the copy in > location 2 becomes visible but disabled. I think this is what happens now.
(In reply to Sachin Hosmani [:sachin] from comment #45) > (In reply to Blair McBride [:Unfocused] from comment #43) > > Fixing those two things should mean the UI won't need any changes. > > Which UI do you mean? I thought a UI change was in store for sure. We need > to create one that shows uninstalled add-ons right? (Bug 891405) Oh you meant the logic behind showing/hiding the remove button would remain the same. :) But it seems like quite a bit of tweaking is needed to make this hacky uninstall seem like a completely normal one. Is it really a problem if those onDisabled, onDisabling, etc. calls are made here? Currently uninstallation of restartless add-ons happen first as a disable and then a complete removal(when the UI is closed). I suppose the disabling causes those onDisabling, onDisabled listeners to be called. Is that okay?
(In reply to :Irving Reid from comment #44) > If we have an uninstalled addon in a locked location, and a visible addon in > a higher priority location, I'm wondering if we even want to show the > uninstalled one - because even if a user "re-installs" it by using UI to > remove the "uninstalled" flag, they still won't see that version of the > addon in any other UI because it should still be not visible (hidden by the > version in the higher priority location). Ah, yes, indeed. Yay edge cases. Related to this, see bug 910550, which I discussed with Sachin the other day, and finally got around to filing today. > I fear it's going to be very difficult to explain to end users what is > happening in these circumstances. It's already a bit difficult with the > existing visibility rules, but this is going to make it even more so. > > My first guess would be to have the behaviour *only* affect the visibility > of the addon, so in the situation: > > Location 1 (high priority but locked): Addon-1 version 1.3 > Location 2 (lower priority): Addon-1 version 1.2 > > The location 1 copy is visible, and may or may not be enabled; the location > 2 copy is not visible. > > If the user uninstalls the copy from location 1, my idea is that the copy in > location 2 becomes visible but disabled. > > If we could always enforce the rule that the locked locations are lower > priority (so there would be no user-installed copies of the addon hidden > behind locked copies), then it might make sense to set the uninstall flag on > *all* the locked copies. This is a pretty rare scenario, I would guess; it > requires something like a Firefox distribution that has the addon bundled in > the distribution, installed on an OS that also has a copy of that addon in a > system location. I could see an IT department trying to standardize their > Firefox environment getting themselves into a situation like that, but I'm > not sure we need to spend too much time trying to figure out what the > perfect user experience is in that situation. This is something I discussed with Sachin the other day. I'd like to (eventually) introduce a UI specifically for managing this, and keep the uninstall button do what people expect (make the addon go away). See bug 910550. In the meantime, making the uninstall hide all copies of an addon in locked locations (which is an even rarer edge case) gets us half way there. I'm a lot less concerned with being able to reveal the copy of the addon in the lower priority location in this case - since it's both a rare edge case, and it has less of an impact. When the visible copy of the addon is in the profile, it's likely because you've installed a different copy, or the add-on manager has done an upgrade - which can break integration with third party software, so it's important to be able to switch back to the system-installed copy.
(In reply to Sachin Hosmani [:sachin] from comment #45) Clarified all this on IRC. Let me know if we missed anything :) > (In reply to :Irving Reid from comment #44) > > If the user uninstalls the copy from location 1, my idea is that the copy in > > location 2 becomes visible but disabled. > > I think this is what happens now. Almost. It doesn't get disabled. But I want to move away from "uninstall" being a lie. When you hit the Uninstall button in any situation, the addon should disappear. If there are multiple copies in different locations, managing those should be a separate UI (bug 910550). (In reply to Sachin Hosmani [:sachin] from comment #46) > But it seems like quite a bit of tweaking is needed to make this hacky > uninstall seem like a completely normal one. Is it really a problem if those > onDisabled, onDisabling, etc. calls are made here? > Currently uninstallation of restartless add-ons happen first as a disable > and then a complete removal(when the UI is closed). I suppose the disabling > causes those onDisabling, onDisabled listeners to be called. Is that okay? Discussed this on IRC too. But for the record: This type of uninstall needs to avoid sending onDisabling/onDisabled events - it needs to look exactly like a normal uninstall to any code using the API. Otherwise it's going to require constantly adding special cases for it everywhere. Bug 891405 and bug 910550 should be the only UI-side changes needed for all of this, and they're just supplementary (ie, we'll get around to it - but they're not required for this to land).
Oh, see also bug 557710. Sachin, that was probably the reason for the bug you described to me the other day, where installing a restartless version of an addon already installed in a locked location didn't show the upgrade notification in about:addons.
Attached patch patch (3) (obsolete) — — Splinter Review
Some improvements: - No UI changes. I changed the existing API to hold the uninstallation / reinstallation implementation detail. - The uninstalled property is set to all add-ons of the same ID in locked locations. - getUninstalledAddons returns only the uninstalled add-on in the highest priority location only if there is no other copy already installed. - A new "reInstall" method in AddonWrapper that will install an uninstalled add-on. Some concerns: An add-on (in locked location) is made invisible the moment the remove button is pressed. For non-restartless add-ons, this will lead to the undo/restart UI disappearing when the list view is refreshed. Is this an issue? Also, a use case where there was a UI problem: - Open Addons Manager's list view. - Hit the remove button on a non-restartless add-on in a locked location. - The restart/undo UI is shown. - Open Scratchpad and use the API (reInstall method) to re-install the add-on. This will change all the fields correctly in the DB. However, it fails to communicate with the list item to change it into an add-on entry. One way is to use the onExternalInstall event, but I don't know if there is an easy way to get the existing add-on in the list view since the add-on is already invisible at this point.
Attachment #795007 - Attachment is obsolete: true
Attachment #797974 - Flags: review?(bmcbride)
(In reply to Sachin Hosmani [:sachin] from comment #50) > An add-on (in locked location) is made invisible the moment the remove > button is pressed. For non-restartless add-ons, this will lead to the > undo/restart UI disappearing when the list view is refreshed. Is this an > issue? Hmm, doesn't seem like a show-stopper bug. Would be nice to fix though, if it's not too complex. Is this because you're setting the visible DB field to false immediately? Can that not be left until PREF_PENDING_OPERATIONS is next processed on shutdown/startup? > Also, a use case where there was a UI problem: > - Open Addons Manager's list view. > - Hit the remove button on a non-restartless add-on in a locked location. > - The restart/undo UI is shown. > - Open Scratchpad and use the API (reInstall method) to re-install the > add-on. > This will change all the fields correctly in the DB. However, it fails to > communicate with the list item to change it into an add-on entry. One way is > to use the onExternalInstall event, but I don't know if there is an easy way > to get the existing add-on in the list view since the add-on is already > invisible at this point. You need to send the onOperationCancelled event - see XPIProvider.cancelUninstallAddon(). You'll need to call that when a re-install is requested when an uninstall is pending. It'll need updated so AddonWrapper.cancelUninstall() can work here as well.
Comment on attachment 797974 [details] [diff] [review] patch (3) Review of attachment 797974 [details] [diff] [review]: ----------------------------------------------------------------- Making good progress :) ::: toolkit/mozapps/extensions/AddonManager.jsm @@ +1877,5 @@ > }, > > /** > + * Asynchronously gets all add-ons that have been uninstalled but are still in > + * the database because they are in locked install locations. Nit: This comment should only mention that the add-ons have been uninstalled, not the reasons. That they're in a locked location is an implementation detail that only XPIProvider cares about. Also, eventually, I'm thinking we could eventually use this API for bug 612168. @@ +1896,5 @@ > + let addons = []; > + > + new AsyncObjectCaller(this.providers, "getUninstalledAddons", { > + nextObject: function getUninstalledAddons_nextObject > + (aCaller, aProvider) { Nit: Don't wrap function definitions like this, it makes reading it error-prone. The 80-character line limit is a guideline, not a hard rule :) Readable alternatives include: nextObject: function getUninstalledAddons_nextObject(aCaller, aProvider) { And: nextObject: function getUninstalledAddons_nextObject(aCaller, aProvider) { ::: toolkit/mozapps/extensions/XPIProvider.jsm @@ +3460,5 @@ > + * A callback to pass an array of AddonWrappers to. > + */ > + getUninstalledAddons: > + function XPI_getUninstalledAddons(aCallback) { > + XPIDatabase.getUninstalledAddons(function (aAddons) { Nit: Name this function. @@ +3466,5 @@ > + // There may be many add-ons with the same ID in different install locations. > + // In such cases, we return only the add-on in the highest priority location. > + for (let addon of aAddons) { > + let found = false; > + for (let i in uninstalled) { If you make |uninstalled| a Set instead of an array, you won't need to loop through it here. @@ +3469,5 @@ > + let found = false; > + for (let i in uninstalled) { > + if (uninstalled[i].id === addon.id) { > + found = true; > + if (uninstalled[i]._installLocation.scope > addon._installLocation.scope) { Alas, just comparing the value of .scope won't work for a couple of reasons: * The values of the AddonManager.SCOPE_* constants aren't guaranteed to be ordered. We could add a new scope, and it would need to have a value of 16, but it could be lower priority than SCOPE_SYSTEM. * There can be multiple different locations with the same scope. So one system-scope location will have a higher priority than another system-scope location (see the calls to addRegistryInstallLocation and addDirectoryInstallLocation in XPIProvider.startup). So, instead, you need to find what position the location is in the XPIProvider.installLocations array - that's the actual priority. Probably want to spin that out to a separate method though. @@ +3490,5 @@ > + aCallback(results); > + return; > + } > + let first = uninstalled.shift(); > + AddonManager.getAddonByID(first.id, function(addon) { Just go directly through XPIDatabase to get this - we don't need to worry about other providers. Bonus points if you do it by adding a XPIDatabase.getVisibleAddonsForIDs() method - then you could use the results for that in the loop above, rather than iterating through it separately here. @@ +6344,5 @@ > + this.__defineGetter__("uninstalled", function AddonWrapper_uninstalledGetter() { > + return aAddon.uninstalled; > + }); > + > + this.__defineSetter__("uninstalled", function AddonWrapper_uninstalledSetter(val) { Most of this shouldn't be in AddonWrapper (after all, it is only meant to be a wrapper). XPIProvider itself should do the heavy lifting, such that cases like XPIProvider.uninstallAddon() shouldn't result in code in AddonWrapper being run. That might mean you could share code between normal uninstall and this type of uninstall (if it doesn't make it too complex). Thinking more about the API, I think this should be a read-only property. It's odd that you could uninstall an addon via |addon.uninstall();| and via |addon.uninstalled=true;|. Which leaves needing an API to re-install the add-on - and the AddonWrapper.reinstall() method handles that already. @@ +6356,5 @@ > + // Gets copies of the same add-on in other locked install locations and > + // marks them "uninstalled". We want this because we don't want hidden add-ons > + // in lower priority locations surfacing when an add-on of the same ID > + // is removed from a higher priority location. > + function uninstallOtherAddonCopies() { You could simplify this whole function a lot by adding a XPIDatabase.getAllAddonsForID() method. @@ +6373,5 @@ > + } > + } > + } > + } > + if (aAddon.inDatabase) { If there's no else-block for this, just negate it and return early. @@ +6390,5 @@ > + requiresRestart); > + > + XPIDatabase.setAddonProperties(aAddon, { > + uninstalled: true, > + userDisabled: true, Why is userDisabled=true needed here? @@ +6393,5 @@ > + uninstalled: true, > + userDisabled: true, > + visible: false > + }); > + AddonManagerPrivate.callAddonListeners("onPropertyChanged", this, ["uninstalled"]); No need to send this event - normal uninstalls don't do this. @@ +6473,5 @@ > if (val == this.userDisabled) > return val; > > + if (!val && aAddon.uninstalled) { > + LOG("Can't enable an uninstalled add-on."); But you can *disable* an uninstalled addon? Just remove the |!val| check, as neither case makes sense. @@ +6532,5 @@ > throw new Error("Add-on is already marked to be uninstalled"); > XPIProvider.uninstallAddon(aAddon); > }; > > + this.reInstall = function AddonWrapper_install() { Nit: reinstall Also, AddonWrapper_reinstall @@ +6534,5 @@ > }; > > + this.reInstall = function AddonWrapper_install() { > + if (!aAddon.inDatabase) > + throw new Error("Cannot install an add-on that isn't in the database."); reinstall @@ +6536,5 @@ > + this.reInstall = function AddonWrapper_install() { > + if (!aAddon.inDatabase) > + throw new Error("Cannot install an add-on that isn't in the database."); > + if (!aAddon.uninstalled) > + throw new Error("Cannot install an add-on that isn't uninstalled."); Ditto
Attachment #797974 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] from comment #51) > (In reply to Sachin Hosmani [:sachin] from comment #50) > > An add-on (in locked location) is made invisible the moment the remove > > button is pressed. For non-restartless add-ons, this will lead to the > > undo/restart UI disappearing when the list view is refreshed. Is this an > > issue? > > Hmm, doesn't seem like a show-stopper bug. Would be nice to fix though, if > it's not too complex. Is this because you're setting the visible DB field to > false immediately? Can that not be left until PREF_PENDING_OPERATIONS is > next processed on shutdown/startup? So we would need to retrieve all add-ons with "uninstalled" property = true from the database and set all of them as not visible. But the retrieval from the database is asynchronous and we can't have an async call in the middle of synchronously executing code (startup/shutdown). (In reply to Blair McBride [:Unfocused] from comment #52) > Comment on attachment 797974 [details] [diff] [review] > patch (3) > > Review of attachment 797974 [details] [diff] [review]: > ----------------------------------------------------------------- > > Making good progress :) > > ::: toolkit/mozapps/extensions/AddonManager.jsm > @@ +1877,5 @@ > > }, > > > > /** > > + * Asynchronously gets all add-ons that have been uninstalled but are still in > > + * the database because they are in locked install locations. > > Nit: This comment should only mention that the add-ons have been > uninstalled, not the reasons. That they're in a locked location is an > implementation detail that only XPIProvider cares about. Also, eventually, > I'm thinking we could eventually use this API for bug 612168. Okay. But I felt we must tell API users (or whoever is looking at the code) not to expect this method to return all uninstalled add-ons because it returns only a particular kind of them. > @@ +3466,5 @@ > > + // There may be many add-ons with the same ID in different install locations. > > + // In such cases, we return only the add-on in the highest priority location. > > + for (let addon of aAddons) { > > + let found = false; > > + for (let i in uninstalled) { > > If you make |uninstalled| a Set instead of an array, you won't need to loop > through it here. A Set wouldn't really work, would it? I instead changed it to an object with add-on IDs as keys and add-ons as values. That got rid of the iteration. :) > @@ +6356,5 @@ > > + // Gets copies of the same add-on in other locked install locations and > > + // marks them "uninstalled". We want this because we don't want hidden add-ons > > + // in lower priority locations surfacing when an add-on of the same ID > > + // is removed from a higher priority location. > > + function uninstallOtherAddonCopies() { > > You could simplify this whole function a lot by adding a > XPIDatabase.getAllAddonsForID() method. But such a method would be an asynchronous one. It would get used in XPIProvider.uninstallAddon which works synchronously. Is that ok? (and by the way a helper function called checkInstallLocation in XPIProvider.uninstallAddon is in a similar situation)
Attached patch patch (4) (obsolete) — — Splinter Review
But for those questions I asked in comment 53, I think the rest of the issues have been addressed in this patch.
Attachment #797974 - Attachment is obsolete: true
Attachment #801105 - Flags: review?(bmcbride)
Attachment #787761 - Attachment is obsolete: true
Sachin, are you likely to have more time to work on this or shall we re-assign it to someone else?
Yes, I think I should be able to finish it. I've been waiting for a review. It was mostly working well as I remember.
Hi Guys, I am new to this bug. Will your modification have a "Force Remove"? In other words, will it erase the offender's extension directory in the user's profile? Many thanks, -T
Extensions of this kind aren't installed in the profile, but elsewhere. The extension files won't be deleted, but all traces of the extension will be removed from the profile.
I have read in comment 19 : "Yes, we've long said that Firefox won't make changes to files and registry entries installed by other applications. Worst case it could actually break the other application." I consider that an external application that install something (e.g. extension or plugging)in Firefox without my authorization or my knowledge and without a "remove" button has a VERY BAD BEHAVIOUR. I should be entitled to remove physically all the file and registry from my disk as Windows does when I un-install a program. I don't want to have garbage on my disk or the risk to have the extension that restart after a new release of FF as I have already seen. If the "other application" crashes because I have erased its extension, it is its responsibility ! And the "other application" has the responsibility to correct itself and to learn to behave correctly. So I am in favour to change the development policy by erasing files and registry. This will avoid to have to manage have several levels of inactivated **** extensions...
Blocks: 1008565
Sachin, has there been any progress here?
Flags: needinfo?(sachinhosmani2)
Hey Jorge, my last patch wasn't reviewed. I'm sure the code would've changed a lot since then and I'll have to work on it again, but Blair, do we want this fixed atm?
Flags: needinfo?(sachinhosmani2) → needinfo?(bmcbride)
Sorry - yes, we want this fixed... but I just haven't been allowed any time to give this the attention it needs. I haven't forgotten it though.
Flags: needinfo?(bmcbride)
I'm currently looking at bug 879480. I'll try to get back to this when that's done.
While I do intend to fix this when I'm done with bug 879480, I don't want to stop anyone else from attending to this if it is urgent.
I had forgotten that this blocks bug 879480. Blair, what tests shall I write for this? Are UI tests going to be necessary?
Flags: needinfo?(bmcbride)
(In reply to Sachin Hosmani [:sachin] from comment #69) > I had forgotten that this blocks bug 879480. It may be easier to just split up bug 879480 - make that bug just cover installing, and file a new bug for uninstalling. That would help simplify things, I think. > Blair, what tests shall I write for this? Are UI tests going to be necessary? I think the existing UI tests should cover this fine already, so long as the XPCShell tests ensure that the API behaves the same between locally-installed and globally-installed add-ons. So you'll need XPCShell tests to cover: * Any uninstalled global add-on doesn't appear in all the usual ways to get an add-on (getAddonById, getAddonsByTypres) * Any such add-on doesn't get loaded on next startup * The API behaves the same (ie, same events are sent, pendingOperations is consistent) * The files aren't removed even if Firefox has write access to them Will need to do this for both restartless and non-restartless add-ons. You'll need to register a fake global install directory the test harness can place add-ons into. Can do that by overriding the XRESysSExtPD directory alias - see usage of registerDirectory in test_distropution.js and test_startup.js
Flags: needinfo?(bmcbride)
(In reply to Blair McBride [:Unfocused] (I don't read bugmail - needinfo? me!) from comment #70) > So you'll need XPCShell tests to cover: Er, and the new getUninstalledAddons API, of course.
Comment on attachment 801105 [details] [diff] [review] patch (4) Review of attachment 801105 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/extensions/XPIProvider.jsm @@ +3489,5 @@ > + getUninstalledAddons: > + function XPI_getUninstalledAddons(aCallback) { > + let self = this; > + XPIDatabase.getUninstalledAddons(function getUninstalledAddons_getUninstalledAddons(aAddons) { > + let uninstalled = {}; Think this function would generally benefit from making `uninstalled` a Map. @@ +3503,5 @@ > + > + let uninstalledIds = []; > + for (let id in uninstalled) { > + if (uninstalled.hasOwnProperty(id)) > + uninstalledIds.push(id); You don't really need this for block... @@ +3506,5 @@ > + if (uninstalled.hasOwnProperty(id)) > + uninstalledIds.push(id); > + } > + // We exclude all add-ons that whose copies are already installed. > + XPIDatabase.getVisibleAddonsForIDs(uninstalledIds, ... instead here you can just pass in: [...uninstalled.values()] @@ +4370,5 @@ > + > + if (!aAddon.uninstalled) > + throw new Error("Can't reinstall an add-on that hasn't been uninstalled."); > + > + let requiresRestart = XPIProvider.enableRequiresRestart(aAddon); Have to also check if there's already an enabled version of this add-on in another location. If there is, it may require a restart to disable even if this version doesn't need a restart. @@ +4385,5 @@ > + XPIDatabase.setAddonProperties(aAddon, { > + uninstalled: false > + }); > + XPIDatabase.makeAddonVisible(aAddon); > + XPIDatabase.setAddonProperties(aAddon, { Can coalesce these calls to setAddonProperties. @@ +4407,5 @@ > + } > + AddonManagerPrivate.callAddonListeners("onInstalled", wrapper, > + requiresRestart); > + } > + AddonManagerPrivate.callAddonListeners("onOperationCancelled", wrapper); You're mixing up some event flows here. onOperationCancelled should only fire if the add-on was pending uninstall. The install events should only fire if the add-on was already uninstalled. In fact, you should be able to skip a lot of this function if the add-on was pending uninstall. @@ +4408,5 @@ > + AddonManagerPrivate.callAddonListeners("onInstalled", wrapper, > + requiresRestart); > + } > + AddonManagerPrivate.callAddonListeners("onOperationCancelled", wrapper); > + flushStartupCache(); Only need to do this for restartless operations. For non-restartless, it happens automatically on next startup. @@ +6440,3 @@ > // Add-ons that are installed by a file link cannot be upgraded > if (!aAddon._installLocation.isLinkedAddon(aAddon.id)) > permissions |= AddonManager.PERM_CAN_UPGRADE; This still shouldn't apply to add-ons in locked locations. Also, I think it would be worth introducing a new permission for this: PERM_CAN_REINSTALL @@ +6465,5 @@ > return val; > > + if (aAddon.uninstalled) { > + LOG("Can't change userDisabled of an uninstalled add-on."); > + return this.userDisabled; This should throw an exception. Also, the enable/disable permissions shouldn't be included if the add-on is marked uninstalled.
Attachment #801105 - Flags: review?(bmcbride) → review-
About reinstalling global add-ons, I feel we should disallow reinstallation (both API-wise and UI-wise when it's done) when there is another copy present in a higher priority location (like user profile). As far as I understand, there can be no situation now, when an add-on in a higher priority location still exists and is disabled/invisible but a copy of it in a lower priority location is active. We don't want the higher priority add-on automatically getting visible during next startup. What do you say?
Flags: needinfo?(bmcbride)
I am not sure I have well understood comment #73 from Sachin Hosmani. I'll try with examples of legitimate sequences. Start situation : a globally installed add-on. 0) Now I install an other version (older or newer) in one of my user profiles that I use for tests. It hides the global one. 1) I test it and I am not satisfied, so I disable it in my test user profile. I don't want this higher priority add-on automatically getting visible during next startup. 2) I test it and I am satisfied, so I disable/uninstall the old global add-on. Can I install globaly the tested add-on ? What occurs for my test user profile ? I may do nothing and the version in the profile takes precedence over the equal global version. I may disable/uninstall the local version and the global one is in use. I don't want this higher priority add-on automatically getting visible during next startup (but this is harmless). 3) after 1) I, later, do again 0) but with an other version and I loop on 1) or 2). I assume this is still possible... Can I do this by re-enabling the UN-satisfactory version and using the update function to obtain the new version ? After several iterations of this process my system becomes dirty : true UN-install (when possible) is better than disabled/invisible ! In the past I had a global add-on that I cannot UN-install from the limited user accounts but using the administrator account I succeeded to UN-install it globally. This sort of try doesn't cost much but unfortunately doesn't always work ! Nevertheless it should be documented...
(In reply to Jean-Marie COUPRIE from comment #74) > Start situation : a globally installed add-on. > 0) Now I install an other version (older or newer) in one of my user > profiles that I use for tests. It hides the global one. > 1) I test it and I am not satisfied, so I disable it in my test user > profile. I don't want this higher priority add-on automatically getting > visible during next startup. Disabling the add-on won't make it invisible in the first place. It will stay visible and disabled. > 2) I test it and I am satisfied, so I disable/uninstall the old global > add-on. Can I install globaly the tested add-on ? Well, you can't move add-ons across install locations via Firefox UI. But if you have admin access to the machine, it's simply a matter of deleting the old global add-on and moving the new add-on into place (all at your own risk. it can break the third party application which installed it). Also, this really doesn't sound like a common use case. Shouldn't the third party vendor be doing these updates?
(In reply to Sachin Hosmani [:sachin] from comment #75) > (In reply to Jean-Marie COUPRIE from comment #74) > > Start situation : a globally installed add-on. > > 0) Now I install an other version (older or newer) in one of my user > > profiles that I use for tests. It hides the global one. > > 1) I test it and I am not satisfied, so I disable it in my test user > > profile. I don't want this higher priority add-on automatically getting > > visible during next startup. > Disabling the add-on won't make it invisible in the first place. It will > stay visible and disabled. I am happy with the unsatisfactory tested extension staying disabled and no strong objection to it being visible as disabled. Is the global version visible as activated. > > > 2) I test it and I am satisfied, so I disable/uninstall the old global > > add-on. Can I install globally the tested add-on ? > Well, you can't move add-ons across install locations via Firefox UI. But if > you have admin access to the machine, it's simply a matter of deleting the > old global add-on and moving the new add-on into place (all at your own > risk. it can break the third party application which installed it). Also, > this really doesn't sound like a common use case. Shouldn't the third party > vendor be doing these updates? Yes : the third party vendor should update its global (or not) extensions. The true problem occurs when the vendor has disappeared or I like no more his application or I have UN-installed the application but this has not UN-installed the global extension, and I have found something better functionally equivalent. But AFAIK, I can't globally install an extension via Firefox UI even if I have admin access to the machine : this is a limitation when the extension is needed for all the profiles of all the accounts of the machine (perhaps a new bug to fill).
(In reply to Sachin Hosmani [:sachin] from comment #73) > About reinstalling global add-ons, I feel we should disallow reinstallation > (both API-wise and UI-wise when it's done) when there is another copy > present in a higher priority location (like user profile). > > As far as I understand, there can be no situation now, when an add-on in a > higher priority location still exists and is disabled/invisible but a copy > of it in a lower priority location is active. We don't want the higher > priority add-on automatically getting visible during next startup. Yep, that's correct... for now. Eventually I'd like to change that with bug 910550 - but I don't think you need to worry about that here.
Flags: needinfo?(bmcbride)
I've written the tests, but there are some edge cases here. When I uninstall a non-restartless global add-on, it gets marked `pendingUninstall`. But since we don't iterate through inactive add-ons after restart (unless they've changed), this `pendingUninstall` flag remains on the uninstalled add-on. This shows problems when I `reinstall()` the add-on. It thinks the add-on is pending an uninstall and tries to cancel the uninstall. During startup, I think we'll have to look at every "uninstalled" add-on in db and unset its `pendingUninstall` property should it be set. What do you think? Also, the current code has no way of knowing that after a `reinstall()` of an uninstalled non-restartless global add-on, there should be a "pending install" flag set on it. But since there is no `pendingInstall` property (it decides it based on other properties), will I have to add one for this? Is there any other way?
Flags: needinfo?(bmcbride)
(In reply to Sachin Hosmani [:sachin] from comment #78) > During startup, I think we'll have to look at every "uninstalled" > add-on in db and unset its `pendingUninstall` property should it be set. > What do you think? That sounds reasonable. This would only have to happen when we startup with the PREF_PENDING_OPERATIONS pref set to true - and at which point, we're already checking add-ons (and have the DB loaded) anyway. > Also, the current code has no way of knowing that after a `reinstall()` of > an uninstalled non-restartless global add-on, there should be a "pending > install" flag set on it. But since there is no `pendingInstall` property (it > decides it based on other properties), will I have to add one for this? That also sounds reasonable.
Flags: needinfo?(bmcbride)
Attached patch patch (5) (obsolete) — — Splinter Review
As per our emails, I've added a preference for pending property resets during startup. In head_addons.js it is required that onExternalInstall be associated with restartless add-ons only. I couldn't understand why. So I removed that assertion for this bug.
Attachment #801105 - Attachment is obsolete: true
Attachment #8575300 - Flags: review?(bmcbride)
Comment on attachment 8575300 [details] [diff] [review] patch (5) Redirecting to Dave (who I owe many beverages) - too swamped to be able to get to reviews unless they pertain to a certain deadline :\
Attachment #8575300 - Flags: review?(bmcbride) → review?(dtownsend)
Comment on attachment 8575300 [details] [diff] [review] patch (5) Review of attachment 8575300 [details] [diff] [review]: ----------------------------------------------------------------- As a preface taking over a review that has already been back and forth a few times presents some challenges. Particularly I'm not going to spend the time reading the entire bug history to see everything that has been discussed before so if I complain about something that you and Blair have already agreed upon please push back on me and point me to the discussion. Blair is the owner so he gets final say on stuff. The tests are missing so I'd like to see those before completing this review. I'm posting these comments as a checkpoint but I'd suggest uploading the patch with the tests and waiting for me to complete the review of that before you start addressing them. ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ +2286,5 @@ > + throw new Error(aInstallLocationName2 + " is not a valid install location."); > + let location1 = this.installLocationsByName[aInstallLocationName1]; > + let location2 = this.installLocationsByName[aInstallLocationName2]; > + let result = this.installLocations.indexOf(location2) - this.installLocations.indexOf(location1); > + return (result === 0) ? result : (result / Math.abs(result)); Generally comparator methods like this only need to return negative, positive or 0, the actual value doesn't matter. So no need to divide and just compare against 0 throughout. @@ +3052,5 @@ > } > } > } > > + // If add-on was marked uninstalled and was pending uninstall last time, let's Nit: whitespace at end of line @@ +3059,5 @@ > + XPIDatabase.setAddonProperties(aOldAddon, { > + pendingUninstall: false > + }); > + } > + // If add-on was reinstalled and was pending reinstall last time, let's Nit: whitespace at end of line @@ +3915,5 @@ > + * @param aCallback > + * A callback to pass an array of AddonWrappers to. > + */ > + getUninstalledAddons: > + function XPI_getUninstalledAddons(aCallback) { There's an additional check we need in this function. If there is an installed add-on then we shouldn't be returning any uninstalled add-ons from lower priority install locations as the user can't reinstall those. Or I guess they can but it won't actually change anything so I don't think it's worth introducing the confusion by showing them to the user. @@ +3917,5 @@ > + */ > + getUninstalledAddons: > + function XPI_getUninstalledAddons(aCallback) { > + let self = this; > + XPIDatabase.getUninstalledAddons(function getUninstalledAddons_getUninstalledAddons(aAddons) { Oh for promises in this code. Still you can use fat arrow instead to avoid needing to define self: XPIDatabase.getUninstalledAddons(aAddons => { ... }); @@ +4722,5 @@ > + function notifyAddonChangesAndCallCallback() { > + if (aAddon.type == "theme" && aAddon.active) > + AddonManagerPrivate.notifyAddonChanged(null, aAddon.type, requiresRestart); > + aCallback && aCallback(true); > + } I'm not seeing any benefit to breaking this out into a function and calling it all over the place. We want to be sure it is always called at the end of uninstallAddon so just leave the code there. @@ +4784,5 @@ > + XPIDatabase.setAddonProperties(aAddon, { > + visible: false, > + active: false, > + uninstalled: true > + }); This should only happen if !requiresRestart. Put it by the test for lockedLocation below and make sure we have a test that trying to get an add-on that is in a locked location and pending uninstall works. @@ +4794,5 @@ > + }); > + // Decrement PREF_PENDING_PROPERTY_RESETS by 1 because it's no longer needed. > + Services.prefs.setIntPref(PREF_PENDING_PROPERTY_RESETS, > + Preferences.get(PREF_PENDING_PROPERTY_RESETS, 0) - 1); > + } Space after here for readability. Please do this after most multi-line if blocks @@ +4856,5 @@ > + if (aAddon.uninstalled) { > + // This was a speical type of uninstallation because the add-on is in a > + // locked location. > + XPIDatabase.setAddonProperties(aAddon, { > + visible: true, The fact that we check the visible property below suggests that there might be a case where we call this for an add-on that isn't in the highest install location. We should check that before blindly marking this one visible. @@ +4870,5 @@ > > // TODO hide hidden add-ons (bug 557710) > let wrapper = createWrapper(aAddon); > AddonManagerPrivate.callAddonListeners("onOperationCancelled", wrapper); > In uninstallAddon you mark all the lower priority versions of this add-on uninstalled, you need to clear that here. @@ +4883,5 @@ > + * > + * @param aAddon > + * The DBAddonInternal to reinstall. > + * @param aCallback > + * Callback function to call when done. Nit: whitespace at the end of the line. @@ +4902,5 @@ > + "a higher priority location."); > + aCallback && aCallback(false); > + return; > + } > + let existingAddonRequiresRestart = Boolean((aExistingAddon && !aExistingAddon.userDisabled && No need for Boolean here @@ +4907,5 @@ > + XPIProvider.disableRequiresRestart(aExistingAddon))); > + > + // The reinstall requires restart also when the existing add-on needs a > + // restart to be disabled. > + let addonRequiresRestart = Boolean(XPIProvider.installRequiresRestart(aAddon) || Nor here. Name this reinstallRequiresRestart @@ +4959,5 @@ > + > + if (!addonRequiresRestart) { > + if (aAddon.bootstrap) { > + let file = aAddon._installLocation.getLocationForID(aAddon.id); > + XPIProvider.callBootstrapMethod(aAddon, file, "install", If there's an existing add-on we should be using the upgrade/downgrade code @@ +4967,5 @@ > + } > + AddonManagerPrivate.callAddonListeners("onInstalled", wrapper, > + addonRequiresRestart); > + } > + if (addonRequiresRestart) else @@ +4970,5 @@ > + } > + if (addonRequiresRestart) > + flushStartupCache(); > + aCallback && aCallback(true); > + }); You should be reinstalling all add-ons in lower install locations here too. @@ +6755,5 @@ > permissions |= AddonManager.PERM_CAN_DISABLE; > } > } > > + // Add-ons that are are pending uninstall Keep the rest of the comment here ::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini @@ +242,5 @@ > # Bug 676992: test consistently fails on Android > fail-if = os == "android" > [test_types.js] > [test_uninstall.js] > +[test_uninstall_global_addons_restartless.js] I don't see these tests in the patch @@ +243,5 @@ > fail-if = os == "android" > [test_types.js] > [test_uninstall.js] > +[test_uninstall_global_addons_restartless.js] > +run-sequentially = Uses global XCurProcD dir. There shouldn't be any reason for this, see http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_startup.js#102 for how to redefine some of the locked install locations to directories we can play with safely
Attachment #8575300 - Flags: review?(dtownsend)
Attached patch patch (6) — — Splinter Review
Sorry about the missing tests. I have no idea why they didn't appear.
Attachment #8575300 - Attachment is obsolete: true
Attachment #8578726 - Flags: review?(dtownsend)
Comment on attachment 8578726 [details] [diff] [review] patch (6) Review of attachment 8578726 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the long delay here, I've been trying to get hold of Unfocused to discuss some changes but apparently he's out for a while so I'll just go ahead and make the decision on his behalf. There are a few inconsistencies with this patch. Particularly when uninstalling a locked add-on we mark all versions in all install locations uninstalled. When reinstalling we only reinstall the highest priority add-on. Similarly when a user installs a fresh version of the add-on the locked versions remain uninstalled and won't be revealed if the new version is dropped from the filesystem. The other oddity is this. A user has an add-on in location A, B and C where locations A and B are locked. User uninstalls add-on from C, this causes us to reveal the add-on in location B, user is confused. Then user "uninstalls" add-on from B and it causes A to be "uninstalled" too. User is happy. We reveal add-ons when uninstalling right now because there is no way to hide them but now we can do that we should "uninstall" A and B when uninstalling C too. I think the real model we want is this. When a user chooses to uninstall an add-on through the UI, the add-on goes away, any versions in non-locked locations (we can rely on there being only one I think) are removed from disk and any other versions are hidden. If a user re-installs an add-on from a locked location or installs it fresh from the web the add-on comes back again. As long as an add-on is visible if the visible version is removed from the filesystem when we do the startup scan we reveal the lower priority version as we do currently. Now you can amend the current patch fairly easily to do that but I'm concerned by the uninstalled property for each add-on instance. It can easily get out of sync leading to problems like those I describe above. A better choice is to instead add a new map to the JSON database of uninstalled add-ons. When the user uninstalls just mark that ID as uninstalled. When the add-on is re-installed clear it. Sorry that this is asking for more churn in this already long bug but I think we're really close to an awesome user experience here.
Attachment #8578726 - Flags: review?(dtownsend) → review-
Any progress? This one really makes me mad. Just had a lengthy discussion in Firefox IRC about this behavior. Currently Firefox a) allows shady extensions to be installed globally without requiring any action or user consent in the Firefox browser and b) offers no solution to remove such an extension after not prefenting installation in the first place. There are several options to improve the situation. 1. Ask the user if extension should be installed 2. allow removing global extensions (what this exact bug report is requesting) 3. as long as #2 is not in place, at least offer a link to the file to be removed in the systems file browser (this could be done via link in the details page of the global extension, or if mozilla wants to hide it on the surface, about:support, which lists extensions could have a right-click option for each extension so the user has quick access to the files in question) Not only do I think mozilla should be more protective of it's users, also could this be a real security risk when the user installs shady software. While the system then might be compromised already, I find the logic that then to compromise the browser as well, is just collateral damage, to be fatal and wrong. This bug has been untouched for over 6 months. Has any progress been made? What are the steps to escalate this or increase importance? Currently 27 cc: humans tell me, I am not the only one worried or annoyed by the current behavior.
(In reply to steve_-_ from comment #86) > Any progress? This one really makes me mad. Just had a lengthy discussion in > Firefox IRC about this behavior. Currently Firefox a) allows shady > extensions to be installed globally without requiring any action or user > consent in the Firefox browser and b) offers no solution to remove such an > extension after not prefenting installation in the first place. > > There are several options to improve the situation. > > 1. Ask the user if extension should be installed We already do require the user to explicitly enable extensions installed in this manner. I know that that isn't ideal since it means the disabled extension still shows up in the add-ons manager list but to all intents and purposes it has the same effect as not installing the extension, the extension's code doesn't run. If an app is bypassing this enable prompt then you should file a bug to get the offender blocklisted. > Not only do I think mozilla should be more protective of it's users, also > could this be a real security risk when the user installs shady software. > While the system then might be compromised already, I find the logic that > then to compromise the browser as well, is just collateral damage, to be > fatal and wrong. We're taking other steps like requiring all add-ons to have at least a minimal code review before it will be allowed to run in Firefox. > This bug has been untouched for over 6 months. Has any progress been made? > What are the steps to escalate this or increase importance? No progress has been made, no-one is actively working on this bug. It would be up to the product managers to decide that this was more important than we've so far considered it to be.
Assignee: sachinhosmani2 → nobody
Status: ASSIGNED → NEW
(In reply to steve_-_ from comment #86) > Any progress? This one really makes me mad. Just had a lengthy discussion in > Firefox IRC about this behavior. Currently Firefox a) allows shady > extensions to be installed globally without requiring any action or user > consent in the Firefox browser and b) offers no solution to remove such an > extension after not prefenting installation in the first place. Hi Steve, Firefox is very easy to infect. Just drop a JS over in C:\Program Files\Mozilla Firefox\browser\components. See https://bugzilla.mozilla.org/show_bug.cgi?id=1169417 That bug and this one are just an ongoing problem with Firefox security. I wish they would get moving on both of these. -T
Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Resolution: INACTIVE → ---

Hey Mike, do we plan to ship something like this via enterprise policy?

Flags: needinfo?(mozilla)
Priority: -- → P2

If an addon is installed in a real global/third party location (outside of Firefox), there really isn't much we could do, because we wouldn't have the file permissions to uninstall it.

You do being up an interesting point, though, which is that if a global addon is blocked via policy, it should be force disabled.

I've opened bug 1564985 for that.

I think this bug can be closed.

Flags: needinfo?(mozilla)

We recently shipped abuse reporting in Firefox, which introduces abuse reporting categories, and the “Wasn’t wanted / impossible to get rid of” category is by far the top one. I haven't caught up with the whole discussion, but at least having a way to remove the extension from the Extensions pane would go a long way in making users feel in control of their experience.

I'll bring this up with the rest of the add-ons team to see if it can be addressed sooner rather than later.

Sideloading has been disabled in general for these locations, and remove is now available to the user which removes it from Firefox. If a distribution (e.g. esr or linux) enables sideloading, the old behavior remains, but IMO this is fine. Closing this but linking to the sideload meta bug.

Depends on: 1602839
Status: REOPENED → RESOLVED
Closed: 6 years ago → 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: