Closed Bug 1313298 Opened 4 years ago Closed 2 years ago
Installing an extension that is already installed but disabled should prompt to enable it
193.77 KB, image/png
7.80 MB, video/mp4
59 bytes, text/x-review-board-request
85.81 KB, image/png
46 bytes, text/x-phabricator-request
|Details | Review|
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0) Gecko/20100101 Firefox/52.0 Build ID: 20161008030200 Steps to reproduce: 1. Disable extension 2. Page can not find the extension and trigger to reinstall extension 3. After installed, the extension status was disable also, not same as Chrome, the extension will be enable auto. Actual results: The extension is not enabled. Expected results: The extension should be enabled same as Chrome.
With which extension did you meet this issue?
Component: Untriaged → Add-ons Manager
Product: Firefox → Toolkit
We explicitly test for the current behavior (preserving the disabled status) and have been for the last 6 years: http://searchfox.org/mozilla-central/rev/445097654c1cc1098ee0171a29c439afe363a588/toolkit/mozapps/extensions/test/xpcshell/test_bootstrap.js#755-756 Perhaps if we handle an InstallTrigger for an installed but disabled extension we could just present a notification to the user that explains the situation and asks if they want to re-enable. Markus, what do you think?
(In reply to Loic from comment #1) > With which extension did you meet this issue? cisco webex extension: https://addons.mozilla.org/en-US/firefox/addon/cisco-webex-extension/
agreed that current behavior not desired. need a message for how to clarify existing notification so it give the user the clear scenario (when they have but disabled). change notification from current install notification to something more like.... "do you want to enable and install this" - but in a better UX way :). We have the notification - but just need to word it better to ask about enabling this.
Priority: -- → P2
Whiteboard: UI needed
We can run the regular install flow, but change the install-dialog to something like shown in the attachment. I will check copy with Scott when we review all permission install copy. Good catch of a use-case we missed when building out the new install flow including permissions. I will add it to the doc.
clearing old ni
Severity: normal → minor
Whiteboard: UI needed → triaged
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: extension reinstall cant enable auto → Installing an extension that is already installed but disabled should prompt to enable it
Note, copy for the dialog is in the doc attached to bug 1316996
Whiteboard: triaged → [permissions] triaged
Not sure why this got marked as blocking permissions, its a separate issue.
No longer blocks: webext-permissions
I just ran into this issue on 60.0b4
Would you consider implementing something like the attached re-enable prompt provided by Markus J? I think he has come up with a good idea but seems it was left for dead.
This is something we would like to do but it has not been a high priority. If there are any contributors in the community who would like to work on it, we can provide assistance and would welcome patches.
Working with a team to provide a patch. We've found it's easiest to follow the same workflow as Chrome and re-enable the addon after the user has selected "Allow" and "Add" (no additional re-enable prompt as the user has accepted the addon). Would this be a patch that would be accepted by Mozilla?
I'm not sure exactly what you were asking in the previous question, the desired flow is described in comment 16
whoops, the desired flow is described in comment 5
Thanks for the quick response. Not sure I follow on the previous question, that was my first comment on this thread :) Adding the attached prompt from comment 5 would definitely add more work towards building the patch, is it a requirement given the permissions from the "Add" Addon prompt are the same as the Re-enable prompt?
Sorry, I still don't understand what you're proposing. If you have a patch ready you can just attach it here. If not, perhaps you could just describe step-by-step and in detail the alternative that you're working on.
See example of re-enabling a disabled extension in Chrome on Momentum.
No patch yet as we're wanting to check with you before building :) Step by Step User Workflow: 1) User downloads Addon which had been previously disabled 2) User selects "Allow" from AMO prompt 3) User selects "Add" from permissions request prompt 4) Addon is re-enabled See attached video of the same workflow on Chrome.
Ah, so comment 5 is not asking to add another dialog, it is asking to change the wording of the current install dialog so that in the event the extension is already installed but disabled, the wording of the heading and the button change from "Add" to "Re-enable". The result would be the same as what's shown in the Chrome video, but with clearer wording in the dialog.
Thanks for clearing that up, would that be a requirement for the patch or can the "Add" remain unchanged as it does with Chrome?
(In reply to Alex J from comment #24) > Thanks for clearing that up, would that be a requirement for the patch or > can the "Add" remain unchanged as it does with Chrome? It can. I believe, re-enabling on repeated install, without any UI changes will be an improvement to what we have now. (But it will miss to give the user context on why this install might behave slightly different from a regular install.)
Markus & Andrew S sorry for the delay. Thank you for responding, we've met and we're able to move forward with creating a patch to re-enable the extension as long as we don't have to make the UI changes / pop-up the re-enable prompt. I'll make sure to update when we start working on it and can provide an ETA. Thanks again!
Hi Andrew and Markus, we're hard at work on the patch, could you tell us when it needs to be done by to get into FF62?
You probably have to get it in before the Soft Code Freeze, which is June 14th. https://docs.google.com/document/d/17FvsBOudLr15DgN-E82UamLhnweDHmGzQu4wIEi1QUg/edit#
Thanks Marcus! We'll see what we can do.
Hey guys. Would you mind reviewing the following patch https://reviewboard.mozilla.org/r/250724/ for the issue above? Just published it and assigned :aswan as a reviewer (per suggestion from IRC #introduction channel). Let me know if you have any questions, and whoever who is entitled to assign tickets, please assign me to this one. Thanks!
Hi again, guys! Marcus, Andrew, assuming that code freeze for FF62 is planned on Jun 14 (and it's basically tomorrow), are there any chances for the patch above to be reviewed and included in the build? Thanks.
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review257012 Artyom, thank you very much for the patch. I have a few comments: 1. I'm a little confused about the desired outcome here. The way this patch is written, re-installing the same version of a an already-installed-but-disabled extension will cause it to be re-enabled, but manually installing a different version will not. Handling the latter case would entail modifying the code that actually launches an install and would be a relatively larger change. But this is a somewhat quirky behavior. 2. Assuming we're okay with the behavior described, I think we can do this with less special-case code. Confusingly, the userDisabled bit is actually propagated across a re-install here: https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/toolkit/mozapps/extensions/internal/XPIDatabase.jsm#529 I believe we can just apply the if-this-is-a-reinstall logic there and leave the rest of the install flow unchanged. 3. This change should break some existing tests that will need to be updated. We should also have a new test to verify that the desired reinstall behavior is working correctly. There is an overview of running tests locally here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Running_automated_tests or you can use the try server: https://wiki.mozilla.org/ReleaseEngineering/TryServer
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review257012 Hi Andrew, I think that #1 is fine for now. The logic means enabling addon back if users try to reinstall exactly the same addon. Regarding #2 I'm not sure that I can do that just by replacing the logic I wrote with `if-reinstall-then-set-userDisabled-false` in XPIDatabase.updateBlocklistState function here (https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/toolkit/mozapps/extensions/internal/XPIDatabase.jsm#529 as you suggested). Because in that case FF won't turn the features declared inside chrome-settings-override section in manifest file on (e.g. custom search provider). Actually, if user enables addon back from about:addons page, exactly the same function `XPIDatabase.updateAddonMetadata` (https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/toolkit/mozapps/extensions/internal/XPIDatabase.jsm#2174) is running behind. This function updates addon's metadata in DB and then run bootstrap.startup with BOOTSTRAP_REASONS.ADDON_ENABLE reason that eventually leads to calling `Extension.jsm/startup` method (https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/toolkit/components/extensions/Extension.jsm#1689) that is running through manifest file and utilizing all the features declared in it. But if I run `XPIDatabase.updateAddonMetadata` in the place you suggested to put the logic, in that case it would run some redundant logic which is responsible for upgrading extension there (https://searchfox.org/mozilla-central/rev/d544b118e26422877108c42723b26e9bb4539721/toolkit/mozapps/extensions/internal/XPIInstall.jsm#1774), is that fine? Regarding #3, would you mind suggesting where the tests connected to this installation flow reside? And what's the test runner run them? Because after running `/.mach help` I can see many different test commands available, that are running different type of tests. What's particular place I need to look at? And what's the command is responsible for running those tests?
(In reply to Artyom Pranovich from comment #34) > Regarding #2 I'm not sure that I can do that just by replacing the logic I > wrote with `if-reinstall-then-set-userDisabled-false` in > XPIDatabase.updateBlocklistState function here > (https://searchfox.org/mozilla-central/rev/ > d544b118e26422877108c42723b26e9bb4539721/toolkit/mozapps/extensions/internal/ > XPIDatabase.jsm#529 as you suggested). Because in that case FF won't turn > the features declared inside chrome-settings-override section in manifest > file on (e.g. custom search provider). Hm, that looks like a separate bug in the chrome settings handling. Mike, it doesn't look like the chrome settings API handles extension upgrades, is there a bug for that / are you aware of it? In any case, this is where a new unit test would help, as it would clarify the intention about exactly what events and bootstrap methods should be triggered and in what sequence. As it stands right now, if a user re-installs an existing extension, the browser shuts down the extension, calls either update() or uninstall() followed by install(), then starts up the extension. This happens when the user re-installs the exact same version which is a little quirky but its also not a scenario that justifies writing more code to implement some different behavior. However, if we want this case to just re-enable the existing extension, then the patch should be revised to avoid trigger onInstalling and onInstalled listeners and to avoid doing any I/O (ie, no calls to installAddon() ) > Regarding #3, would you mind suggesting where the tests connected to this > installation flow reside? And what's the test runner run them? Because after > running `/.mach help` I can see many different test commands available, that > are running different type of tests. What's particular place I need to look > at? And what's the command is responsible for running those tests? You could add a new test case to toolkit/mozapps/extensions/test/xpcshell/test_install.js but that file is already pretty lengthy, I think a new test in that directory would be ideal. There's a basic overview of the xpcshell test harness here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
We don't allow upgrades to install search engines. That was by design, because an extension could keep changing/upgrading in order to get user to try to install the engine.
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review257012 Hey guys. I just made an update to my initial changes based on Andrew feedback. Unfortunately I wasn't able to publish my updates with Andrew only in reviewers (since bugzilla said that he is on PTO until Jun 25 and publishig was failing), so I added user :kmag (per suggestions from IRC #introduction channel). I did it because I'll be on PTO Mon-Wed next week too, so won't able to pusblish updates and don't wanna miss 3 potential days for review. When Andrew is back please feel free to remove :kmag reviewer if he is unrelevant here :) Let me provide a recap on the changes done. 1. If user intentionally reinstalls disabled extension of the SAME version installed, we enable this extension back and trigger `onEnabling` and `onEnabled` (instead of `onInstalling` and `onInstalled`) listeners w/o any I/O actions. We don't reinstall the extension (and actually don't run any new logic) in case if it was marked pending for uninstalling. 2. If user intentionally reinstalls disabled extension of DIFFERENT version installed, it's actually an update from browser perspective, but we need to make sure that extension is enabled back, so `userDisabled` value is set to `false` if all conditions are met (see my changes at XPIDatabase.jsm) and propagated further for update. In that case we trigger `onInstalling` and `onInstalled` listeners with all I/O operations needed (actually the logic is the same as it used to be for regular install/update). 3. If disabled extension is trying to be updated by automatic browser background check - my logic doesn't run and the state of extension is remaining the same (actually I'm not even sure that this case is possible, but I added the check for that and wrote few tests for this particular case). So basically only intentional user actions might lead to reenabling extension. Regarding the features inside `manifest.json` file. 1. If disabled extension of the same version was reinstalled by user, we reenable all the `chrome_settings_overrides` along with `chrome_url_overrides` (e.g. default search engine and new tab page). 2. If disabled extensions was uppgraded to different version we don't reenable search engine (like :mkaply mentioned), but only new `chrome_url_overrides/new tab`. Actually I didn't change anything connected to this, all the logic is the same as it worked before my changes, but I just added few appropriate tests for that. Please note that I run all the relevant tests and fixed failed ones, and also created some new tests related to my logic added. Hope everything above makes sense. Let me know if you have any questions and please provide the feedback on that. P.S. just wondering is there still any way to get into FF62 build with that patch if it's approved? Thanks.
Hi everyone! Andrew, did you have a chance to take a look at my changes? Looking forward to hearing your thoughts. Thanks.
Hi, does anyone know what the status of this bug is? Looks like a patch was submitted and resubmitted a few weeks ago, will it be in FF62?
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review263818 Sorry this review took so long, it fell through the cracks as it passed back and forth between Kris and me. There are a few stylistic issues, but I'd like to concentrate on the big issues first. We've already got a situation where `updateBlocklistState()` is heavily overloaded. This code could use an overhaul but that's out of scope for this bug, instead lets just pull out the code that copies properties from an existing addon -- basically the code that handles `options.oldAddon` and the callers that pass that option. Instead, lets have a new method called something like `AddonInternal.propagateDisabledState()` that copies those properties and then calls `updateBlocklistState()`. Then the special handling needed for this bug can just be done at the appropriate point inside XPIInstall.jsm. Also, I didn't trace the logic in your patch too closely but something's wrong given the test changes included in this patch. I thought the desired behavior was that only if the same version was re-installed would a disabled extension be re-enabled, but it appears that you've made this apply to installing any version?
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review263818 Hey Andrew. Thanks for your feedback. Addressing your concern about installing addon of different version, please read through my comment above. It only happens if user manually (and consequently intentionally) initiates installation process. So all of my changes are connected to user intentional reinstalls of disabled addon. I think it makes sense, since there is highly probable case when user disabled addon for some reason, then time passed, newer version of addon was released, and user decided to install it again. In that case with my changes applied extension should be enabled back. I outlined everything above in my previous comment in details, so please check it again. I hope that is reasonable, but if you have any objections please raise them and we can discuss. Once we agree on the main logic for enabling different/same versions, I'll address your concern with overloaded `updateBlocklistState()` function. Let me know if you have any questions. Thanks, Artyom.
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review263818 Hi Andrew, Did you have a chance to run over my comment above, and the one on the top 5 weeks ago where I explained all my changes and the logic behind it? Looking forward to hearing feedback from you and happy to answer your questions and digging into details if needed. Thanks.
Attachment #8984953 - Flags: review- → review?(aswan)
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review263818 Andrew :aswan, I haven't heard anything from you for a while. Would you be able to review, or it makes sense to assign another reviewer? Thanks!
Comment on attachment 8984953 [details] Bug #1313298. Allow to reinstall extensions that were disabled by user, if user wants to do it. https://reviewboard.mozilla.org/r/250724/#review268394 Please use the needinfo flag if you have questions. Sorry I overlooked the comment in which you revised the behavior. It makes me pretty uneasy, there are many paths by which we can get into the addon installation code, and nothing here ensures that the re-enable logic is *only* run for user-initiated installs. Moreover, we have no way to guard against a future regression causing the re-enable logic to be run at the wrong time. In addition, there are some corner cases that need some thought. Part of the problem with making this change without the UI portion is that its unclear what we should do with soft-blocked addons. In the absence of UI, I don't think they should be re-enabled upon an upgrade. All things considered, I think that handling this from the front-end with appropriate UI (as originally specified in the bug) is probably the best path.
Hi Andrew, Just to be on the same page I created simple flow diagram explaining new changes added. Looking at the following statement from your comment above > In the absence of UI, I don't think they should be re-enabled upon an upgrade. makes me think that it's still unclear for you that automatic background update *won't reenable extension back*, so basically extension will be at the same disabled state after background check runs. That said again I wanna clarify that *only intentional user actions* might lead to reinstalling/reenabling an addon. Would you mind reviewing diagram I attached to make understanding of the logic easier? Hope it makes sense. Artyom.
(In reply to Artyom Pranovich from comment #46) > Looking at the following statement from your comment above > > In the absence of UI, I don't think they should be re-enabled upon an upgrade. > makes me think that it's still unclear for you that automatic background > update *won't reenable extension back*, so basically extension will be at > the same disabled state after background check runs. No, that comment was specifically about soft-blocked addons. I don't think that installing a new version of something that is soft-blocked should automatically re-enable it. > That said again I wanna clarify that *only intentional user actions* might > lead to reinstalling/reenabling an addon. I agree with the intention, the concern I expressed in comment 45 is that I think its very difficult to really ensure that this patch correctly implements that behavior (and moreover, to guard against future regressions that change that behavior)
Andrew, addressing your concern about soft-blocked addons, am I correct that `updateBlocklistState()` function already has the logic that ensures soft-blocked addons to be not enabled back, since `softDisabled` property is propagated further. > this.softDisabled = oldAddon.softDisabled; I just actually checked this logic, manually changing this prop to `true` in debugger mode and I can confirm that addon is still disabled after reinstall (if it was soft-blocked before and user manually tried to reinstall it). Just keep in mind that logic I've added is only connected to `userDisabled` property. Addressing another concern below, > there are many paths by which we can get into the addon installation code, and nothing here ensures that the re-enable logic is *only* run for user-initiated installs Am I right that the only way for extension to be installed w/o user intention is browser background check and subsequent update? If yes, so update is always get through `XPIInstall.createUpdate()` and there I ensured that appropriate param (`browserAutoAddonUpdate: true`) is passing further and to be used in `updateBlocklistState()`. And aren't the tests we have are responsible for guarding against future regression? And basically to make us sure that my changes don't break anything? Would you mind clarifying on that please? But still, if you have reasonable concerns to disallow reinstall of different addon versions, probably we can change the patch allowing only the same version and will create separate issue and consequently new patch for discussing the reinstall of different version in future? Thanks.
(In reply to Artyom Pranovich from comment #48) > Andrew, addressing your concern about soft-blocked addons, am I correct that > `updateBlocklistState()` function already has the logic that ensures > soft-blocked addons to be not enabled back, since `softDisabled` property is > propagated further. > > this.softDisabled = oldAddon.softDisabled; > I just actually checked this logic, manually changing this prop to `true` in > debugger mode and I can confirm that addon is still disabled after reinstall > (if it was soft-blocked before and user manually tried to reinstall it). > Just keep in mind that logic I've added is only connected to `userDisabled` > property. I was looking at the test changes you made here: https://reviewboard.mozilla.org/r/250724/diff/2#index_header > Addressing another concern below, > > there are many paths by which we can get into the addon installation code, and nothing here ensures that the re-enable logic is *only* run for user-initiated installs > Am I right that the only way for extension to be installed w/o user > intention is browser background check and subsequent update? Perhaps, answering that question definitively would entail auditing all the code that can start an install and confirming that each of them is really originating from an explicit user action. > If yes, so > update is always get through `XPIInstall.createUpdate()` and there I ensured > that appropriate param (`browserAutoAddonUpdate: true`) is passing further > and to be used in `updateBlocklistState()`. > And aren't the tests we have are responsible for guarding against future > regression? And basically to make us sure that my changes don't break > anything? I'm not fond of this approach since the current model (for better or for worse) is that these sorts of decisions happen in whatever front-end code is creating the install object. That model has flaws, to be sure, but I think that not following the existing model is going to be worse for overall maintainability. > But still, if you have reasonable concerns to disallow reinstall of > different addon versions, probably we can change the patch allowing only the > same version and will create separate issue and consequently new patch for > discussing the reinstall of different version in future? That's fine with me if you prefer to approach it that way.
Attachment #9004521 - Attachment description: Bug 1313298. Reinstall disable extension of the same version. r?aswan → Bug 1313298. Reinstall disable extension of the same version.
Comment on attachment 9004521 [details] Bug 1313298. Reinstall disable extension of the same version. Andrew Swan [:aswan] has approved the revision.
Attachment #9004521 - Flags: review+
I just pushed this to try on behalf of Artyom: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5e0641322619278c1a3516d20cf4abb43533127
Hey Andrew, Would you mind looking at my latest comment at https://phabricator.services.mozilla.com/D4435 I'm wondering whether it's acceptable thing or not.
Artyom, I'm not sure off the top of my head what's going wrong. The window file system uses different locking semantics than Linux/Mac, but I don't see an obvious problem. I'll try to take a closer look some time today...
Congratulations, you found another obscure bug (bug 1492352). I've got a fix here that I'll push now. In the mean time, your patch is based on a very old copy of mozilla-central, can you pull a more recent copy, rebase the patch, and upload a more recent version to phabricator? Barring any surprises, we should be able to clear this up and get your patch landed on Wednesday.
Now build is green https://treeherder.mozilla.org/#/jobs?repo=try&revision=af7bf1487d8ce75f96d1e94e558919af862f2a08, so I'm adding `checkin-needed` keyword.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/f88f9b07b082 Reinstall disable extension of the same version. r=aswan
Thanks so much for the patch, Artyom! Your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition#September_2018 Would you be interested in creating a profile on mozillians.org? I'd be happy to vouch for you!
Hi Caitlin! Yes, please, it would be awesome! Would you mind providing some instructions what I need to do to become a part of community.
Yay! I have good news -- you are already part of the community! :) To create a mozillians account, go to https://mozillians.org/ and click on "Log In/Sign Up." Once your account has been created, ping me and I will vouch for you. :) You can also check out other ways to get involved with the community at our wiki: https://wiki.mozilla.org/Add-ons/Contribute We're happy to have you on board!
Flags: needinfo?(cneiman) → needinfo?(artyom.pranovich)
Here we go, this is my account - https://mozillians.org/en-US/u/apranovich/ Thanks :)
I've manage to reproduce this issue on Firefox 54.0b9 using Windows 10 64bit. This is verified fixed using Firefox 64.0b4 (BuildId:20181025233934) on the following OSes: Windows 10 x64, Ubuntu 16.04 x86 and macOS 10.13.6.
You need to log in before you can comment on or make changes to this bug.