Bug 215051 - Unconditionally record string offsets in the fast text codepath
Summary: Unconditionally record string offsets in the fast text codepath
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 206208 214769 215059 215302
  Show dependency tree / graph
 
Reported: 2020-08-01 00:08 PDT by Myles C. Maxfield
Modified: 2020-08-09 00:06 PDT (History)
7 users (show)

See Also:

Attachments
Patch (17.77 KB, patch)
2020-08-01 00:16 PDT, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (18.25 KB, patch)
2020-08-01 00:36 PDT, Myles C. Maxfield 		no flags Details | Formatted Diff | Diff
Patch (18.75 KB, patch)
2020-08-01 00:48 PDT, Myles C. Maxfield 		darin: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>