re-activation to update provider manifest needs ux

VERIFIED FIXED in Firefox 24

Status

()

Firefox
SocialAPI
--
minor
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: mixedpuppy)

Tracking

Trunk
Firefox 25
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox24 verified, firefox25 verified)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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

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

Updated

5 years ago
Blocks: 859886
(Assignee)

Comment 2

5 years ago
Created attachment 763720 [details] [diff] [review]
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)
Assignee: nobody → mixedpuppy
Attachment #763720 - Flags: review?(gavin.sharp)
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.
markh is probably a better reviewer, since he seems closer to this stuff?
(Assignee)

Updated

5 years ago
Attachment #763720 - Flags: review?(gavin.sharp) → review?(mhammond)
(Assignee)

Comment 5

5 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
(Assignee)

Comment 6

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

not quite working yet, will have to examine this more.
Attachment #763720 - Attachment is obsolete: true
Attachment #763720 - Flags: review?(mhammond)
(Assignee)

Comment 7

5 years ago
Created attachment 763955 [details] [diff] [review]
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.
Attachment #763955 - Flags: review?(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

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

Comment 10

5 years ago
(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.
(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?
(Assignee)

Comment 12

5 years ago
Created attachment 764990 [details] [diff] [review]
fix updating manifests, add worker api for self-update

https://tbpl.mozilla.org/?tree=Try&rev=102a5a53163e
Attachment #763955 - Attachment is obsolete: true
Attachment #763955 - Flags: review?(mhammond)
Attachment #764990 - Flags: review?(mhammond)
(Assignee)

Comment 13

5 years ago
Created attachment 765178 [details] [diff] [review]
fix updating manifests, add worker api for self-update

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

Comment 15

5 years ago
Created attachment 765189 [details] [diff] [review]
fix updating manifests, add worker api for self-update

feedback added

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

Comment 17

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

Comment 18

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

Comment 19

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

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

Comment 20

5 years ago
try four, remove another references that looks circular

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

Comment 21

5 years ago
Created attachment 766478 [details] [diff] [review]
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
Attachment #765189 - Attachment is obsolete: true
Attachment #766478 - Flags: review+
(Assignee)

Comment 23

5 years ago
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
Attachment #766478 - Flags: approval-mozilla-aurora?
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

Updated

5 years ago
Attachment #766478 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8dd428369503
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
(Assignee)

Comment 28

5 years ago
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
Attachment #766478 - Flags: approval-mozilla-aurora?
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
Attachment #766478 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/f2b49f275dff
status-firefox24: --- → fixed
status-firefox25: --- → fixed
Florian and edA-qa, please check that this works as expected for you with tomorrow's Firefox Nightly and Aurora builds.
Flags: needinfo?(florian)
Flags: needinfo?(em)

Updated

5 years ago
Depends on: 886227

Comment 32

5 years ago
(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?
(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

5 years ago
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.
Flags: needinfo?(em)
I'm calling this verified. Thanks for your help edA-qa mort-ora-y.
Status: RESOLVED → VERIFIED
status-firefox24: fixed → verified
status-firefox25: fixed → verified
Flags: needinfo?(florian)
You need to log in before you can comment on or make changes to this bug.