New issue
Advanced search Search tips

Issue 899868 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 750990

Blocking:
issue 923446



Sign in to add a comment

[LayoutNG] DCHECK for text-transform: uppercase and ß.

Project Member Reported by bunge...@chromium.org, Oct 29

Issue description

Chromium at 9aaa9887153703218ffdc5e89532c189fd609f52 with layoutng on (does not reproduce with layoutng off).

Discovered at http://unifraktur.sourceforge.net/testcases/small-caps/ .

The minimized test case is

<span style="text-transform: uppercase">ß</span>

which DCHECKS like

[1:1:1029/150837.365747:FATAL:position.cc(137)] Check failed: static_cast<unsigned>(offset) <= ToCharacterData(anchor_node_)->length() (2 vs. 1)#text "\u00DF"
 
That was actually the wrong Chromium SHA, the correct SHA is 8824cfe1e3a4897d685e70e6f6225afa2d8fc633 .
Note that this may be related to  issue 852470 . Not sure if that needs to be fixed to fix this particular issue though.
Owner: e...@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 10

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

commit abcd4da5aa44fea262bf8a450bcb14e4dd910541
Author: Emil A Eklund <eae@chromium.org>
Date: Sat Nov 10 05:46:26 2018

[LayoutNG] Fix text-transform for 8-bit text

Fix a bug in CaseMappingHarfBuzzBufferFiller where 8-bit strings weren't
up-converted to 16-bit in cases were the original and case mapped string
differed in length which hit a CHECK in the String::Characters16 method.

Bug:  899876 , 899868
No-Try: true
Change-Id: I5de8fbaabbc08932904846ffa4674129263f50aa
Reviewed-on: https://chromium-review.googlesource.com/c/1330723
Commit-Queue: Emil A Eklund <eae@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607106}
[add] https://crrev.com/abcd4da5aa44fea262bf8a450bcb14e4dd910541/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash-expected.txt
[add] https://crrev.com/abcd4da5aa44fea262bf8a450bcb14e4dd910541/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash.html
[modify] https://crrev.com/abcd4da5aa44fea262bf8a450bcb14e4dd910541/third_party/blink/renderer/platform/fonts/shaping/case_mapping_harfbuzz_buffer_filler.cc

Status: Fixed (was: Started)
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 12

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

commit 8e40418fa31e14332698753e479e87c06369f64e
Author: Adam Rice <ricea@chromium.org>
Date: Mon Nov 12 06:32:05 2018

Revert "[LayoutNG] Fix text-transform for 8-bit text"

This reverts commit abcd4da5aa44fea262bf8a450bcb14e4dd910541.

Reason for revert: Causing failures on Linux debug bot: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20Tests%20%28dbg%29%281%29/75629

Original change's description:
> [LayoutNG] Fix text-transform for 8-bit text
> 
> Fix a bug in CaseMappingHarfBuzzBufferFiller where 8-bit strings weren't
> up-converted to 16-bit in cases were the original and case mapped string
> differed in length which hit a CHECK in the String::Characters16 method.
> 
> Bug:  899876 , 899868
> No-Try: true
> Change-Id: I5de8fbaabbc08932904846ffa4674129263f50aa
> Reviewed-on: https://chromium-review.googlesource.com/c/1330723
> Commit-Queue: Emil A Eklund <eae@chromium.org>
> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
> Reviewed-by: Stefan Zager <szager@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607106}

TBR=szager@chromium.org,cbiesinger@chromium.org,eae@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  899876 , 899868
Change-Id: I0dbcc4d3c64cfcc9f074b6255dca09885043eed7
Reviewed-on: https://chromium-review.googlesource.com/c/1331096
Reviewed-by: Adam Rice <ricea@chromium.org>
Commit-Queue: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607156}
[delete] https://crrev.com/d934948431f2afd0e40130c91f9ca4cb37bb6941/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash-expected.txt
[delete] https://crrev.com/d934948431f2afd0e40130c91f9ca4cb37bb6941/third_party/WebKit/LayoutTests/fast/text/8-bit-text-transform-crash.html
[modify] https://crrev.com/8e40418fa31e14332698753e479e87c06369f64e/third_party/blink/renderer/platform/fonts/shaping/case_mapping_harfbuzz_buffer_filler.cc

Status: Started (was: Fixed)
Unfortunately, layoutng is reporting that the minimal test case in the original report turns into 'S' when it should be 'SS'.

See https://chromium-review.googlesource.com/c/chromium/src/+/1331138 .
Well, it is 'SS' in the non-layoutng case. It is unclear if the output here should be the pre or post text-transform. It appears layoutng is correct(?) here in producing the actual text of the node, but the existing code seems to produce the text after the transform? Seems we need separate expectations in that case.
No wait... in that case (getting the original text and not the transformed text) one should get 'ß'. So the layoutng case is incorrect. It should produce either 'SS' or 'ß' depending on if the pre or post text-transform content is expected.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 28

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

commit ec8ad11b2e541a3e41a847e762b5a289937a3919
Author: Ben Wagner <bungeman@chromium.org>
Date: Wed Nov 28 16:11:56 2018

Reland "[LayoutNG] Fix text-transform for 8-bit text"

This is a reland of abcd4da5aa44fea262bf8a450bcb14e4dd910541

Original change's description:
> [LayoutNG] Fix text-transform for 8-bit text
>
> Fix a bug in CaseMappingHarfBuzzBufferFiller where 8-bit strings weren't
> up-converted to 16-bit in cases were the original and case mapped string
> differed in length which hit a CHECK in the String::Characters16 method.
>
> Bug:  899876 , 899868
> No-Try: true
> Change-Id: I5de8fbaabbc08932904846ffa4674129263f50aa
> Reviewed-on: https://chromium-review.googlesource.com/c/1330723
> Commit-Queue: Emil A Eklund <eae@chromium.org>
> Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
> Reviewed-by: Stefan Zager <szager@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607106}

Bug:  899876 , 899868
Change-Id: I6b35a8335ab77fc700040a37f23278b67ec4eda5
Reviewed-on: https://chromium-review.googlesource.com/c/1331138
Commit-Queue: Ben Wagner <bungeman@chromium.org>
Reviewed-by: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Emil A Eklund <eae@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611716}
[modify] https://crrev.com/ec8ad11b2e541a3e41a847e762b5a289937a3919/third_party/blink/renderer/platform/fonts/shaping/case_mapping_harfbuzz_buffer_filler.cc
[add] https://crrev.com/ec8ad11b2e541a3e41a847e762b5a289937a3919/third_party/blink/web_tests/fast/text/8-bit-text-transform-crash-expected.html
[add] https://crrev.com/ec8ad11b2e541a3e41a847e762b5a289937a3919/third_party/blink/web_tests/fast/text/8-bit-text-transform-crash.html

Status: Fixed (was: Started)
Status: Started (was: Fixed)
The small-caps variant is fixed, but this text-transform issue still exists. I'm not sure why fast/text/8-bit-text-transform-crash.html passed on linux_layout_tests_layout_ng , but it still dchecks  in my local build of Chromium at 1a85e52504e9b381e0c the same way as in the original report.
Interesting, I'll take a look.

Thanks!
Cc: e...@chromium.org
Labels: -Pri-3 Pri-2
Owner: xiaoche...@chromium.org
Status: Assigned (was: Started)
Hits an assert in LayoutText::PositionForCaretOffset (and crashes in PositionTemplate<>::PositionTemplate without the assert).

Xiaocheng, could you take a look please?

Blockedon: 750990
Cc: kojii@chromium.org
We currently don't have proper support for text-transform in NGOffsetMapping, causing out-of-range offsets.

kojii@:

As discussed before, we are not going to work around this issue before using ICU to handle text-transform directly in LayoutNG.

Do we still stick to this plan, or are we going to introduce some workarounds to unblock LayoutNG shipping?
I think handling text-transform in NG is still the plan.

Minor point but since this is a known deferred feature, I think it's better to fail more silently and gracefully if possible. By DCHECK failure, it's not easy to know if, for instance, we crash or show bunch of garbages on the next line when DCHECK is not enabled.

What about converting DCHECK to TODO for known cases such as when 'text-transform' is applied?
We either need to fall back on legacy in these cases or add support for it in NG, crashing on real world content isn't really an option.
Sorry for the delay.

I'm leaning towards working around it and turing DCHECKs into silent failures, and track these workarounds with issue 7500990.
Sounds like a good plan.

Comment 20 by xiaoche...@chromium.org, Jan 18 (4 days ago)

Blocking: 923446

Sign in to add a comment