862314 - possible to install a provider twice

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Florian Quèze [:florian]]

Steps to reproduce:
1. Open the add-on manager.
2. Select the "Services" section.
3. Open http://mixedpuppy.github.io/socialapi-demo/ in another tab.
4. Click the "Activate The Demo Provider" button.
5. (If the demo provider isn't installed yet) Click "Enable Services" on the door hanger notification.
6. Look in the add-on manager: "Demo Social Service 1.0" appeared.
7. Go back to the http://mixedpuppy.github.io/socialapi-demo/ tab and click the "Activate The Demo Provider" button again.
8. Go back to the add-on manager.

Expected result: one "Demo Social Service 1.0" item in the list.

Actual result: a "Demo Social Service 1.0" item per click on the Activate button.


Note: In the add-on manager, selecting the "Extensions" section and selecting "Services" again makes the duplicates disappear.

Tested with: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20130415 Firefox/23.0

Comment 1

[Shane Caraveo (:mixedpuppy)]

We need to check that the provider is actually uninstalled before allowing it to be installed again.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent double install.patch

This is really an edge case bug, it is unlikely that a typical user will open about:addons, remove a provider, open a new tab (without closing about:addons) and attempt to reactivate the same provider.  So while this also affects Fx22, I think it is fine to just get the fix into Fx23.

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

Comment 4

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 744216 [details] [diff] [review]
prevent double install.patch

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

how hard is it to add tests for this case?

::: browser/base/content/test/social/browser_social_activation.js
@@ +41,5 @@
>    }
>  }
>  
>  function addBuiltinManifest(manifest) {
> +  setBuiltinManifestPref("social.manifest."+manifest.origin, manifest);

might as well add a space around the '+' operator while we are here.

::: toolkit/components/social/SocialService.jsm
@@ +489,5 @@
>    installProvider: function(aDOMDocument, data, installCallback) {
>      let sourceURI = aDOMDocument.location.href;
>      let installOrigin = aDOMDocument.nodePrincipal.origin;
>  
> +    // bug 862314 don't allow double installation.

The comment probably doesn't need to refer to the bug as it is somewhat self-evident that installing an already installed provider is somewhat bad.  (I tend to think bug numbers in comments should be for non-obvious cases where looking up the bug will add enough context to make things clear)

what is the impact of throwing this error?  ie, will it appear in the console?  Should we just return without throwing (possibly with a "message" logged rather than an error report)?

Also, is it possible that in the future it *will* be necessary to re-activate a provider to get an updated manifest?  I guess this isn't a bug deal at the moment though...

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 744216 [details] [diff] [review]
prevent double install.patch

Does this patch address the case in bug 862334? If a provider has been removed, but is still displayed as "... has been removed" with a 'Cancel' link in the add-on manager, I would expect that installing the provider again would work, not throw.

Comment 6

[Shane Caraveo (:mixedpuppy)]

(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Comment on attachment 744216 [details] [diff] [review]
> prevent double install.patch
> 
> Does this patch address the case in bug 862334? If a provider has been
> removed, but is still displayed as "... has been removed" with a 'Cancel'
> link in the add-on manager, I would expect that installing the provider
> again would work, not throw.

Why would you expect that?  Installing the provider again results in two installs of the provider.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #4)
> Comment on attachment 744216 [details] [diff] [review]
> prevent double install.patch
> 
> Review of attachment 744216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> how hard is it to add tests for this case?

pretty difficult without more investigation.  The issue has to do with some internal state in the addon manager itself, where the user has "removed" the provider, but the addon manager has not yet uninstalled the provider.

> ::: browser/base/content/test/social/browser_social_activation.js
> @@ +41,5 @@
> >    }
> >  }
> >  
> >  function addBuiltinManifest(manifest) {
> > +  setBuiltinManifestPref("social.manifest."+manifest.origin, manifest);
> 
> might as well add a space around the '+' operator while we are here.
> 
> ::: toolkit/components/social/SocialService.jsm
> @@ +489,5 @@
> >    installProvider: function(aDOMDocument, data, installCallback) {
> >      let sourceURI = aDOMDocument.location.href;
> >      let installOrigin = aDOMDocument.nodePrincipal.origin;
> >  
> > +    // bug 862314 don't allow double installation.
> 
> The comment probably doesn't need to refer to the bug as it is somewhat
> self-evident that installing an already installed provider is somewhat bad. 
> (I tend to think bug numbers in comments should be for non-obvious cases
> where looking up the bug will add enough context to make things clear)
> 
> what is the impact of throwing this error?  ie, will it appear in the
> console?  Should we just return without throwing (possibly with a "message"
> logged rather than an error report)?

Sure we could just return rather than throw an error.

> Also, is it possible that in the future it *will* be necessary to
> re-activate a provider to get an updated manifest?  I guess this isn't a bug
> deal at the moment though...

There are separate update mechanisms.  I'm not sure they are supported right now.

Comment 8

[Florian Quèze [:florian]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > Comment on attachment 744216 [details] [diff] [review]
> > prevent double install.patch
> > 
> > Does this patch address the case in bug 862334? If a provider has been
> > removed, but is still displayed as "... has been removed" with a 'Cancel'
> > link in the add-on manager, I would expect that installing the provider
> > again would work, not throw.
> 
> Why would you expect that?

It's not uncommon for users confused about the state of something to uninstall it and try installing it again in the hope it will fix things. (At least I saw this advice often being given by/to Windows users.)
Currently if a user trying this doesn't close the add-on manager tab before attempting to install the social provider again, we get into bug 862334.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent double install.patch

slightly different patch that fixes the remove/undo/install states in the addon manager.

https://tbpl.mozilla.org/?tree=Try&rev=17d8cb5b1e8f

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> Created attachment 746103 [details] [diff] [review]
> prevent double install.patch
> 
> slightly different patch that fixes the remove/undo/install states in the
> addon manager.

I'm really having a problem getting the right try builds today :(

https://tbpl.mozilla.org/?tree=Try&rev=57296bbae1a3

Comment 11

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 746103 [details] [diff] [review]
prevent double install.patch

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

The changes look OK, but I don't understand the tests - it looks like you are changing the tests in ways that shouldn't be affected by this patch, but haven't added new tests to exercise the new code?

::: browser/base/content/test/social/browser_social_activation.js
@@ +41,5 @@
>    }
>  }
>  
>  function addBuiltinManifest(manifest) {
> +  setBuiltinManifestPref("social.manifest."+manifest.origin, manifest);

I think I mentioned this space around operators in the previous review?

@@ +152,5 @@
> +          checkSocialUI();
> +          Services.prefs.clearUserPref("social.whitelist");
> +          next();
> +        })
> +      }, "activation panel should be showing");

I don't understand how this change related to the bug.  It almost looks like this is fixing an unrelated orange?

Ditto for the similar changes below - they all seems to be doing the same basic thing.  Can we refactor this into a helper function?

::: toolkit/components/social/SocialService.jsm
@@ +846,5 @@
>  function AddonInstaller(sourceURI, aManifest, installCallback) {
>    aManifest.updateDate = Date.now();
>    // get the existing manifest for installDate
>    let manifest = SocialServiceInternal.getManifestByOrigin(aManifest.origin);
> +  let isUpgrade = !!manifest;

Use isNewInstall = !manifest; - then the other places that use it read better (ie, they lose the negation)

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #11)
> Comment on attachment 746103 [details] [diff] [review]
> prevent double install.patch
> 
> Review of attachment 746103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The changes look OK, but I don't understand the tests - it looks like you
> are changing the tests in ways that shouldn't be affected by this patch, but
> haven't added new tests to exercise the new code?

> @@ +152,5 @@
> > +          checkSocialUI();
> > +          Services.prefs.clearUserPref("social.whitelist");
> > +          next();
> > +        })
> > +      }, "activation panel should be showing");
> 
> I don't understand how this change related to the bug.  It almost looks like
> this is fixing an unrelated orange?

The install function is now async since I'm asking the addonmanager for the related addon prior to installing a new one.

I'll fixup for the other comments.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent double install.patch

feedback added, tests added, lots of async test fixing going on

new try

https://tbpl.mozilla.org/?tree=Try&rev=9a155dfade0b

Comment 14

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

Comment on attachment 747187 [details] [diff] [review]
prevent double install.patch

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

LGTM

::: browser/base/content/test/social/browser_social_activation.js
@@ +85,5 @@
> +function waitForProviderLoad(cb) {
> +  Services.obs.addObserver(function providerSet(subject, topic, data) {
> +    Services.obs.removeObserver(providerSet, "social:provider-set");
> +    info("social:provider-set observer was notified");
> +    // executeSoon to let the browser UI observers run first

this comment should probably go next to the executeSoon

@@ +111,5 @@
> +  }
> +  return null;
> +}
> +
> +function click_addon_remove_button(tab, aCallback) {

these 3 helper function should be named as camelCase for consistency?

::: browser/base/content/test/social/social_activate.html
@@ +11,5 @@
>    "icon32URL": "chrome://branding/content/favicon32.png",
>    "icon64URL": "chrome://branding/content/icon64.png",
>  
>    // at least one of these must be defined
> +  "sidebarURL": "/browser/browser/base/content/test/social/social_sidebar.html",

it looks like you've just changed the order here.  If the order is relevant, then I'd expect we have underlying larger problems? :)  Might as well revert this path of the patch...

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent double install.patch

feedback implemented, carry forward r+

Comment 16

[Shane Caraveo (:mixedpuppy)]

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

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 747218 [details] [diff] [review]
prevent double install.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 755126
User impact if declined: possible to install same provider twice with addon manager open
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Apparently this patch works so well that a provider sometimes doesn't install at all! Backed out for intermittent mochitest b-c failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/46a74e87779c

https://tbpl.mozilla.org/php/getParsedLog.php?id=22758645&tree=Mozilla-Inbound

19:32:31  WARNING -  TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_activation.js | uncaught exception - TypeError: Social.provider is null at chrome://mochitests/content/browser/browser/base/content/test/social/browser_social_activation.js:91

Comment 19

[Shane Caraveo (:mixedpuppy)]

Try with test fix

https://tbpl.mozilla.org/?tree=Try&rev=1e180f9dcf82

Comment 20

[Shane Caraveo (:mixedpuppy)]

looks like bc is fixed, but running a full try

https://tbpl.mozilla.org/?tree=Try&rev=8766806d2eaa

Comment 21

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6b5a9da506

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/7a6b5a9da506

Comment 23

[Ryan VanderMeulen [:RyanVM]]

This needs some love for the uplift.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent double install on beta.patch

beta version of patch with try push at:

https://tbpl.mozilla.org/?tree=Try&rev=401268e288d6

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent double install on beta.patch

fixed bc tests for beta

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/242121889ccd

Comment 27

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101 Firefox/22.0

In 22 beta 2 I can`t install Demo Social Service. After hitting Activate The Demo Provider nothing happens. In latest nightly and aurora I am able to install this and the issue is fixed.

Comment 28

[Shane Caraveo (:mixedpuppy)]

(In reply to Bogdan Maris [QA] [:bogdan_maris] from comment #27)
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:22.0) Gecko/20100101
> Firefox/22.0
> 
> In 22 beta 2 I can`t install Demo Social Service. After hitting Activate The
> Demo Provider nothing happens. In latest nightly and aurora I am able to
> install this and the issue is fixed.

You can validate the fix on 22 by using one of the builtin providers, such as cliqz, instead of the demo provider.  Visit mozsocial.cliqz.com to activate.

Comment 29

[Bogdan Maris, Desktop Test Engineering]

(In reply to Shane Caraveo (:mixedpuppy) from comment #28)
> You can validate the fix on 22 by using one of the builtin providers, such
> as cliqz, instead of the demo provider.  Visit mozsocial.cliqz.com to
> activate.
Thanks Shane.
Verified as fixed in Firefox 22 beta 2 (buildID: 20130521223249).

Comment 30

[u279076]

Samvedana, can you please verify this is fixed in Firefox 23?

Comment 31

[Samvedana (:Samvedana)]

Attachment: Demo social service with problem

User Agent : Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20130430 Firefox/23.0
Build ID : 20130430030941

Tested on nightly and I am able to reproduce this issue on this build.

User Agent : Mozilla/5.0 (Windows NT 6.2; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0
Build ID : 20130708202947

Tested on Firefox 23-B4, I can see desired result. Please see the attached file.

Comment 32

[Samvedana (:Samvedana)]

Attachment: Demo social service on FX 23 B4