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

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jaws, Assigned: tcsc)

Tracking

Trunk
Firefox 50
Points:
---
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(firefox49 affected, firefox50 fixed)

Details

Attachments

(1 attachment)

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?
Flags: needinfo?(rnewman)
Thom,
  Would you please have a look at this when you get a chance.
Assignee: nobody → tchiovoloni
Flags: needinfo?(rnewman) → firefox-backlog+
Priority: -- → P3
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?)
Attachment #8770620 - Flags: feedback?(markh)
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.
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 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/
Attachment #8770620 - Flags: feedback?(markh)
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/
Attachment #8770620 - Attachment description: Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests f?markh → Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests
Attachment #8770620 - Flags: review?(markh)
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!
Attachment #8770620 - Flags: review?(markh) → review+
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
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
Backout by ryanvm@gmail.com:
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.
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 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/
Attachment #8770620 - Attachment description: Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests → Bug 1268912 - Use unicode apostrophe for client.name2 in sync properties, working around issues with sync tests.
Priority: P3 → P1
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/
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8be536c3838c
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
You need to log in before you can comment on or make changes to this bug.