1359294 - Permaorange on Android in test262/built-ins/String/prototype/toLocaleUpperCase/special_casing.js when Gecko 55 merges to beta on 2017-06-12

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Phil Ringnalda (:philor)]

It's a pain to see it, since I had to file two harness bugs on my way to filing this, but if you search https://public-artifacts.taskcluster.net/ELJuP6gISpCziQnZ0Wt1Eg/0/public/logs/live_backing.log for the string "unexpected" you'll find

INFO -  REFTEST TEST-START | http://10.0.2.2:8854/jsreftest/tests/jsreftest.html?test=test262/built-ins/String/prototype/toLocaleUpperCase/special_casing.js
INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/jsreftest/tests/jsreftest.html?test=test262/built-ins/String/prototype/toLocaleUpperCase/special_casing.js | 258 / 677 (38%)
INFO -  {"action":"test_end","time":1493046584589,"thread":null,"pid":null,"source":"reftest","test":"http://10.0.2.2:8854/jsreftest/tests/jsreftest.html?test=test262/built-ins/String/prototype/toLocaleUpperCase/special_casing.js","status":"FAIL","expected":"PASS","message":"Unknown :0: uncaught exception: Test262Error: LATIN SMALL LETTER SHARP S Expected SameValue(�߻, �SS�) to be true item 1","extra":{"status_msg":"UnexpectedFail"}}
INFO -  REFTEST INFO | Saved log: START http://10.0.2.2:8854/jsreftest/tests/jsreftest.html?test=test262/built-ins/String/prototype/toLocaleUpperCase/special_casing.js
INFO -  REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering AfterOnLoadScripts
INFO -  REFTEST INFO | Saved log: Initializing canvas snapshot
INFO -  REFTEST INFO | Saved log: [CONTENT] AfterOnLoadScripts belatedly entering WaitForTestEnd
INFO -  REFTEST INFO | Saved log: [CONTENT] WaitForTestEnd: Adding listeners
INFO -  REFTEST INFO | Saved log: Initializing canvas snapshot
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: dispatching MozReftestInvalidate
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_SPELL_CHECKS
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: APZ flush not required
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FINISH
INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: Completed
INFO -  REFTEST INFO | Saved log: [CONTENT] RecordResult fired
INFO -  REFTEST INFO | Saved log: RecordResult fired

which is going to be permaorange on both opt and debug Android when mozilla-central merges to mozilla-beta on 2017-06-12.

Comment 1

[Phil Ringnalda (:philor)]

[Tracking Requested - why for this release]: merge permaorange, closed tree, delayed b1, wailing and rending of garments.

Comment 2

[Marcia Knous [:marcia]]

Tracking 55+ for this permaorange.

Comment 3

[André Bargull [:anba]]

We've got two options here:
- Disable the special casing tests when EXPOSE_INTL_API isn't set.
- Or stop using the localeToUpperCase/localeToLowerCase callbacks in XPCLocale.

Comment 4

[Shu-yu Guo [:shu]]

anba, what's causing the failure here? And which option do you think is best? I really have very little opinion regarding Intl stuff so I'm happy to defer.

Comment 5

[André Bargull [:anba]]

(In reply to Shu-yu Guo [:shu] from comment #4)
> anba, what's causing the failure here?

In bug 1318403, I've updated String.prototype.to[Locale]{Lower,Upper}Case() to support special-casing rules, e.g. ß (sharp-S) is now properly upper-cased to SS. But when EXPOSE_INTL_API isn't set (= Android), str_toLocaleUpperCase still uses the old "localeCallback" functionality (http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/js/src/jsstr.cpp#1249-1256). In this case, http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/js/xpconnect/src/XPCLocale.cpp#119 calls http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/intl/unicharutil/util/nsUnicharUtils.cpp#192, which does a character-by-character upper-case conversion without applying special-casing rules.

> And which option do you think is best? I really have very little opinion regarding Intl stuff so I'm happy to defer.

I'd probably remove these two lines http://searchfox.org/mozilla-central/rev/876c7dd30586f9c6f9c99ef7444f2d73c7acfe7c/js/xpconnect/src/XPCLocale.cpp#108-109, because the current implementation of the case-conversion functions in jsstr.cpp provides better spec compliance and is also faster.

Comment 6

[André Bargull [:anba]]

Removing the callbacks (https://hg.mozilla.org/try/rev/6a2888b2b112) seems to work: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d4b0898d94a39d150404da2cdb26385e0e2a4d6

Comment 7

[Shu-yu Guo [:shu]]

(In reply to André Bargull from comment #6)
> Removing the callbacks (https://hg.mozilla.org/try/rev/6a2888b2b112) seems
> to work:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6d4b0898d94a39d150404da2cdb26385e0e2a4d6

Could you please put this patch up and r? Waldo. Thanks!

Comment 8

[André Bargull [:anba]]

Attachment: bug1359294-part1-remove-case-callbacks-from-xpclocale.patch

This removes the toLocaleUpper/LowerCase callbacks from XPCLocale, because they don't appear to be useful anymore (comment #5).

Comment 9

[André Bargull [:anba]]

Attachment: bug1359294-part2-remove-case-callbacks-from-jsapi.patch

I don't know if it's okay to remove these callbacks from jsapi, other embedders may still use them?

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 8863895 [details] [diff] [review]
bug1359294-part1-remove-case-callbacks-from-xpclocale.patch

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

::: js/xpconnect/src/XPCLocale.cpp
@@ +77,5 @@
>    {
>      MOZ_COUNT_CTOR(XPCLocaleCallbacks);
>  
> +    localeToUpperCase = nullptr;
> +    localeToLowerCase = nullptr;

Put a comment by these to the effect that this makes toLocaleUpperCase/toLocaleLowerCase -- iff these hooks are consulted at all, which they are not in Intl-enabled builds -- behave identical to toUpperCase/toLowerCase.  Ideally run the wording past me/#jsapi if you can.

Comment 11

[André Bargull [:anba]]

Attachment: bug1359294.patch

Updated per review comments, carrying r+ from Waldo.

Comment 12

[André Bargull [:anba]]

Try (just compilation): https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe1bff8711e6a47701a718fc73ca22489d9ca871
Try (with an additional commit to pref-off ICU on Android Nightly to validate tests succeed): https://treeherder.mozilla.org/#/jobs?repo=try&revision=dec02b35ae7be6f1e01a83abdda85204fdfdce5a

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8bf16b7579c
Remove toLocaleLowerCase/toLocaleUpperCase callbacks in XPCLocale. r=Waldo

Comment 14

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/d8bf16b7579c