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

Issue 727229 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Regression: ‘Force UI direction’,’Force text direction’ and ‘Enable RTL’ flags is not working.

Reported by abom...@etouch.net, May 29 2017

Issue description

Chrome Version: 61.0.3114.0 (Official Build) cf4f838225c132dee06cc21dd6a576347734589b-refs/heads/master@{#475229}
OS: Mac(10.12.3,10.11.6)

What steps will reproduce the problem?
1. Launch chrome and navigate to chrome://flags
2. Select ‘Right-to-left’ direction for Force UI direction flag and click on ‘Relaunch’ button, enter text in omnibox
3. Observe, Similarly for other two flags.

Actual: ‘Force UI direction’,’Force text direction’ and ‘Enable RTL’ flags is not working.(Refer screencast)
Expected: ‘Force UI direction’,’Force text direction’ and ‘Enable RTL’ flags should work.

This is regression issue, broken in ‘M 60’ and below is manual bisect info:
Good build:60.0.3103.0 (472588)
Bad build:60.0.3104.0(473014)

Note: Issue is not seen on Windows and Linux OS.

 
Actual_rtl.mov
6.4 MB Download
Exp_rtl.mov
4.1 MB Download
Cc: rbasuvula@chromium.org
Labels: hasbisect
Owner: eugene...@chromium.org
Status: Assigned (was: Unconfirmed)
Providing normal bisect due to builds are not invoking in per-revision bisect.Below are the bisect results,
Good build:60.0.3103.0 (Revision:472588).
Bad build:60.0.3104.0 (Revision:473014).

You are probably looking for a change made after 472887 (known good), but no later than 472896 (first known bad).

CHANGE-LOG URL:
---------------
https://chromium.googlesource.com/chromium/src/+log/1f6af8456c81693146656a175027e39b79ed2a52..0b1095670424340f092fb3a7d4436115d2a80e2f

From the CL above, assigning the issue to the concern owner

@eugenebut: Could you please look into the issue, pardon me if it has nothing to do with your changes and if possible please assign it to concern owner.

Review-Url: https://codereview.chromium.org/2884973005
Note :Mac specific issue and Able to reproduce in latest Canary #61.0.3114.0
Owner: rbasuvula@chromium.org
CL touches only ios/web/web_state/ui/crw_web_controller.mm which is not even compiled on mac

Comment 3 by shrike@chromium.org, May 30 2017

Cc: lgrey@chromium.org
Labels: ReleaseBlock-Stable
Owner: mbjorge@chromium.org
+lgrey@ (not sure if Mac RTL changed the behavior of this flag)

Possibly 89c24d4 [TSAN] Fix TSAN error in i18n RTL. by mbjorge · 12 days ago

Cc: k...@chromium.org

Comment 5 by lgrey@chromium.org, May 30 2017

Re #3, Mac RTL shouldn't affect this flag.

Comment 6 by k...@chromium.org, May 30 2017

The video doesn't show the use of --force-text-direction=rtl. Was it working in 60.0.3103.0 ?
Ah, yeah it could definitely been my CL, though it's not obvious to me why it would be macOS specific
My guess is that the g_icu_text_direction is never getting set. Probably the windows/linux paths are doing a call to 'SetICUDefaultLocale' at some point and the mac path is not? Just speculation though, I don't have a mac setup to test with. Will put up a revert 
Revert going through CQ: https://codereview.chromium.org/2917523002/
Waiting on a comitter+1 on the CL from #9
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 5 2017

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

commit 27175dace3576a3e351348cf8272c20b0d3aceec
Author: mbjorge <mbjorge@chromium.org>
Date: Mon Jun 05 20:12:55 2017

Revert of [TSAN] Fix TSAN error in i18n RTL. (patchset #3 id:40001 of https://codereview.chromium.org/2877983004/ )

Reason for revert:
Appears to break the Force UI Direction / Force Text Direction / FOrce RTL flags on mac.

BUG= 727229 

Original issue's description:
> [TSAN] Fix TSAN error in i18n RTL.
>
> Use an atomic to protect the global g_icu_text_direction cached value
> from multiple readers/writers.
>
> Also reduce acccess to g_icu_text_diretion so it only has a single write
> location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
> to simplify the race-y-ness.
>
> Outside of test code, SetICUDefaultLocal is only used in two locations:
> ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
> places outside of test code, so it seemed reasonable to move the heavy
> logic (updating g_icu_text_direction when it changes) into what I imagine to be
> a function that is called rarely and let the one that is used a lot
> (IsRTL/ICUIsRTL) just do a simple read.
>
> This still still leaves a race condition in that IsRTL may give in
> inaccurate result if SetICUDefaultLocale is executed at the same time.
> In particular, IsRTL may return the value for the previously set default
> locale after the default locale has been updated.
>
> BUG=695929
> TEST=cast_shell_browsertests under TSAN before/after
>
> Review-Url: https://codereview.chromium.org/2877983004
> Cr-Commit-Position: refs/heads/master@{#472890}
> Committed: https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e6d23dc776c36

TBR=jshin@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=695929

Review-Url: https://codereview.chromium.org/2917523002
Cr-Commit-Position: refs/heads/master@{#477064}

[modify] https://crrev.com/27175dace3576a3e351348cf8272c20b0d3aceec/base/i18n/rtl.cc

Not sure how to CP to the release branch though? Can anyone point me to some docs for that process?
Owner: rbasuvula@chromium.org
rbasuvula@ are you able to confirm that this is fixed with the revert? I do not have a mac setup to test with
Thanks for the revert. abombde@ Can you please verify in latest canary.
Labels: Merge-Request-60
Labels: -Merge-Request-60
ah, sorry, will add Merge-Request-60 label for the revert after it's verified
With respect to comment #14:

Above issue seems to be fixed on Latest Canary Version:61.0.3123.0 (Official Build)193094009c210267cfa2c194ec9875711ccaf647-refs/heads/master@{#477506} 64 bit using Mac Pro(10.12.3)
Labels: TE-Verified-M61 TE-Verified-61.0.3123.0
Owner: mbjorge@chromium.org
As per comment #18. Adding TE-Verified labels for M-61.

@mbjorge: Please merge in to M-60.

Thank You!
Labels: Merge-Request-60
Merge to M60 CL at: https://chromium-review.googlesource.com/c/527328/
Project Member

Comment 21 by sheriffbot@chromium.org, Jun 7 2017

Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Owner: bustamante@chromium.org
Labels: -Merge-Review-60 Merge-Approved-60
Fix has been verified in canary, and marked as rb-stable. Approving merge for M60. 
I have a CR+1 and Merge-Approved now, but I don't have permissions to `git cl land` or to even mark CQ+2 on the CL. Let me know if there is something else I need to do, otherwise I'll trust that the powers-that-be will be able to submit the M60 CL
I'm not sure what is the cl in c#20, but here are instructions you need to cherry-pick your revert in c#11 back to M60:

https://sites.google.com/a/chromium.org/dev/developers/how-tos/drover

Please merge the patch to M60 branch(3112),Beta RC cut is scheduled @ 4.00 PM PST tomorrow(06/13).
Project Member

Comment 27 by bugdroid1@chromium.org, Jun 14 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eeb12a2d4f56714eafd9e71dba80ca378ace13e9

commit eeb12a2d4f56714eafd9e71dba80ca378ace13e9
Author: Mike Bjorge <mbjorge@chromium.org>
Date: Wed Jun 14 20:44:20 2017

Revert of [TSAN] Fix TSAN error in i18n RTL. (patchset #3 id:40001 of https://codereview.chromium.org/2877983004/ )

Reason for revert:
Appears to break the Force UI Direction / Force Text Direction / FOrce RTL flags on mac.

BUG= 727229 

Original issue's description:
> [TSAN] Fix TSAN error in i18n RTL.
>
> Use an atomic to protect the global g_icu_text_direction cached value
> from multiple readers/writers.
>
> Also reduce acccess to g_icu_text_diretion so it only has a single write
> location (in SetICUDefaultLocal) and a single read location (ICUIsRTL),
> to simplify the race-y-ness.
>
> Outside of test code, SetICUDefaultLocal is only used in two locations:
> ppapi_plugin_main.cc and l10n_util.cc, whereas IsRTL is used in >100
> places outside of test code, so it seemed reasonable to move the heavy
> logic (updating g_icu_text_direction when it changes) into what I imagine to be
> a function that is called rarely and let the one that is used a lot
> (IsRTL/ICUIsRTL) just do a simple read.
>
> This still still leaves a race condition in that IsRTL may give in
> inaccurate result if SetICUDefaultLocale is executed at the same time.
> In particular, IsRTL may return the value for the previously set default
> locale after the default locale has been updated.
>
> BUG=695929
> TEST=cast_shell_browsertests under TSAN before/after
>
> Review-Url: https://codereview.chromium.org/2877983004
> Cr-Commit-Position: refs/heads/master@{#472890}
> Committed: https://chromium.googlesource.com/chromium/src/+/89c24d4a46c1f0e7ad03891a659e6d23dc776c36

BUG=695929

(cherry picked from commit 27175dace3576a3e351348cf8272c20b0d3aceec)

Review-Url: https://codereview.chromium.org/2917523002
Cr-Original-Commit-Position: refs/heads/master@{#477064}
Change-Id: I7e06fc64d20b673766f2bed3f084326e590024f1
Reviewed-on: https://chromium-review.googlesource.com/527328
Reviewed-by: Jungshik Shin <jshin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#345}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/eeb12a2d4f56714eafd9e71dba80ca378ace13e9/base/i18n/rtl.cc

Comment 28 by abom...@etouch.net, Jun 28 2017

Labels: TE-Verified-M60 TE-Verified-60.0.3112.50
Above issue seems to be fixed on Latest Beta Version:60.0.3112.50 (Official Build) 1b8bb7c2c2f4bd3a42ec5a63db827ad9930f2536-refs/branch-heads/3112@{#485} 64 bit using Mac Pro(10.12.3)
Status: Verified (was: Assigned)
Marking as verified per comment #28; bustamante@ please correct me if I'm wrong.

Sign in to add a comment