New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 908114 link

Starred by 6 users

Issue metadata

Status: Fixed
Owner:
Closed: Nov 29
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-11-29
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

Finch locale logic is broken in M71

Project Member Reported by asvitk...@chromium.org, Nov 23

Issue description

Finch locale logic is broken in M71 on Mac.

This breaks Finch experiments targeting a specific locale - so e.g. we may show english strings to non-english language users if the experiment is using UI string overrides.

Fix is pretty straight forward, I have a CL for it.

Marking M71 RBS so that we don't break Finch on stable.
 
Looks like it might be broken on other platforms than Mac as well.

Here's a graph of ChromeSurvey study (which is locale targeted) whose actives shows a drop around Oct 5:

https://uma.googleplex.com/variations?sid=cfcfce8b3df03faa4d101020ece5d602

Which is when https://chromium-review.googlesource.com/q/1228615 landed.
Labels: OS-Android OS-Chrome OS-iOS OS-Linux OS-Windows
Setting OS all per the above.

Probably the logic is likely broken because intl.app_locale pref (prefs::kApplicationLocale) is often not set. We still want to use l10n_util::GetApplicationLocale() in that case, but the logic wasn't doing that

If that's the case, https://chromium-review.googlesource.com/c/chromium/src/+/1349770 should be fixing it.
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
Summary: Finch locale logic is broken in M71 (was: Finch locale logic is broken in M71 on Mac)
Updating title. It's not just broken on Mac - affects all platforms.
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 26

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1

commit 8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Mon Nov 26 21:55:29 2018

Fix variations locale logic broken in M71.

There were two issues:

1. The new language::GetApplicationLocale() function that was
introduced as part of M71 changes to variations service did not
match the previous behavior - it had an early return when the
locale pref didn't exist - which would be hit when the user didn't
explicitly set their language in Chrome. But that's not sufficient
since l10n_util::GetApplicationLocale() actually does logic beyond
what's provided to it via the pref. On Mac, it gets the language
from the system. The changes to components/language were sufficient
to fix things on Mac.

2. On non-Mac platforms, the call to l10n_util::GetApplicationLocale()
also had a dependency on ResourceBundle being initialized, so this part
did not work correctly either. IsLocaleAvailable() was returning false
when there was no ResourceBundle. There was also a TODO to make the
LocaleDataPakExists() API static - which then could be used without
ResourceBundle being initialized. This was mostly straight-forward
except for use of delegate_->GetPathForLocalePack(). This delegate
API was only overridden (outside of tests) by CastResourceDelegate.
Given cast doesn't use the code path in question (variations), the
code is changed to only use that delegate if the resource bundle has
been initialized.

Also adds a check that the locale determined after resource bundle
has been loaded matches what was used to initialize variations.

Bug:  908114 
Change-Id: Ief6bf3e370bf2773187387c1fd12fc4517b31a69
Reviewed-on: https://chromium-review.googlesource.com/c/1349770
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610954}
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/chrome/browser/metrics/chrome_feature_list_creator.cc
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/chrome/browser/metrics/chrome_feature_list_creator.h
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/components/language/core/browser/locale_util.cc
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/components/variations/service/variations_field_trial_creator.h
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/components/variations/service/variations_service.cc
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/components/variations/service/variations_service.h
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/l10n/l10n_util.cc
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/resource/resource_bundle.cc
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/resource/resource_bundle.h
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/resource/resource_bundle_android.cc
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/resource/resource_bundle_ios.mm
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/resource/resource_bundle_mac.mm
[modify] https://crrev.com/8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1/ui/base/resource/resource_bundle_unittest.cc

NextAction: 2018-11-27
Pls update this bug with canary result tomorrow.
Pls verify this bug on canary version 72.0.3623.0 tomorrow.
The NextAction date has arrived: 2018-11-27
CL from C#5 has introduced a top browser crash on Windows tracked in Issue 908791
Updated on Issue 908791. The fix was not sufficient to fix all instances of the original issue and the CHECK added in the CL uncovered cases where the fix was not sufficient. Working on a follow-up to address those ones.

The remaining issues (which currently cause a crash due to the CHECK) are believe to be Windows specific and I expect to have a fix for them today. Not planning to revert the CL unless I cannot find a fix today.
Thank you  asvitkine@. 
Pls note that this change will directly go to stable without beta coverage once approved for merge. So pls make sure itis super safe to merge. Merge has to happen latest by this Friday morning.
NextAction: 2018-11-29
Thank you, pls update bug with canary result on Thursday morning and request a merge to M71 if change looks good in canary.
iOS sends build to Apple on Thursday, so we needs the fix before Thursday. Did we verify on canary for iOS that the fix works?
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 28

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5814800fa70a7a7a7975c9943cdf0c310873321c

commit 5814800fa70a7a7a7975c9943cdf0c310873321c
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Wed Nov 28 01:05:16 2018

Fix Windows first run crash related to locale.

The crash would happen if you install Chrome of a specific language
and run it for the first time on a Windows OS of a different
language.

It was caused by code setting the language pref in-between the two
places where it was queried - such that the first time it was
queried resulted in a different value than the second.

This triggered the CHECK that was introduced by this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1349770

Which correctly identified the problem. This CL fixes the issue
by moving the importing of the first-run locale setting to
earlier.

Bug: 908791,  908114 
Change-Id: Ie2e0178f298f78d38e0bc7f0b58c2205bb617561
Reviewed-on: https://chromium-review.googlesource.com/c/1352482
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611480}
[modify] https://crrev.com/5814800fa70a7a7a7975c9943cdf0c310873321c/chrome/browser/chrome_resource_bundle_helper.cc
[modify] https://crrev.com/5814800fa70a7a7a7975c9943cdf0c310873321c/chrome/browser/metrics/chrome_feature_list_creator.cc

Labels: Target-71 Target-72 M-72
"iOS sends build to Apple on Thursday, so we needs the fix before Thursday. Did we verify on canary for iOS that the fix works?"

It doesn't look like there are any studies that use locale filtering on iOS, so the fix is not critical for iOS.
Labels: Merge-Request-71
Requesting merge for #c5 and #c15 to M71.

We don't have canary data for #c15 yet since it missed canary. :(

However #c5 is a pre-req for merging  crbug.com/907434  as well, which is approved for merge but is dependent on #c5.

I believe the fix in #c15 should be very safe since it just moves a very simple operation earlier. Without that fix, some new installs on Windows will encounter crashes.
Project Member

Comment 19 by sheriffbot@chromium.org, Nov 28

Labels: -Merge-Request-71 Hotlist-Merge-Review Merge-Review-71
This bug requires manual review: Less than 2 days to go before AppStore submit on M71
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Pls merge the changes to current canary branch 3624, so I can trigger new canary from same branch and update bug with canary result tomorrow morning.
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 28

Labels: merge-merged-3624
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0fff6adab912ce24c61f12f52aced29cefeaaf81

commit 0fff6adab912ce24c61f12f52aced29cefeaaf81
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Wed Nov 28 16:18:48 2018

Fix Windows first run crash related to locale.

The crash would happen if you install Chrome of a specific language
and run it for the first time on a Windows OS of a different
language.

It was caused by code setting the language pref in-between the two
places where it was queried - such that the first time it was
queried resulted in a different value than the second.

This triggered the CHECK that was introduced by this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1349770

Which correctly identified the problem. This CL fixes the issue
by moving the importing of the first-run locale setting to
earlier.

TBR=asvitkine@chromium.org

(cherry picked from commit 5814800fa70a7a7a7975c9943cdf0c310873321c)

Bug: 908791,  908114 
Change-Id: Ie2e0178f298f78d38e0bc7f0b58c2205bb617561
Reviewed-on: https://chromium-review.googlesource.com/c/1352482
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#611480}
Reviewed-on: https://chromium-review.googlesource.com/c/1352673
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/branch-heads/3624@{#3}
Cr-Branched-From: b54fb9cc3bbc522b006c38f4a3e88cbda2be1c59-refs/heads/master@{#611430}
[modify] https://crrev.com/0fff6adab912ce24c61f12f52aced29cefeaaf81/chrome/browser/chrome_resource_bundle_helper.cc
[modify] https://crrev.com/0fff6adab912ce24c61f12f52aced29cefeaaf81/chrome/browser/metrics/chrome_feature_list_creator.cc

Cc: abdulsyed@chromium.org benmason@chromium.org kariahda@chromium.org srinivassista@chromium.org
Triggering new canary from branch 3624 for Android, Desktop and iOS with merge listed at #21.


Canary version 72.0.3624.2 currently building with this merge in.
Labels: -OS-iOS
This fix is not critical for iOS in M71, so I am removing that platform.

asvitkine: please create M72 RBS to merge this for M72 if you haven't already. Thanks!
Cc: gov...@chromium.org
Crashes are fixed in 72.0.3624.2 canary. Pinging for merge to M71. (Merge label already set.)
Labels: -Hotlist-Merge-Review -Merge-Review-71 Merge-Approved-71
Merge approved to 71, branch 3578.
Pls mark this bug as fixed after the merge if nothing else is pending. Thank you.
Looks like there are some CLs that this change depends on that only landed in M72 that also need to be merged:
https://chromium-review.googlesource.com/c/1294591 
https://chromium-review.googlesource.com/c/chromium/src/+/1292516

Per chatting with Krishna, will go ahead and merge those as well. I merged the first, but the second is running into merge conflicts. :( Will look more tomorrow.
The NextAction date has arrived: 2018-11-29
Project Member

Comment 30 by bugdroid1@chromium.org, Nov 29

Labels: -merge-approved-71 merge-merged-3578
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9190108597ebc9aa4f51052cc62ef6ce95b43703

commit 9190108597ebc9aa4f51052cc62ef6ce95b43703
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Thu Nov 29 13:49:35 2018

Fix variations locale logic broken in M71.

There were two issues:

1. The new language::GetApplicationLocale() function that was
introduced as part of M71 changes to variations service did not
match the previous behavior - it had an early return when the
locale pref didn't exist - which would be hit when the user didn't
explicitly set their language in Chrome. But that's not sufficient
since l10n_util::GetApplicationLocale() actually does logic beyond
what's provided to it via the pref. On Mac, it gets the language
from the system. The changes to components/language were sufficient
to fix things on Mac.

2. On non-Mac platforms, the call to l10n_util::GetApplicationLocale()
also had a dependency on ResourceBundle being initialized, so this part
did not work correctly either. IsLocaleAvailable() was returning false
when there was no ResourceBundle. There was also a TODO to make the
LocaleDataPakExists() API static - which then could be used without
ResourceBundle being initialized. This was mostly straight-forward
except for use of delegate_->GetPathForLocalePack(). This delegate
API was only overridden (outside of tests) by CastResourceDelegate.
Given cast doesn't use the code path in question (variations), the
code is changed to only use that delegate if the resource bundle has
been initialized.

Also adds a check that the locale determined after resource bundle
has been loaded matches what was used to initialize variations.

TBR=asvitkine@chromium.org

(cherry picked from commit 8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1)

Bug:  908114 
Change-Id: Ief6bf3e370bf2773187387c1fd12fc4517b31a69
Reviewed-on: https://chromium-review.googlesource.com/c/1349770
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#610954}
Reviewed-on: https://chromium-review.googlesource.com/c/1352957
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#848}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/chrome/browser/metrics/chrome_feature_list_creator.cc
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/chrome/browser/metrics/chrome_feature_list_creator.h
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/components/language/core/browser/locale_util.cc
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/components/variations/service/variations_field_trial_creator.h
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/components/variations/service/variations_service.cc
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/components/variations/service/variations_service.h
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/l10n/l10n_util.cc
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/resource/resource_bundle.cc
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/resource/resource_bundle.h
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/resource/resource_bundle_android.cc
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/resource/resource_bundle_ios.mm
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/resource/resource_bundle_mac.mm
[modify] https://crrev.com/9190108597ebc9aa4f51052cc62ef6ce95b43703/ui/base/resource/resource_bundle_unittest.cc

Labels: Merge-Merged-71-3578
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/9190108597ebc9aa4f51052cc62ef6ce95b43703

Commit: 9190108597ebc9aa4f51052cc62ef6ce95b43703
Author: asvitkine@chromium.org
Commiter: asvitkine@chromium.org
Date: 2018-11-29 13:49:35 +0000 UTC

Fix variations locale logic broken in M71.

There were two issues:

1. The new language::GetApplicationLocale() function that was
introduced as part of M71 changes to variations service did not
match the previous behavior - it had an early return when the
locale pref didn't exist - which would be hit when the user didn't
explicitly set their language in Chrome. But that's not sufficient
since l10n_util::GetApplicationLocale() actually does logic beyond
what's provided to it via the pref. On Mac, it gets the language
from the system. The changes to components/language were sufficient
to fix things on Mac.

2. On non-Mac platforms, the call to l10n_util::GetApplicationLocale()
also had a dependency on ResourceBundle being initialized, so this part
did not work correctly either. IsLocaleAvailable() was returning false
when there was no ResourceBundle. There was also a TODO to make the
LocaleDataPakExists() API static - which then could be used without
ResourceBundle being initialized. This was mostly straight-forward
except for use of delegate_->GetPathForLocalePack(). This delegate
API was only overridden (outside of tests) by CastResourceDelegate.
Given cast doesn't use the code path in question (variations), the
code is changed to only use that delegate if the resource bundle has
been initialized.

Also adds a check that the locale determined after resource bundle
has been loaded matches what was used to initialize variations.

TBR=asvitkine@chromium.org

(cherry picked from commit 8cf3cc0c7ff0ca6483cee573716f1cd53970bfd1)

Bug:  908114 
Change-Id: Ief6bf3e370bf2773187387c1fd12fc4517b31a69
Reviewed-on: https://chromium-review.googlesource.com/c/1349770
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Reviewed-by: anthonyvd <anthonyvd@chromium.org>
Reviewed-by: Xi Han <hanxi@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#610954}
Reviewed-on: https://chromium-review.googlesource.com/c/1352957
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#848}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Project Member

Comment 32 by bugdroid1@chromium.org, Nov 29

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7059a3fa21aed3376471de76abddf8833fe0b575

commit 7059a3fa21aed3376471de76abddf8833fe0b575
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Thu Nov 29 13:55:50 2018

Fix Windows first run crash related to locale.

The crash would happen if you install Chrome of a specific language
and run it for the first time on a Windows OS of a different
language.

It was caused by code setting the language pref in-between the two
places where it was queried - such that the first time it was
queried resulted in a different value than the second.

This triggered the CHECK that was introduced by this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1349770

Which correctly identified the problem. This CL fixes the issue
by moving the importing of the first-run locale setting to
earlier.

TBR=asvitkine@chromium.org

(cherry picked from commit 5814800fa70a7a7a7975c9943cdf0c310873321c)

Bug: 908791,  908114 
Change-Id: Ie2e0178f298f78d38e0bc7f0b58c2205bb617561
Reviewed-on: https://chromium-review.googlesource.com/c/1352482
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#611480}
Reviewed-on: https://chromium-review.googlesource.com/c/1354270
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#849}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
[modify] https://crrev.com/7059a3fa21aed3376471de76abddf8833fe0b575/chrome/browser/chrome_resource_bundle_helper.cc
[modify] https://crrev.com/7059a3fa21aed3376471de76abddf8833fe0b575/chrome/browser/metrics/chrome_feature_list_creator.cc

The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/7059a3fa21aed3376471de76abddf8833fe0b575

Commit: 7059a3fa21aed3376471de76abddf8833fe0b575
Author: asvitkine@chromium.org
Commiter: asvitkine@chromium.org
Date: 2018-11-29 13:55:50 +0000 UTC

Fix Windows first run crash related to locale.

The crash would happen if you install Chrome of a specific language
and run it for the first time on a Windows OS of a different
language.

It was caused by code setting the language pref in-between the two
places where it was queried - such that the first time it was
queried resulted in a different value than the second.

This triggered the CHECK that was introduced by this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/1349770

Which correctly identified the problem. This CL fixes the issue
by moving the importing of the first-run locale setting to
earlier.

TBR=asvitkine@chromium.org

(cherry picked from commit 5814800fa70a7a7a7975c9943cdf0c310873321c)

Bug: 908791,  908114 
Change-Id: Ie2e0178f298f78d38e0bc7f0b58c2205bb617561
Reviewed-on: https://chromium-review.googlesource.com/c/1352482
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#611480}
Reviewed-on: https://chromium-review.googlesource.com/c/1354270
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/branch-heads/3578@{#849}
Cr-Branched-From: 4226ddf99103e493d7afb23a4c7902ee496108b6-refs/heads/master@{#599034}
Status: Fixed (was: Started)
Marking as fixed per merges.
Thank you asvitkine@ for working late last night on merges. 

Side Note: Re #28, we (asvitkine@ & I) decided to take additional two merges in as that was the safest approach, otherwise we needed to revert total 8 changes from M71 which is more risky.

Sign in to add a comment