Closed Bug 1354055 Opened 7 years ago Closed 7 years ago

Don't cache wrong result in OSPreferences::ReadSystemLocales

Categories

(Core :: Internationalization, enhancement)

Product:
Core
Component:
Internationalization
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: zbraniecki, Assigned: zbraniecki)

References

Details

Attachments

(1 file)

Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android.
59 bytes, text/x-review-board-request
rnewman
: review+
jfkthame
: review+
Details 
Due to the hack we're using on Android, where we read the system locales from a pref, instead of from the system, we run into some race condition where we attempt to read the pref before prefs are ready.

This causes us to "cache" en-US as a locale irrelevant of what the real locale is.
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Comment on attachment 8855264 [details]
Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android.

https://reviewboard.mozilla.org/r/127142/#review129972

Seems reasonable!
Attachment #8855264 - Flags: review?(rnewman) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5d3bc5f2c41f
Don't cache wrong result in OSPreferences::ReadSystemLocales on Android. r=rnewman
https://hg.mozilla.org/mozilla-central/rev/5d3bc5f2c41f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Backed out in https://hg.mozilla.org/mozilla-central/rev/19e555dcd7e2 for mass Android reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=89470738&repo=autoland
Status: RESOLVED → REOPENED
status-firefox55: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla55 → ---
Can you help me understand how's the failure related to the patch? I can't see any relation.
Flags: needinfo?(rnewman)
The reftests are failing because the font size is wrong. If you grab the two lines below each test failure, and base64-decode them, you'll get images, and indeed they differ. In the case of the one I grabbed, the reference had a smaller font than the test.

I expect these are failing because the locale is, apparently, used to decide on the font size:

INFO -  REFTEST INFO | SET PREFERENCE pref(font.size.variable.x-western,24)
INFO -  REFTEST INFO | SET PREFERENCE pref(font.size.variable.zh-HK,36)
INFO -  REFTEST TEST-LOAD | http://10.0.2.2:8854/tests/layout/reftests/reftest-sanity/font-size-24.html | 50 / 1281 (3%)
INFO -  REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/reftest-sanity/font-default.html == http://10.0.2.2:8854/tests/layout/reftests/reftest-sanity/font-size-24.html | image comparison, max difference: 255, number of differing pixels: 1644

I suspect that your patch is now making things reflect locale choices in a way it previously did not -- specifically the OS locale rather than defaulting to en-US -- and that now means we're getting different font sizes on Android!

I have no idea how font rendering is done, so that's pretty much the limit of my understanding.
Flags: needinfo?(rnewman)
I'm sorry for bothering you more, but I can't seem to be able to even run this test, which makes it impossible to investigate it :(

I tried both on linux and mac, following https://wiki.mozilla.org/Mobile/Fennec/Android/Testing#reftests_.28and_crashtests_and_js-reftests.29 - connected Android to the same Wifi, and USB made sure that `adb devices` shows the device and all I'm getting is:

https://pastebin.com/KN8KUSHz

I don't understand this log and I'm not sure how to debug it. Need some help to get this bug landed from the Fennec team, I'm afraid.
Flags: needinfo?(rnewman)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #11)

> I don't understand this log and I'm not sure how to debug it. Need some help
> to get this bug landed from the Fennec team, I'm afraid.

Over to someone on the Fennec team…
Flags: needinfo?(s.kaspari)
Flags: needinfo?(rnewman)
Flags: needinfo?(gbrown)
Nothing jumps out at me, except perhaps "detected broken kqueue" -- I haven't seen that before.

Does Firefox start? (Do you see it open on your device?) Does it remain open?

Does "adb logcat" show firefox starting? Any errors in there?

Have you tried running on an emulator instead? (Disconnect your device and run your mach command again - it should offer to start an emulator.)

fwiw, './mach reftest layout/reftests/reftest-sanity/reftest.list' works for me, on Ubuntu 16, with an emulator.


I'll take a closer look tomorrow.
I see "org.mozilla.fennec_zbraniecki still alive after SIGABRT: waiting..." and "Cmd line: org.mozilla.fennec_mcomella". Are there two different builds of Fennec installed? The _mcomella reference is from an old ANR report, so possibly a red herring, but perhaps worth checking.

I'd check on installed apps with something like "adb shell pm list packages | grep mozilla", then "adb uninstall org.mozilla.fennec_xxx" until only the desired one is installed.

If that doesn't help, try my suggestions from comment 13.

Still not working? Import the patch from bug 1355222, then run 

  ./mach reftest --log-tbpl-level=debug layout/reftests/reftest-sanity/reftest.list

and show me the output.
Flags: needinfo?(gbrown)
(In reply to Richard Newman [:rnewman] from comment #9)
> The reftests are failing because the font size is wrong. If you grab the two
> lines below each test failure, and base64-decode them, you'll get images,
> and indeed they differ.

There's no need to actually manually decode them - in the Logviewer, click on "Open Analyzer" and you can directly view and compare the failing reftest results.
Comment on attachment 8855264 [details]
Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android.

Ok, I think I fixed it.

Boy, was it hard to debug.

So, basically, in several places in fonts we use systemLocale assuming that it'll have some value.
But in the test environment on Android, the `intl.locale.os` may be empty and in that case we return empty for systemLocale.

That caused the reftest errors.

I unified the calls in OSPreferences to use one GetSystemLocales (instead of repeating the logic in 3 calls) and added a hack so that:

a) we don't cache empty results (when ReadSystemLocales returns false)
b) we assign "en-US" when we can't read a locale

This fixes the reftests and I think is a reasonable solution, until we get the proper system locale in Android.

It's fairly urgent as we need to land it and verify before the merge day and I'd like to get a review on this from :jfkthame.
Flags: needinfo?(s.kaspari)
Attachment #8855264 - Flags: review?(jfkthame)
Comment on attachment 8855264 [details]
Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android.

https://reviewboard.mozilla.org/r/127142/#review132104

::: intl/locale/OSPreferences.cpp:46
(Diff revision 3)
> -  }
> -  aRetVal = mSystemLocales;
> +    aRetVal = mSystemLocales;
> -  return status;
> +    return true;
> +  }
> +
> +  if(ReadSystemLocales(aRetVal)) {

nit, space before the open-paren

::: intl/locale/OSPreferences.cpp:52
(Diff revision 3)
> +    mSystemLocales = aRetVal;
> +    return true;
> +  }
> +
> +  // If we failed to get the system locale, we still need
> +  // to return something because tere are tests our there that

typos:
s/tere/there/
s/our/out/

::: intl/locale/OSPreferences.cpp:307
(Diff revision 3)
> -  *aCount = mSystemLocales.Length();
> +  AutoTArray<nsCString, 10> systemLocales;
> +  GetSystemLocales(systemLocales);

This is a bit unfortunate, as it means that (just because of the ugly android hack), we're doing an extra copy of the array of strings every time this is called: first we copy them from mSystemLocales into the local systemLocales array (so it allocates a new nsCString for each entry), and then into the newly-allocated output array.

I think we should try to avoid this extra copy, maybe something like this (untested):

    AutoTArray<nsCString,10> tempLocales;
    nsTArray<nsCString>* systemLocalesPtr;
    if (!mSystemLocales.IsEmpty()) {
      // use cached value
      systemLocalesPtr = &mSystemLocales;
    } else {
      // get a (perhaps temporary/fallback/hack) value
      GetSystemLocales(tempLocales);
      systemLocalesPtr = &tempLocales;
    }
    *aCount = systemLocalesPtr->Length();
    ...etc

so that we only do the extra call to the other GetSystemLocales, with the associated copying, in the (rare) event that we don't have a cached value to use.

::: intl/locale/OSPreferences.cpp:323
(Diff revision 3)
> -  if (mSystemLocales.IsEmpty()) {
> -    ReadSystemLocales(mSystemLocales);
> +  AutoTArray<nsCString, 10> systemLocales;
> +  GetSystemLocales(systemLocales);

Similar, let's avoid the extra call and copy if we've got a cached value. Suggestion:

    if (!mSystemLocales.IsEmpty()) {
      aRetVal = mSystemLocales[0];
    } else {
      AutoTArray<nsCString,10> locales;
      GetSystemLocales(locales);
      if (!locales.IsEmpty()) {
        aRetVal = locales[0];
      }
    }
Attachment #8855264 - Flags: review?(jfkthame)
Comment on attachment 8855264 [details]
Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android.

Thanks :jfkthame! Updated the patch and re-requesting review.
Attachment #8855264 - Flags: review?(jfkthame)
Comment on attachment 8855264 [details]
Bug 1354055 - Don't cache wrong result in OSPreferences::ReadSystemLocales on Android.

https://reviewboard.mozilla.org/r/127142/#review132184
Attachment #8855264 - Flags: review?(jfkthame) → review+
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8652c254c6e2
Don't cache wrong result in OSPreferences::ReadSystemLocales on Android. r=jfkthame,rnewman
https://hg.mozilla.org/mozilla-central/rev/8652c254c6e2
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: