908461 - Protocol deprecation indicators: client support

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

1. This service is shutting down soon. Sent on success responses, triggers UI like "The old Firefox Sync service is shutting down soon. [Learn more]."

  2. This service is shutting down really soon. Sent as a failure response. Triggers UI like "The old Firefox Sync service is no longer available. To continue syncing you need to take action. [Learn more]."

Comment 1

[Richard Newman [:rnewman]]

Attachment: Soft EOL, non-final strings.

Comment 2

[Richard Newman [:rnewman]]

Attachment: Hard EOL, non-final strings.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Includes some small fixes to head_http_server to avoid mixed-return warnings.

Strings open to change.

Tests verify that we notify (which will hit the UI code), adjust sync intervals in the hard-eol case, and throw or not as we always have for error responses.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 796259 [details] [diff] [review]
Proposed patch. v1

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

I mostly just glanced at this. This looks good to me. I assume you are trying to get this in the lowest numbered Firefox as possible. Please don't hesitate to badger me about reviews.

::: services/sync/modules/policies.js
@@ +848,5 @@
>      switch (resp.status) {
> +      case 200:
> +      case 404:
> +      case 513:
> +        let xwa = resp.headers['x-weave-alert'];

Shouldn't we check xwa on *every* response?

Comment 5

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Please don't hesitate to badger me about reviews.

Thanks!


> Shouldn't we check xwa on *every* response?

In theory, yes, we should have a response filter that does something with X-W-A on every response. (It's hilarious to have a messaging channel in the spec that the client does absolutely nothing with.)

But here we're just (mis-)using X-W-A as a structured side-channel.

Given that I want to uplift this, and I don't want to take the risk of breaking error handling elsewhere, I took some pains to be less invasive than I could have been.

In this case, I'm mainly targeting _fetchInfo (that is, we check for this once per sync), not altering whether an error is thrown, and only checking in the cases where we think this kind of error might be returned -- stable successful fetches (soft EOL), 513 (the new error code; hard EOL), and 404 (because we've seen that bug before, and perhaps that'll be the state we hit during decommissioning).

Comment 6

[Richard Newman [:rnewman]]

On the subject of strings:

These messages are to be trapped in amber before we implement Firefox Account-aware messaging (Bug 908465). Some clients will be far enough behind that they'll see these, not the new stuff.

Early in the transition to Sync.next, we'll be encouraging migration via storage indicators and FxA-aware messaging. The two services will run concurrently with no EOL messaging.

The two indicators in this bug will come into play when we begin a firmer ramp-down (soft-eol), and then when we stop providing service (hard-eol), and will be seen by two groups of people:

* Those who are sticking with Fx 25-28 (say) for some delierate reason, or who have fallen out of automatic updates somehow.
* Those laggards who might have upgraded Firefox enough to get FxA on another device, but not this one, before the EOL period begins -- they won't get the storage indicators or FxA-aware messaging, but all they need to do is upgrade for things to start working (until hard EOL, of course).

As such, the goals are:

* Include a Learn More link that will advise the user to upgrade. We might want a SUMO page for this, rather than just hitting getfirefox.com.
* Have vague enough wording that we don't trap the wrong sentiments in amber, should our plans not survive contact with the enemy.
* Lesser: be general enough that other Sync service providers (e.g., ownCloud) could start sending these responses and not confuse the heck out of users.

Comment 7

[Richard Newman [:rnewman]]

Madhava: can you suggest someone to review my wording for this and the equivalent on Android? Ordinarily I'd bug Ian, but he's afk for a couple of days, and I know he has a full plate…

Comment 8

[Richard Newman [:rnewman]]

Decided to change the strings to say "Your":

warning.sync.eol.label = Service Shutting Down
warning.sync.eol.description = Your Firefox Sync service is shutting down soon. Upgrade Firefox to keep syncing.
error.sync.eol.label = Service Unavailable
error.sync.eol.description = Your Firefox Sync service is no longer available. You need to upgrade Firefox to keep syncing.


This makes more sense from two directions:

* For self-hosted users, it acknowledges that it's not "the" service.
* It implies that there's a different (new?) Sync service that you get when you upgrade.


Also clearing info, 'cos Madhava's on vacation.

Comment 9

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Just the string changes; otherwise the same as v1.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Experience on Beta.

This reuses existing strings.

Comment 11

[Richard Newman [:rnewman]]

Attachment: Experience on Beta.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Patch for Aurora and Beta. (no strings) v1

Comment 13

[Richard Newman [:rnewman]]

Attachment: Patch for Aurora and Beta. (no strings) v2

One day I'll qref every time.

Comment 14

[Richard Newman [:rnewman]]

Attachment: Patch for Aurora and Beta. (no strings) v3

... and now with a change to have the test work without the dynamic port/URL stuff.

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 796904 [details] [diff] [review]
Proposed patch. v2

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

Please check with l10n crowd before landing.

::: browser/base/content/browser-syncui.js
@@ +211,5 @@
>  
> +  onEOLNotice: function (data) {
> +    let code = data.code;
> +    let kind = (code == "hard-eol") ? "error" : "warning";
> +    let url = data.url || "https://www.mozilla.org/firefox/?utm_source=synceol";

The URL feels like something that shouldn't be inline. File-level constant or even pref seem more appropriate.

::: services/sync/locales/en-US/sync.properties
@@ +40,5 @@
>  error.sync.quota.description = Sync failed because it exceeded the server quota. Please review which data to sync.
>  error.sync.viewQuotaButton.label = View Quota
>  error.sync.viewQuotaButton.accesskey = V
> +warning.sync.eol.label = Service Shutting Down
> +warning.sync.eol.description = Your Firefox Sync service is shutting down soon. Upgrade Firefox to keep syncing.

I'm pretty sure localizers won't like seeing "Firefox Sync" and "Firefox" inline. Shouldn't these be &brandShortName or something like that? Might want to get additional review from somebody who knows more.

::: services/sync/modules/policies.js
@@ +865,5 @@
> +        }
> +
> +        this.handleServerAlert(xwa);
> +        break;
> +        

Nit: ws

@@ +870,1 @@
>        case 400:

I prefer my switch case statements in order. But this code is EOL, right? If so, we don't care too much about maintainability, so meh.

Comment 16

[Gregory Szorc [:gps]]

Comment on attachment 796945 [details] [diff] [review]
Patch for Aurora and Beta. (no strings) v3

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

Please be sure this is marked for manual QA verification, especially since we're presumably landing this so late in a Beta cycle.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #15)

> I'm pretty sure localizers won't like seeing "Firefox Sync" and "Firefox"
> inline. Shouldn't these be &brandShortName or something like that? Might
> want to get additional review from somebody who knows more.

I did this for three reasons:

1. The rest of the file is full of them. 
2. More broadly, "Firefox Sync" is a proper noun, not a brand element; even if you're running Nightly it's called "Firefox Sync".
3. As I understand it, using entities in property files is a pain in the ass.

But I feel your pain.

Comment 18

[Richard Newman [:rnewman]]

N.B., I do need to rework the patch to catch the "upgrade Firefox" inclusion!

Comment 19

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Added a Metro-compatible way of getting the app name, and format it into the description string.

Fixed whitespace, introduced a pseudo-constant for the default URL.

Comment 20

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/9e834d0decc2

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 796945 [details] [diff] [review]
Patch for Aurora and Beta. (no strings) v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  New work to allow for graceful EOL of old Sync service.

User impact if declined: 
  Less pleasant EOL experience. We want this as far up as we can get it to catch the most users.

Testing completed (on m-c, etc.): 
  Manually tested. Just landed on s-c, merging soon.

Risk to taking this patch (and alternatives if risky): 
  Should be slim -- extension.

String or IDL/UUID changes made by this patch:
  None for the uplift patch.

Comment 22

[Alex Keybl [:akeybl]]

(In reply to Richard Newman [:rnewman] from comment #21)
> Comment on attachment 796945 [details] [diff] [review]
> Patch for Aurora and Beta. (no strings) v3
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
>   New work to allow for graceful EOL of old Sync service.
> 
> User impact if declined: 
>   Less pleasant EOL experience. We want this as far up as we can get it to
> catch the most users.
> 
> Testing completed (on m-c, etc.): 
>   Manually tested. Just landed on s-c, merging soon.
> 
> Risk to taking this patch (and alternatives if risky): 
>   Should be slim -- extension.
> 
> String or IDL/UUID changes made by this patch:
>   None for the uplift patch.

We'd rather not take any risk in FF24 at this point - can you speak to the benefit of taking this patch one release sooner? How many releases until this is fully deprecated?

Comment 23

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/mozilla-central/rev/9e834d0decc2

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 796945 [details] [diff] [review]
Patch for Aurora and Beta. (no strings) v3

(In reply to Alex Keybl [:akeybl] from comment #22)

> We'd rather not take any risk in FF24 at this point - can you speak to the
> benefit of taking this patch one release sooner? How many releases until
> this is fully deprecated?

It depends on a number of things: the release date for a system that replaces Sync (could be December), the rate of adoption of that system (they'll overlap for a few releases), and the rate of user upgrades across all Firefox versions.

I *hope* the number of users who will still be using Fx24 when we start the EOL process will be small, so if you're tightening the risk crank, that's totally fine by me.

Comment 25

[James Bonacci [:jbonacci]]

QA will hang on for final decision on where this is going to land. Then I will be bugging :nalexander and :rnewman about test requirements for this bug/patch...

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4285cf92c4a7

Comment 27

[Richard Newman [:rnewman]]

That patch needed to be adjusted for Aurora.

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

Comment 28

[James Bonacci [:jbonacci]]

:rnewman and :gps
https://bugzilla.mozilla.org/show_bug.cgi?id=908461#c16

Do you have some specific STRs or QA requirements for this manual testing?
What would you like to see for Nightly, Aurora, Beta?

Comment 29

[Richard Newman [:rnewman]]

(In reply to James Bonacci [:jbonacci] from comment #28)
> :rnewman and :gps
> https://bugzilla.mozilla.org/show_bug.cgi?id=908461#c16
> 
> Do you have some specific STRs or QA requirements for this manual testing?
> What would you like to see for Nightly, Aurora, Beta?

This is not something that can be manually tested until there's server support -- Bug 908467. Shouldn't be a lot of work.

Behavior will be something like "when the server sends a hard EOL, show this; when it sends a soft EOL, show this other thing, verify that sync intervals change accordingly". I'll help you thrash out a more detailed plan when we have server support.

Comment 30

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

Comment on attachment 797374 [details] [diff] [review]
Proposed patch. v3

>diff --git a/browser/base/content/browser-syncui.js b/browser/base/content/browser-syncui.js

>+  _getAppName: function () {
>+    try {
>+      let syncStrings = new StringBundle("chrome://browser/locale/sync.properties");
>+      return syncStrings.getFormattedString("sync.defaultAccountApplication", [brandName]);
>+    } catch (ex) {}

Looks like this might not matter since it's never called for Metro, but this will always throw because "brandName" is undefined. I filed bug 989825.