Bug 215051

Summary: Unconditionally record string offsets in the fast text codepath
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, dino, jonlee, simon.fraser, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 206208, 214769, 215059, 215302    
Attachments:
Description Flags
Patch
none
Patch
none
Patch darin: review+

Description Myles C. Maxfield 2020-08-01 00:08:26 PDT 
Unconditionally record string offsets in the fast text codepath
Comment 1 Myles C. Maxfield 2020-08-01 00:16:52 PDT 
Created attachment 405770 [details]
Patch
Comment 2 Myles C. Maxfield 2020-08-01 00:36:44 PDT 
Created attachment 405773 [details]
Patch
Comment 3 Myles C. Maxfield 2020-08-01 00:48:24 PDT 
Created attachment 405776 [details]
Patch
Comment 4 Darin Adler 2020-08-01 21:03:09 PDT 
Comment on attachment 405776 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=405776&action=review

> Source/WebCore/platform/graphics/FontCascade.cpp:1266
> -        ASSERT_WITH_SECURITY_IMPLICATION(offsetInString < textRun.length());
> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(offsetInString < static_cast<GlyphBufferStringOffset>(textRun.length()));

Changing this to a RELEASE_ASSERT makes it unnecessary to have it inside an if statement that checks the same thing. Just put this outside the if and don’t check this in the if.

> Source/WebCore/platform/graphics/FontCascade.cpp:1273
> +        U16_NEXT(textRun.characters16(), offsetInString, static_cast<GlyphBufferStringOffset>(textRun.length()), baseCharacter);

Can we put static_cast<GlyphBufferStringOffset>(textRun.length()) in a local variable? The long expression makes it hard to see what is going on. And we could probably convert without a cast if we used a local. And we use it twice, once here and once in the assertion.

Also, seems like this should be U16_GET, not U16_NEXT. We don’t want to bump offsetInString. And if we did, we’d have to do it in the 8-bit case above.
Comment 5 Myles C. Maxfield 2020-08-03 21:50:04 PDT 
Committed r265241: <https://trac.webkit.org/changeset/265241>
Comment 6 Radar WebKit Bug Importer 2020-08-03 21:51:19 PDT 
<rdar://problem/66507427>
Comment 7 Myles C. Maxfield 2020-08-08 23:40:08 PDT 
Committed r265414: <https://trac.webkit.org/changeset/265414>