872605 - re-activation to update provider manifest needs ux

Status: VERIFIED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Florian Quèze [:florian]]

Steps to reproduce:
1. Have a social provider that has an "Activate" button that installs a provider without workerURL.
2. Update the code of the activate button, to include a workerURL pointing on a file that doesn't exist on the server.
3. Click the Activate button again.

Expected result:
- Some notification that the social provider has been updated.
- The social provider is reloaded (and shown as broken).

Actual result:
- The preference is updated, but there's no visible feedback.
- After a restart, the social provider is broken.

Comment 1

[edA-qa mort-ora-y]

Here's a reproduction that doesn't involve an error state and will be encountered by a lot of people. (This reproduction can be done whe the https://mozsocial.cliqz.com/ provider)
1. Using Firefox 21, install a current provider, which won't have a shareURL
2. Upgrade to Firefox 23 (Aurora)
3. Reactivate the provider, which now has a shareURL
4. Zero feedback to user (Error)
5. New shareURL not used (Error)
6. Worker not reloaded (Error)

Note that even if you turn on/off the provider nothing is reloaded. You need to reload the browser before seeing the changes.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Attachment: upgrade manifest from reactivation

This allows a provider to update the manifest and have it take affect without restarting firefox.  If the provider is the current provider, it is completely reloaded.  The "undo" panel is shown again, which isn't quite appropriate, but we wont be able to change strings (ie. we should uplift this to fx23)

Comment 3

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

Having "addBuiltinProvider" do this auto-magically doesn't feel right - seems like there should be a more explicit API for this. But I admit losing track of how this code all works - maybe addBuiltinProvider is the right place and it just needs a better name.

Comment 4

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

markh is probably a better reviewer, since he seems closer to this stuff?

Comment 5

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #3)
> Having "addBuiltinProvider" do this auto-magically doesn't feel right -
> seems like there should be a more explicit API for this. But I admit losing
> track of how this code all works - maybe addBuiltinProvider is the right
> place and it just needs a better name.

Right now this is the most direct route for refreshing the active providers, and since we really should have a way for providers to update in the short term (fx23) I'd like to go this way.  I do think we need some refactoring in SocialService.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 763720 [details] [diff] [review]
upgrade manifest from reactivation

not quite working yet, will have to examine this more.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: upgrade manifest from reactivation

The previous patch didn't fix browser_social_multiprovider.js.  That test was using activateFromOrigin which expects the pref to exist now (and a comment within activateFromOrigin alludes to that), however, the setup for that test uses runSocialTestWithProvider which does not add the prefs.  The primary change in addBuiltinProvider just makes that a hard requirement in this situation.

I also added a fix to activateFromOrigin, but can remove it if it is a big deal.

Comment 8

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

(In reply to edA-qa mort-ora-y from comment #1)
> Here's a reproduction that doesn't involve an error state and will be
> encountered by a lot of people. (This reproduction can be done whe the
> https://mozsocial.cliqz.com/ provider)
> 1. Using Firefox 21, install a current provider, which won't have a shareURL
> 2. Upgrade to Firefox 23 (Aurora)

At this point, what state is the provider in?  I'm guessing it is broken?

> 3. Reactivate the provider, which now has a shareURL

And this step requires the user to manually navigate to the provider's site, locate a page with the activation button, then click it, after which things work again?

If that's correct, it sounds like a bigger problem than what this patch is solving.  ie, I don't have any real objection to activation also doing an "upgrade", but it sounds like we actually want to avoid the browser/provider being broken after step 2 and avoid step 3 completely?

Comment 9

[edA-qa mort-ora-y]

After Step 2 our provider still works but it is lacking a share button. It is however not hard to imagine that some providers would completely break at this point.

I'd also prefer the user doesn't have to go back to your site to do an upgrade manually. If there a "reloadManifest" API of some kind I could use that -- since I'll be able to detect on my own when it's outdated.

Alternately, if this is to behave more like an AddOn, you may wish to have a standard way of detecting that it is outdated.  However, unlike an addon there is no way to keep running an outdated version since the pages/worker are always reloaded at start, thus getting the newer version but old manifest.

Comment 10

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #8)
> (In reply to edA-qa mort-ora-y from comment #1)
> > Here's a reproduction that doesn't involve an error state and will be
> > encountered by a lot of people. (This reproduction can be done whe the
> > https://mozsocial.cliqz.com/ provider)
> > 1. Using Firefox 21, install a current provider, which won't have a shareURL
> > 2. Upgrade to Firefox 23 (Aurora)
> 
> At this point, what state is the provider in?  I'm guessing it is broken?

no.  the only new feature in the manifest is the shareurl, which in a normal case would never be loaded until the user goes to the share button and chooses that providers.  We simply are not reloading the provider instances when an update occurs.  This patch is necessary regardless of how the manifest pref gets updated (ie. user clicking activate button, or new api for worker)

> > 3. Reactivate the provider, which now has a shareURL
> 
> but it sounds like we actually want to avoid the browser/provider
> being broken after step 2 and avoid step 3 completely?

They are not.

Comment 11

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

(In reply to Shane Caraveo (:mixedpuppy) from comment #10)
> > At this point, what state is the provider in?  I'm guessing it is broken?
> 
> no.  the only new feature in the manifest is the shareurl,

So the provider is broken WRT share functionality.  And the only way that can be fixed is for the user to navigate to the activation page.

That still sounds sub-optimal and "broken" to me.  Are there other possibilities worth discussing that don't force the user to navigate to a specific page to take action?

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: fix updating manifests, add worker api for self-update

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

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: fix updating manifests, add worker api for self-update

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

Comment 14

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

Comment on attachment 765178 [details] [diff] [review]
fix updating manifests, add worker api for self-update

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

Looking good, but there are a few tweaks necessary IMO.

::: browser/base/content/test/social/browser_addons.js
@@ +280,5 @@
> +    let activationURL = manifest2.origin + "/browser/browser/base/content/test/social/social_activate.html"
> +    addTab(activationURL, function(tab) {
> +      let doc = tab.linkedBrowser.contentDocument;
> +      let installFrom = doc.nodePrincipal.origin;
> +      Services.prefs.setCharPref("social.whitelist", installFrom);

this pref should be reset

::: browser/base/content/test/social/social_worker.js
@@ +151,5 @@
>        case "share-data-message":
>          if (testPort)
>            testPort.postMessage({topic:"got-share-data-message", result: event.data.result});
>          break;
> +      case "worker.update":

it looks like manifest.get doesn't have test coverage - maybe just call that and bump the version number by 1 - that would avoid duplicating the manifest here too.

::: browser/modules/Social.jsm
@@ +161,5 @@
>        // An engine change caused by adding/removing a provider should notify
>        if (topic == "provider-added" || topic == "provider-removed") {
>          this._updateProviderCache(data);
>          Services.obs.notifyObservers(null, "social:providers-changed", null);
> +      } else if (topic == "provider-update") {

I think we tend to prefer an early return here - ie, return in the first block and drop the "else"

@@ +165,5 @@
> +      } else if (topic == "provider-update") {
> +        // a provider has self-updated its manifest, we need to update our
> +        // cache and possibly reload if it was the current provider.
> +        let provider = data;
> +        let doReload = this.provider.origin == provider.origin;

I'm not sure this check is necessary - currently the provider *must* be current as that's the only way the call can be made in the first-place?  And later, if we change that, it might end up being the case that we need to reload the provider even if they aren't current.

A better check might just be "if (provider.enabled) {... toggle to false then back to true...)?

@@ +256,5 @@
>      // provider has already been activated - we still get called back with it.
>      SocialService.addBuiltinProvider(origin, function(provider) {
>        if (provider) {
>          // No need to activate again if we're already active
> +        if (provider != this.provider)

this looks wrong to me.  The old code would not callback if the provider was already current - and thus would not display the activation prompt.  But it *would* set the new provider current, which IIUC, was the motivation for this change.  This new code always makes the callback, so always displays the activation prompt (and that is the only thing the callback does)

So I don't think this change is necessary for this patch.  If it is, I don't think there is test coverage of the new behaviour (and for that matter, I guess there isn't complete test coverage of the old behaviour wrt the activation prompt either, or tests would fail).  Or I'm just wrong about something :)

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: fix updating manifests, add worker api for self-update

feedback added

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

Comment 16

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

Comment on attachment 765189 [details] [diff] [review]
fix updating manifests, add worker api for self-update

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

::: browser/modules/Social.jsm
@@ +172,5 @@
> +          this._updateProviderCache(providers);
> +          Services.obs.notifyObservers(null, "social:providers-changed", null);
> +          // if we need a reload, do it now
> +          if (provider.enabled) {
> +            this.enabled = false;

I'd really prefer toggling provider.enabled here rather than Social.enabled if possible

Comment 17

[Shane Caraveo (:mixedpuppy)]

(In reply to Mark Hammond (:markh) from comment #16)
> Comment on attachment 765189 [details] [diff] [review]
> fix updating manifests, add worker api for self-update
> 
> Review of attachment 765189 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/modules/Social.jsm
> @@ +172,5 @@
> > +          this._updateProviderCache(providers);
> > +          Services.obs.notifyObservers(null, "social:providers-changed", null);
> > +          // if we need a reload, do it now
> > +          if (provider.enabled) {
> > +            this.enabled = false;
> 
> I'd really prefer toggling provider.enabled here rather than Social.enabled
> if possible

That would only reload the worker, whereas we need to reload all the frames.  Considering an manifest update could change the sidebar url, we need to reload everything.  We do need to fix the reload situation so we don't have to toggle social.enabled.  I've added a followup bug 885200 to do something about reload.

Comment 18

[Shane Caraveo (:mixedpuppy)]

seems the last try hit leaks from intermittent failure in bug 858948, trying again to see if that cleared up.

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

Comment 19

[Shane Caraveo (:mixedpuppy)]

try three, see if the binding of the callbacks I added caused the issue.

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

Comment 20

[Shane Caraveo (:mixedpuppy)]

try four, remove another references that looks circular

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

Comment 21

[Shane Caraveo (:mixedpuppy)]

Attachment: fix updating manifests, add worker api for self-update

fixed leak by adding a getter for the manifest rather than keeping reference.  Only minor changes, carrying forward r+

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

Comment 22

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9661a68f0bff

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 766478 [details] [diff] [review]
fix updating manifests, add worker api for self-update

[Approval Request Comment]
Bug caused by (feature/regressing bug #): web install of manifests
User impact if declined: users wont have updated features supported by social providers (e.g. an existing provider adding support for share)
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 24

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a257161ff235 - your tests weren't particularly fond of it, both simple little mozSocial is undefineds like https://tbpl.mozilla.org/php/getParsedLog.php?id=24503203&tree=Mozilla-Inbound and more severe things like https://tbpl.mozilla.org/php/getParsedLog.php?id=24504005&tree=Mozilla-Inbound

Comment 25

[Shane Caraveo (:mixedpuppy)]

now that bug 860421 is fixed, try again:
https://tbpl.mozilla.org/?tree=Try&rev=b4f85f7f1394

Comment 26

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dd428369503

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8dd428369503

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 766478 [details] [diff] [review]
fix updating manifests, add worker api for self-update

[Approval Request Comment]
Bug caused by (feature/regressing bug #): web install of manifests
User impact if declined: users wont have updated features supported by social providers (e.g. an existing provider adding support for share)
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 29

[bhavana bajaj [:bajaj]]

Comment on attachment 766478 [details] [diff] [review]
fix updating manifests, add worker api for self-update

low risk patch giving users ability to make use of updated features which are now supported by social

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2b49f275dff

Comment 31

[u279076]

Florian and edA-qa, please check that this works as expected for you with tomorrow's Firefox Nightly and Aurora builds.

Comment 32

[edA-qa mort-ora-y]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #31)
> Florian and edA-qa, please check that this works as expected for you with
> tomorrow's Firefox Nightly and Aurora builds.

I'm not clear on exactly what was changed. Is there an API which triggers the reload, or is it automatic/manual, or what triggers it?

Comment 33

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

(In reply to edA-qa mort-ora-y from comment #32)
> I'm not clear on exactly what was changed. Is there an API which triggers
> the reload, or is it automatic/manual, or what triggers it?

Please see the attached patch, but the short version is that the worker can send 2 new messages - 'social.manifest-get' to fetch the current manifest (so the worker can see what version of the manifest is held) and 'social.manifest-set' (to update the manifest if the worker determines it is necessary).

Comment 34

[edA-qa mort-ora-y]

I've implemented what seems like a reasonable solution using this approach (and confirmed it works).
- on 'social.initialize' I post social.manifest-get
- on 'social.manifest' I load our manifest and compare a version field
- if our manifest is newer I call 'social.manifest.set'
- this in turn appears to cause a reload of worker/sidebar

I confirmed a request to load the new worker/sidebar but didn't actually check if the new worker is actually used. I just assume it is.

Comment 35

[u279076]

I'm calling this verified. Thanks for your help edA-qa mort-ora-y.