851936 - allow uninstall of builtin providers

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[WildcatRay]

Currently, I see Facebook Messenger for Firefox listed under the Services tab of about:addons. Unlike addons, it cannot be deleted by the user there.

Also, any instructions that claim to remove fb messenger only disable it. As someone who does not use facebook, I have no desire to even see anything facebook listed anywhere in my browser.

I did succeed in deleting it by deleting the entries in the prefs social.manifest.facebook and social.provider.current.

Users should not only be able to disable fb messenger, but also "delete" it by having a button--labeled either Remove or Delete--that will delete the information in the appropriate pref rather than having to go into about:config with the potential risk of going that route.

Comment 1

[Sid]

Deleting all social providers should also hide Services tab in Add-ons Manager, like Languages tab is hidden when no langpack is installed.

Comment 2

[WildcatRay]

(In reply to Sid from comment #1)
> Deleting all social providers should also hide Services tab in Add-ons
> Manager, like Languages tab is hidden when no langpack is installed.

I can't confirm or deny that is what is supposed to happen as the Services tab remains after clearing the entries in the perfs I mentioned in my initial comment.

On another note, rather than "hard coding" the information in a pref like social.manifest.facebook, why not handle this like either installing an addon or search provider? Users could either enable the Social API or just go to the web site they choose and have the necessary code and/or information "installed" into the browser and off they go. And, much like other addons, they can choose to uninstall it and no longer have to deal with it being there.

If Firefox went this route, it would be something users are familiar with and used to. Also, they would have the control over it that they should have.

Comment 3

[Sid]

(In reply to Ray Murphy (WildcatRay) from comment #2)
> (In reply to Sid from comment #1)
> > Deleting all social providers should also hide Services tab in Add-ons
> > Manager, like Languages tab is hidden when no langpack is installed.
> 
> I can't confirm or deny that is what is supposed to happen as the Services
> tab remains after clearing the entries in the perfs I mentioned in my
> initial comment.

It's just my proposal what _should_ happen.

Comment 4

[WaltS48 [:walts48]]

Platform should be All.

I have the Services tab in my Linux version.

Comment 5

[Grimwald]

Services tab remains on Windows 7 x64 after removing the social entries mentioned in the OP. Really can't see why these services have to be 'hard coded'

Comment 6

[WildcatRay]

(In reply to Grimwald from comment #5)
> Services tab remains on Windows 7 x64 after removing the social entries
> mentioned in the OP. Really can't see why these services have to be 'hard
> coded'

I guess it is possible that hard coding may be the first step, but as the project progresses, it may evolve in a different direction. fb may just be the path chosen to cut the teeth on the overall project.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

One alternative would be to not show providers that weren't "activated". That maps better to some user expectations, but hinders the "discoverability" aspect of having them in the add-ons manager.

Just allowing "removal" (similar to how default search engines allow "removals" that actually map to hiding) is another option that may be a better tradeoff.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: addons manager will only show "installed" providers

This patch relies on a provider manifest being in user-level prefs to signal that it has been installed.  manifests in default prefs will not appear as part of the system until they are otherwise activated.  This patch is ready for review, but I'm not completely convinced about hiding builtin providers yet, will think about it more over the night.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 729937 [details] [diff] [review]
addons manager will only show "installed" providers

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

TBH I'm not that keen on this patch, particularly forcing an installed provider to have a "modified" preference.  I think it is worthwhile looking for a different approach to hiding such providers (and FWIW, I agree with the general intent of the bug - users shouldn't be forced to have an un-removable "Facebook" service in their face, as they aren't likely to understand it is completely benign until activated)

::: toolkit/components/social/SocialService.jsm
@@ +129,5 @@
>  
>  function migrateSettings() {
>    try {
>      // we don't care what the value is, if it is set, we've already migrated
>      Services.prefs.getCharPref("social.activeProviders");

trailing whitespace

Comment 11

[Shane Caraveo (:mixedpuppy)]

Attachment: addons manager will only show "installed" providers

this version stores installed state in ActiveProviders.

Comment 12

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=2c5641c6d83e

Comment 13

[Shane Caraveo (:mixedpuppy)]

this should go into fx21

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #13)
> this should go into fx21

this should go into *fx22*

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 731585 [details] [diff] [review]
addons manager will only show "installed" providers

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

I don't understand why we now have the concept of "active and enabled" vs "active and disabled" - can you elaborate on why this is necessary and why a provider simply being in ActiveProviders isn't enough to indicate if the provider is "installed" or not?

::: toolkit/components/social/SocialService.jsm
@@ +88,5 @@
>    }
>  };
>  
> +/* presence in social.activeProviders means the provider is installed.  The
> + * value is wether the provider is enabled.  When a provider is initially

whether

Comment 16

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #15)
> Comment on attachment 731585 [details] [diff] [review]
> addons manager will only show "installed" providers
> 
> Review of attachment 731585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't understand why we now have the concept of "active and enabled" vs
> "active and disabled" - can you elaborate on why this is necessary and why a
> provider simply being in ActiveProviders isn't enough to indicate if the
> provider is "installed" or not?

We have to track both installed and enabled state since you can have installed providers that are not enabled.  This patch uses activation as installation and separately tracks the enabled state.

Comment 17

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 731585 [details] [diff] [review]
addons manager will only show "installed" providers

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

This still seems more complex than is necessary.  IIUC:

* We already have activeProviders, which tells you which of the builtin manifests are "installed".
* You now have a requirement that providers which are installed can be marked as "disabled by the user".

The changes seem quite a bit more complex than I would expect to support this new requirement - eg, I don't understand how this new requirement implies a need to change now manifests are enumerated, why removeProvider marks a provider as disabled, etc.

Wouldn't a simpler approach be to have a "smart set", much like activeProviders, but called 'disabledProviders'?  Then a provider is still installed if |ActiveProviders.has(origin)|, installed and enabled if |ActiveProviders.has(origin) and !DisabledProviders.has(origin)| and create an invariant that if |DisabledProviders.has(origin)| then |ActiveProviders.has(origin)| must return true.  (storing 'disabled' providers seems better than storing 'enabled' providers - it avoids a need to migrate and in the common case will remain empty)

::: toolkit/components/social/SocialService.jsm
@@ +148,4 @@
>    try {
>      // we don't care what the value is, if it is set, we've already migrated
>      Services.prefs.getCharPref("social.activeProviders");
>      return;

doesn't this mean that if the migration from social.active -> social.activeProviders has happened, the current providers will not be marked as enabled?

@@ +243,3 @@
>        addon.pendingOperations -= AddonManager.PENDING_ENABLE;
>        AddonManagerPrivate.callAddonListeners("onEnabled", addon);
> +      if (onDone) {

there seems a risk here that onDone will be called before addProvider does the provider-added notification.  Wouldn't it be better to pass your own callback to addProvider, which in turn calls onDone?

@@ +287,5 @@
>        addon.pendingOperations |= AddonManager.PENDING_DISABLE;
>      }
>      provider.enabled = false;
>  
> +    ActiveProviders.disable(provider.origin);

this is in a method called 'removeProvider', but you are calling ActiveProviders.disable.  This doesn't make sense to me - if the provider is being removed, why must it be flagged as disabled?

Comment 18

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #17)
> Comment on attachment 731585 [details] [diff] [review]
> addons manager will only show "installed" providers
> 
> Review of attachment 731585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This still seems more complex than is necessary.  IIUC:

TBH, the original patch I suggested is the best approach, it stayed with the manifest (at user level) signifying install and activeProviders signifying enabled state.  Each iteration is making this patch larger and more complex with no added benefit.

> * We already have activeProviders, which tells you which of the builtin
> manifests are "installed".

That's not correct.  Without this patch, the existence of the manifest pref is installed state and activeProviders tells you which of the installed providers are enabled.

This patch changes to having existence in activeProviders to mean installed, and the value stored there is the enabled state.  Migration is not necessary with this change because earlier use conflated the two (ie. it wasn't possible to be installed+disabled).

> * You now have a requirement that providers which are installed can be
> marked as "disabled by the user".
> 
> The changes seem quite a bit more complex than I would expect to support
> this new requirement - eg, I don't understand how this new requirement
> implies a need to change now manifests are enumerated, why removeProvider
> marks a provider as disabled, etc.

The enumeration is more complex because pretty much everywhere we use SocialServiceInternal.manifests we're expecting *installed* providers.  Now some of those (builtin) manifests may not be "installed".  We now need to be able to iterate all manifests or installed manifests.

> Wouldn't a simpler approach be to have a "smart set", much like
> activeProviders, but called 'disabledProviders'?  Then a provider is still
> installed if |ActiveProviders.has(origin)|

That is the case with this patch.

> installed and enabled if
> |ActiveProviders.has(origin) and !DisabledProviders.has(origin)| and create
> an invariant that if |DisabledProviders.has(origin)| then
> |ActiveProviders.has(origin)| must return true.  

It is simpler to use |ActiveProviders.enabled(origin)|

> (storing 'disabled'
> providers seems better than storing 'enabled' providers - it avoids a need
> to migrate and in the common case will remain empty)

This would make this more complex, not less.  There is no need for additional migration with the approach I took, this would require additional migration, ensureing we keep to separate prefs properly in sync, and it optimizes towards the issue around builtin providers, which in the mid to long term will be the minority if not entirely go away.

> ::: toolkit/components/social/SocialService.jsm
> @@ +148,4 @@
> >    try {
> >      // we don't care what the value is, if it is set, we've already migrated
> >      Services.prefs.getCharPref("social.activeProviders");
> >      return;
> 
> doesn't this mean that if the migration from social.active ->
> social.activeProviders has happened, the current providers will not be
> marked as enabled?

No, it means the previous migration set 1 into the value for that provider, which in in this patch still means enabled.  Migration from 21 to this is not necessary.


> @@ +243,3 @@
> >        addon.pendingOperations -= AddonManager.PENDING_ENABLE;
> >        AddonManagerPrivate.callAddonListeners("onEnabled", addon);
> > +      if (onDone) {
> 
> there seems a risk here that onDone will be called before addProvider does
> the provider-added notification.  Wouldn't it be better to pass your own
> callback to addProvider, which in turn calls onDone?

possibly, I'll have to look at it closer.  there was also an issue with ondone being called prior to the onEnabled call, thus the move.

> @@ +287,5 @@
> >        addon.pendingOperations |= AddonManager.PENDING_DISABLE;
> >      }
> >      provider.enabled = false;
> >  
> > +    ActiveProviders.disable(provider.origin);
> 
> this is in a method called 'removeProvider', but you are calling
> ActiveProviders.disable.  This doesn't make sense to me - if the provider is
> being removed, why must it be flagged as disabled?

add/removeProvider calls are actually enable/disable and need to be renamed, regardless of approach.  I choose not to do that in this patch.

Comment 19

[Mark Hammond [:markh] [:mhammond]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #18)
> (In reply to Mark Hammond (:markh) from comment #17)
> > Comment on attachment 731585 [details] [diff] [review]
> > addons manager will only show "installed" providers
> > 
> > Review of attachment 731585 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This still seems more complex than is necessary.  IIUC:
> 
> TBH, the original patch I suggested is the best approach, it stayed with the
> manifest (at user level) signifying install and activeProviders signifying
> enabled state.  Each iteration is making this patch larger and more complex
> with no added benefit.

I hadn't been keeping up with some of the addons changes.  On reviewing the existing code in more detail, I agree with this (sorry 'bout that).  I'll update the patch flags accordingly.

Comment 20

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 729937 [details] [diff] [review]
addons manager will only show "installed" providers

On reflection, I agree this is a better approach.  It's a bit of a shame to add the ".installed" flag to "external" manfiests as it will make updating/comparison later a but tricky, but that's not a show stopper.  It also seems we could use the new installed flag instead of an explicit check for a "user pref" - but that's neither here nor there.

The lack of tests is a little troubling though, so if we can do something sane for testing it would be awesome - particularly the various migration branches.

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: addons manager will only show "installed" providers

https://tbpl.mozilla.org/?tree=Try&rev=59ed721118be

extended tests to handle userpref as installed flag
added builtin install test
locations where we set/get of prefs now handle unicode

Comment 22

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 733085 [details] [diff] [review]
addons manager will only show "installed" providers

I realized the migration code is wrong, will fix and add test before review.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Attachment: uninstall patch

Beyond the previous changes, this fixes migration and adds xpcshell tests for migration.

Comment 24

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=6a3cd3b1cc20

Comment 25

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 733126 [details] [diff] [review]
uninstall patch

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

LGTM with the minor nits fixed.

::: browser/base/content/test/social/browser_addons.js
@@ +36,5 @@
> +  setBuiltinManifestPref(prefname, manifest);
> +  // ensure that manifest2 is NOT showing as builtin
> +  is(SocialService.getOriginActivationType(manifest.origin), "builtin", "manifest is builtin");
> +  is(SocialService.getOriginActivationType(manifest2.origin), "foreign", "manifest2 is not builtin");
> +  

whitespace

@@ +87,3 @@
>      let expectEvent;
> +    let prefname = getManifestPrefname(manifest);
> +    info("using manifest prefname "+prefname);

whitespace around operators.

::: browser/base/content/test/social/head.js
@@ +238,5 @@
>    string.data = JSON.stringify(manifest);
>    Services.prefs.setComplexValue(name, Ci.nsISupportsString, string);
>  }
>  
> +function setBuiltinManifestPref(name, manifest) {

this appears to be used in exactly 1 test - it should probably go in that test until we find something else needs it.  It's also probably worth a comment here noting the subtle requirement that the pref must not be seen as "user modified"

::: toolkit/components/social/SocialService.jsm
@@ +68,5 @@
>      let MANIFEST_PREFS = Services.prefs.getBranch("social.manifest.");
>      let prefs = MANIFEST_PREFS.getChildList("", []);
>      for (let pref of prefs) {
>        try {
> +        var manifest = JSON.parse(MANIFEST_PREFS.getComplexValue(pref, Ci.nsISupportsString).data);

even though it was already 'var', use 'let'?

@@ +130,5 @@
>  function migrateSettings() {
>    try {
>      // we don't care what the value is, if it is set, we've already migrated
>      Services.prefs.getCharPref("social.activeProviders");
> +    

whitespace

@@ +153,2 @@
>      return;
>    } catch(e) {

this exception handler seems too broad.  It would be better if we just wrap the exception handler about the getCharPref() call (which is what we are expecting to fail), and have the rest of the code outside (so some unexpected failure in the rest of the logic doesn't cause us to make poor assumptions.

@@ +157,5 @@
>        let active = Services.prefs.getBoolPref("social.active");
>        if (active) {
> +        // primary difference from SocialServiceInternal.manifests is that we
> +        // only read the default branch here.
> +        let MANIFEST_PREFS = Services.prefs.getDefaultBranch("social.manifest.");

unusual all-caps for a local variable name

@@ +166,5 @@
> +            if (manifest && typeof(manifest) == "object" && manifest.origin) {
> +              // ensure we override a builtin manifest by having a different value in it
> +              if (manifest.builtin)
> +                delete manifest.builtin;
> +              let string = Cc["@mozilla.org/supports-string;1"].

can probably avoid wrapping this line without it exceeding the length of other lines around it.

@@ +179,5 @@
> +            }
> +          } catch (err) {
> +            Cu.reportError("SocialService: failed to load manifest: " + pref +
> +                           ", exception: " + err);
> +          }

ditto for this block - there's now alot more code in the exception handler that we really don't expect to fail.  (eg, in this case, just declare 'active' outside the exception handler and have just the getBoolPref inside it, then the entire 'if (active)' block can move outside.
(Yes, the existing code was guilty of this too, but as the code grows, so does the risk)

@@ +339,5 @@
>      });
>    },
>  
>    getOriginActivationType: function(origin) {
> +    try {

if (Services.prefs.getDefaultBranch("social.manifest.").getPrefType(prefname) == Services.prefs.PREF_STRING)
  return 'builtin';

?  A little clearer and avoids the try/catch block

::: toolkit/components/social/test/xpcshell/test_SocialServiceMigration21.js
@@ +6,5 @@
> +
> +const DEFAULT_PREFS = Services.prefs.getDefaultBranch("social.manifest.");
> +
> +function run_test() {
> +  // Test must run as startup for migration to occure, so we can only test

s/as/at/ and /occure/occur/

@@ +47,5 @@
> +    do_check_true(activeProviders.has(manifest.origin));
> +    do_check_true(MANIFEST_PREFS.prefHasUserValue(manifest.origin));
> +  });
> +  yield true;
> +}

would it be feasable to add another test here (or somewhere) that checks *all* builtin provider prefs have the 'builtin' attribute?  If someone adds one later but neglects to add this, I think some of our logic is likely to fail...

::: toolkit/components/social/test/xpcshell/test_SocialServiceMigration22.js
@@ +6,5 @@
> +
> +const DEFAULT_PREFS = Services.prefs.getDefaultBranch("social.manifest.");
> +
> +function run_test() {
> +  // Test must run as startup for migration to occure, so we can only test

same typos as previous test

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: uninstall patch

feedback implemented, only re-requesting review for new test file.

Comment 27

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 733162 [details] [diff] [review]
uninstall patch

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

LGTM

::: browser/base/content/test/social/browser_defaults.js
@@ +8,5 @@
> +  let prefs = manifestPrefs.getChildList("", []);
> +  ok(prefs.length > 0, "we have builtin providers");
> +  for (let pref of prefs) {
> +    let manifest = JSON.parse(manifestPrefs.getComplexValue(pref, Ci.nsISupportsString).data);
> +    ok(manifest.builtin, "manifest is builtin "+manifest.origin);

spaces between operators

Comment 28

[Shane Caraveo (:mixedpuppy)]

Attachment: uninstall patch

feedback and fixed test error from previous try. carrying forward r+

https://tbpl.mozilla.org/?tree=Try&rev=70575317098c

Comment 29

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c13f65b59301

Comment 30

[Shane Caraveo (:mixedpuppy)]

Attachment: uninstall patch-aurora

[Approval Request Comment]
User impact if declined: unable to uninstall builtin providers, they appear installed by default
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): any builtin provider will appear as installed to the user when the user has taken no action to install
String or IDL/UUID changes made by this patch: none

The patch looks large, it is largely tests.  original patch was applied on top of a patch that will not be added to aurora, this is just to make applying the patch easier.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c13f65b59301

Comment 32

[u279076]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/c13f65b59301

I believe this has caused regression bug 858663.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Attachment: uninstall patch-aurora

[Approval Request Comment]
User impact if declined: unable to uninstall builtin providers, they appear installed by default
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): any builtin provider will appear as installed to the user when the user has taken no action to install
String or IDL/UUID changes made by this patch: none

The patch looks large, it is largely tests.  original patch was applied on top of a patch that will not be added to aurora, this is just to make applying the patch easier.

This patch now also incorporates the patch from bug 858663.  The origin patch here caused 858663.  It is fixed with additional test coverage for that breakage.

Comment 34

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 734122 [details] [diff] [review]
uninstall patch-aurora

(no need for re-review since this is just an uplift combination of two reviewed patches)

Comment 35

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 734122 [details] [diff] [review]
uninstall patch-aurora

bug 858868 has to be rolled into this as well :(

Comment 36

[Shane Caraveo (:mixedpuppy)]

Attachment: uninstall patch-aurora

[Approval Request Comment]
User impact if declined: unable to uninstall builtin providers, they appear installed by default
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): any builtin provider will appear as installed to the user when the user has taken no action to install
String or IDL/UUID changes made by this patch: none

The patch looks large, it is largely tests.  original patch was applied on top of a patch that will not be added to aurora, this is just to make applying the patch easier.

This patch now also incorporates the patch from bug 858663 and bug 858868.  The origin patch here caused bug 858663 and bug 858868.  It is fixed with additional test coverage for that breakage.

Comment 37

[Shane Caraveo (:mixedpuppy)]

pushed attachment #734182 [details] for aurora to try:

https://tbpl.mozilla.org/?tree=Try&rev=5ead6bfad3e9

Comment 38

[u279076]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #31)
> https://hg.mozilla.org/mozilla-central/rev/c13f65b59301

Verified fixed in Firefox Nightly 23.0a1 2013-04-08.

Comment 39

[u279076]

(In reply to Shane Caraveo (:mixedpuppy) from comment #37)
> pushed attachment #734182 [details] for aurora to try:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=5ead6bfad3e9

Verified fixed for Aurora with this Try build.

Comment 40

[Shane Caraveo (:mixedpuppy)]

fixed test, pushed another try
https://tbpl.mozilla.org/?tree=Try&rev=c2cf54c654e6

Comment 41

[Alex Keybl [:akeybl]]

Adding verifyme for QA to take a look once this lands on FF22.

Comment 42

[Alex Keybl [:akeybl]]

Comment on attachment 734182 [details]
uninstall patch-aurora

Very manageable risk, especially given where we are in the cycle and the fact that there can be targeted QA testing.

Comment 43

[u279076]

(In reply to Alex Keybl [:akeybl] from comment #42)
> Comment on attachment 734182 [details]
> uninstall patch-aurora
> 
> Very manageable risk, especially given where we are in the cycle and the
> fact that there can be targeted QA testing.

Thanks Alex, it's on my radar.

Comment 44

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0789c9643fae

Comment 45

[u279076]

(In reply to Shane Caraveo (:mixedpuppy) from comment #44)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/0789c9643fae

Verified fixed in Firefox Aurora 22.0a2 2013-04-09.