New issue
Advanced search Search tips

Issue 899876 link

Starred by 2 users
Status: Fixed
Owner:
e...@chromium.org
Closed: Nov 28
Cc:
e...@chromium.org
drott@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[LayoutNG] DCHECK for font-variant: small-caps and ß.

Project Member Reported by bunge...@chromium.org, Oct 29 
Chromium at 8824cfe1e3a4897d685e70e6f6225afa2d8fc633 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="font-variant: small-caps">ß</span>

The DCHECK is

[1:1:1029/153438.656146:FATAL:wtf_string.h(139)] Check failed: !impl_->Is8Bit().
#3 0x7fffd32cc361 WTF::String::Characters16()
#4 0x7fffd33d2c3a blink::CaseMappingHarfBuzzBufferFiller::CaseMappingHarfBuzzBufferFiller()
#5 0x7fffd33da143 blink::HarfBuzzShaper::ShapeSegment()
#6 0x7fffd33dac8e blink::HarfBuzzShaper::Shape()

It appears that CaseMappingHarfBuzzBufferFiller assumes 'text' is 16 bit but in this case it isn't, perhaps because it became 8 bit after small-caps was applied.

Comment 1 by e...@chromium.org, Nov 5

Cc: drott@chromium.org e...@chromium.org
Owner: ----
Thanks for the minimal repro, I'll take a look.

Comment 2 by e...@chromium.org, Nov 10

Owner: e...@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 3 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

Comment 4 by e...@chromium.org, Nov 10

Status: Fixed (was: Started)
Project Member

Comment 5 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

Comment 6 by bungeman@google.com, Nov 12

Status: Started (was: Fixed)
Project Member

Comment 7 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

Comment 8 by bunge...@chromium.org, Nov 28

Status: Fixed (was: Started)
►

Sign in to add a comment