possible to install a provider twice

VERIFIED FIXED in Firefox 22

Status

defect
VERIFIED FIXED
6 years ago
4 months ago

People

(Reporter: florian, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 23
x86
macOS
Dependency tree / graph

Firefox Tracking Flags

(firefox22 verified, firefox23 verified)

Details

Attachments

(4 attachments, 4 obsolete attachments)

Reporter

Description

6 years ago
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
Assignee

Comment 1

6 years ago
We need to check that the provider is actually uninstalled before allowing it to be installed again.
Summary: Duplicated 'Services' items in the add-on manager after enabling the same social provider multiple times → possible to install a provider twice
Assignee

Updated

6 years ago
Duplicate of this bug: 862334
Assignee

Comment 3

6 years ago
Posted patch prevent double install.patch (obsolete) — Splinter Review
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
Assignee: nobody → mixedpuppy
Attachment #744216 - Flags: review?(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...
Reporter

Comment 5

6 years ago
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.
Assignee

Comment 6

6 years ago
(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.
Assignee

Comment 7

6 years ago
(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.
Reporter

Comment 8

6 years ago
(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.
Assignee

Comment 9

6 years ago
Posted patch prevent double install.patch (obsolete) — Splinter Review
slightly different patch that fixes the remove/undo/install states in the addon manager.

https://tbpl.mozilla.org/?tree=Try&rev=17d8cb5b1e8f
Attachment #744216 - Attachment is obsolete: true
Attachment #744216 - Flags: review?(mhammond)
Attachment #746103 - Flags: review?(mhammond)
Assignee

Comment 10

6 years ago
(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 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)
Assignee

Comment 12

6 years ago
(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.
Assignee

Comment 13

6 years ago
Posted patch prevent double install.patch (obsolete) — Splinter Review
feedback added, tests added, lots of async test fixing going on

new try

https://tbpl.mozilla.org/?tree=Try&rev=9a155dfade0b
Attachment #746103 - Attachment is obsolete: true
Attachment #746103 - Flags: review?(mhammond)
Attachment #747187 - Flags: review?(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...
Attachment #747187 - Flags: review?(mhammond) → review+
Assignee

Comment 15

6 years ago
feedback implemented, carry forward r+
Attachment #747187 - Attachment is obsolete: true
Attachment #747218 - Flags: review+
Assignee

Comment 17

6 years ago
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
Attachment #747218 - Flags: approval-mozilla-aurora?
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
Assignee

Comment 20

6 years ago
looks like bc is fixed, but running a full try

https://tbpl.mozilla.org/?tree=Try&rev=8766806d2eaa
https://hg.mozilla.org/mozilla-central/rev/7a6b5a9da506
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Attachment #747218 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #747218 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
This needs some love for the uplift.
Assignee

Comment 24

6 years ago
beta version of patch with try push at:

https://tbpl.mozilla.org/?tree=Try&rev=401268e288d6
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.
Assignee

Comment 28

6 years ago
(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.
(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).
Samvedana, can you please verify this is fixed in Firefox 23?
Keywords: verifyme
QA Contact: samvedana.gohil
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.
Status: RESOLVED → VERIFIED

Updated

4 months ago
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.