1268912 - Sync's client.name2 string should use a curly apostrophe/quote instead of a straight quote

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

As part of bug 1268159, the rest of the quote characters in strings were converted to curly quotes. However, the apostrophe in sync.properties for client.name2 seems to have some affects on other code.

When the ASCII apostrophe was converted to a Unicode \u2019 character quite a few xpcshell tests failed.


TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_specified.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_specified.js | test_bothEnginesSpecified - [test_bothEnginesSpecified : 124] ["steam"] deepEqual ["steam","stirling"]
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_specified.js | test_bothEnginesSpecified - [test_bothEnginesSpecified : 139] ["stirling"] deepEqual ["stirling","steam"]
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_specified.js | test_bothEnginesDefault - [test_bothEnginesDefault : 153] ["steam"] deepEqual ["steam","stirling"]
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_tab_engine.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_updateEnabledEngines.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_updateEnabledEngines.js | test_disabledRemotelyTwoClients - [test_disabledRemotelyTwoClients : 316] false == true
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_service_sync_updateEnabledEngines.js | test_dependentEnginesEnabledLocally - [test_dependentEnginesEnabledLocally : 379] false == true
TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_clients_engine.js | xpcshell return code: 0 

My guess is that the unicode character is tripping up some other code in the mock testing server.

:rnewman, any ideas here?

Comment 1

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

Thom,
  Would you please have a look at this when you get a chance.

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Attachment: Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

Review commit: https://reviewboard.mozilla.org/r/64050/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64050/

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8770620 [details]
Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

(Trying to clear out some of my old bugs).

So, doing this correctly ends up being quite painful, since it requires going through all the sync tests and checking every JSON.parse/stringify to make sure that it has decoded or encoded UTF-8 at the right time. At the moment, the assumption they all implicitly make is that encryption/decryption is the same as JSON.stringify/parse. Which it is, so long as the text is all ascii (which it is for the tests, except after this patch).

The easy way to avoid this is to clobber the only method that looks at client.name2, which is what I've done. I'm not sure if there's a better option, due to how much work it seems like it would be to fix the tests, although it's clearly less than ideal (and its certainly possible, if not likely, that the same problem will crop up again for future tests).

Requesting feedback since I'm not sure this an alright approach, and I'm also not this was all I needed to do for this bug (e.g. how much of bug 1268159 this should cover, since it looks like most of it landed?)

Comment 4

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

https://reviewboard.mozilla.org/r/64050/#review61188

This sucks, but I agree it isn't a yak we need to shace now. I'm confident non-ascii chars in device names work correctly, so yeah, this sounds fine.

::: services/sync/locales/en-US/sync.properties:6
(Diff revision 1)
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # %1: the user name (Ed), %2: the app name (Firefox), %3: the operating system (Android)
> -client.name2 = %1$S's %2$S on %3$S
> +client.name2 = %1$S’s %2$S on %3$S

We aren't allowed to change the value for an existing string as it will not be picked up by localizers. ie, we need to rename this to (say) client.name3 and update all references.

::: services/sync/tests/unit/head_helpers.js:396
(Diff revision 1)
>      }
>    });
>  }
> +
> +// Avoid an issue where `client.name2` containing unicode characters causes
> +// a number of tests to fail, due to them assuming that we do not need to utf-8

I *think* the issue is more like "inconsistent utf-8 encoding/decoding in the tests vs the sync code" as IIUC, normal non-encoded unicode chars can survive a parse/stringify round trip.

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

https://reviewboard.mozilla.org/r/64050/#review61188

> We aren't allowed to change the value for an existing string as it will not be picked up by localizers. ie, we need to rename this to (say) client.name3 and update all references.

Ah, right, thanks, I think I knew this at one point.

> I *think* the issue is more like "inconsistent utf-8 encoding/decoding in the tests vs the sync code" as IIUC, normal non-encoded unicode chars can survive a parse/stringify round trip.

Unless I'm misunderstanding, this isn't correct, as sending non-utf8 encoded strings to the server causes C++ to become very upset when it tries to convert them into ascii. And so while they survive the parse/stringify round trip, they don't survive going through the server, since it comes out as a 500 error.

Comment 6

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8770620 [details]
Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64050/diff/1-2/

Comment 7

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8770620 [details]
Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64050/diff/2-3/

Comment 8

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

Comment on attachment 8770620 [details]
Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

https://reviewboard.mozilla.org/r/64050/#review61412

Thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/9125083b8dfe
Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests. r=markh

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for perma-timeouts in test_clients_engine.js. Not sure which Sync patch was actually responsible for it, but this didn't have a green Try link, so you'll need to sort it out.

https://treeherder.mozilla.org/logviewer.html#?job_id=10588073&repo=fx-team#L7634

Comment 11

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/98979093d45f
Backed out changesets 48d6706662c0, 53236b4393f2, and 9125083b8dfe (bug 1268912, bug 1244597) for test_clients_engine.js perma-timeouts.

Comment 12

[Francesco Lodolo [:flod]]

For future reference, this kind of changes (curly apostrophe instead of straight single quote) doesn't require a new string ID
https://hg.mozilla.org/mozilla-central/diff/9125083b8dfe/services/sync/locales/en-US/sync.properties

Comment 13

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8770620 [details]
Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64050/diff/3-4/

Comment 14

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8770620 [details]
Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64050/diff/4-5/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/8be536c3838c
Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests. r=markh

Comment 16

[Carsten Book [:Tomcat]]

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