Finch locale logic is broken in M71 |
||||||||||||||||
Issue descriptionFinch 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.
,
Nov 23
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.
,
Nov 25
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.
,
Nov 26
Updating title. It's not just broken on Mac - affects all platforms.
,
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
,
Nov 27
Pls update this bug with canary result tomorrow.
,
Nov 27
Pls verify this bug on canary version 72.0.3623.0 tomorrow.
,
Nov 27
The NextAction date has arrived: 2018-11-27
,
Nov 27
CL from C#5 has introduced a top browser crash on Windows tracked in Issue 908791
,
Nov 27
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.
,
Nov 27
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.
,
Nov 27
Fix is in commit queue: https://chromium-review.googlesource.com/c/chromium/src/+/1352482
,
Nov 27
Thank you, pls update bug with canary result on Thursday morning and request a merge to M71 if change looks good in canary.
,
Nov 27
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?
,
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
,
Nov 28
,
Nov 28
"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.
,
Nov 28
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.
,
Nov 28
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
,
Nov 28
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.
,
Nov 28
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
,
Nov 28
Triggering new canary from branch 3624 for Android, Desktop and iOS with merge listed at #21.
,
Nov 28
Canary version 72.0.3624.2 currently building with this merge in.
,
Nov 28
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!
,
Nov 29
Crashes are fixed in 72.0.3624.2 canary. Pinging for merge to M71. (Merge label already set.)
,
Nov 29
Merge approved to 71, branch 3578.
,
Nov 29
Pls mark this bug as fixed after the merge if nothing else is pending. Thank you.
,
Nov 29
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.
,
Nov 29
The NextAction date has arrived: 2018-11-29
,
Nov 29
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
,
Nov 29
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}
,
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
,
Nov 29
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}
,
Nov 29
Marking as fixed per merges.
,
Nov 29
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 |
||||||||||||||||
Comment 1 by asvitk...@chromium.org
, Nov 23