Bug 200921 - Variadic StringBuilder::append does not handle upconverting from 8-bit to 16-bit correctly
Summary: Variadic StringBuilder::append does not handle upconverting from 8-bit to 16-...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P1 Normal
Assignee: Darin Adler
URL:
Keywords: InRadar
Depends on:
Blocks: 200862
  Show dependency treegraph
 
Reported: 2019-08-20 08:38 PDT by Darin Adler
Modified: 2019-08-23 11:00 PDT (History)
9 users (show)

See Also:


Attachments
Patch (25.55 KB, patch)
2019-08-20 08:45 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2019-08-20 08:38:20 PDT
Variadic StringBuilder::append does not handle upconverting from 8-bit to 16-bit correctly
Comment 1 Darin Adler 2019-08-20 08:45:49 PDT
Created attachment 376771 [details]
Patch
Comment 2 Saam Barati 2019-08-20 09:11:11 PDT
Comment on attachment 376771 [details]
Patch

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

> Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:170
> +        builder.append(String("A"), "B", bullseye, "");

Do we want to expect !is8Bit() after here? (Same with other tests.)
Comment 3 Saam Barati 2019-08-20 09:19:14 PDT
Comment on attachment 376771 [details]
Patch

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

r=me with a couple comments

> Source/WTF/wtf/text/StringBuilder.cpp:275
> +UChar* StringBuilder::appendUninitialized16(CheckedInt32 requiredLength)

I’m not a fan of these functions names since they don’t actually append.

What does the “uninitialized” refer to here?

Maybe we should have a prefix like “bufferFor...”?
Comment 4 Darin Adler 2019-08-20 09:26:11 PDT
Comment on attachment 376771 [details]
Patch

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

Thanks for the review and the comments. I’m going to land as-is but might follow up later on the comments.

>> Source/WTF/wtf/text/StringBuilder.cpp:275
>> +UChar* StringBuilder::appendUninitialized16(CheckedInt32 requiredLength)
> 
> I’m not a fan of these functions names since they don’t actually append.
> 
> What does the “uninitialized” refer to here?
> 
> Maybe we should have a prefix like “bufferFor...”?

The concept of this family of 5 private functions is that they append runs of "uninitialized characters", multiple LChar/UChar that need to be written to. The caller gets a pointer to the newly appended characters and must initialize them. The analogy is to the String::createUninitialized and StringImpl::createUninitialized functions. I am open to renaming the family of functions; note that this patch only touches 2 of the 5.

I don’t think these should be named "append buffer for uninitialized", but I do think we could come up with a name that’s an improvement.

>> Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:170
>> +        builder.append(String("A"), "B", bullseye, "");
> 
> Do we want to expect !is8Bit() after here? (Same with other tests.)

I don’t think that is all that helpful. The final check below will check that the string has the correct contents, and that not be possible if it was still 8-bit. The checks that I did add are necessary to make sure that the test is actually testing what it purports to test.
Comment 5 Darin Adler 2019-08-20 09:58:24 PDT
May be a good name is “extend buffer for appending“?
Comment 6 WebKit Commit Bot 2019-08-20 10:14:43 PDT
Comment on attachment 376771 [details]
Patch

Clearing flags on attachment: 376771

Committed r248903: <https://trac.webkit.org/changeset/248903>
Comment 7 WebKit Commit Bot 2019-08-20 10:14:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-08-20 10:15:20 PDT
<rdar://problem/54516677>
Comment 9 Darin Adler 2019-08-20 10:26:36 PDT
Oops looks like that JSC failure is real and this needs to be rolled out. Sorry!
Comment 10 Darin Adler 2019-08-20 10:31:24 PDT
Unless someone (Sam, Saam) knows how to fix it and we can roll forward instead. Sadly I can’t work on it during the day today.
Comment 11 Saam Barati 2019-08-20 12:23:24 PDT
(In reply to Darin Adler from comment #10)
> Unless someone (Sam, Saam) knows how to fix it and we can roll forward
> instead. Sadly I can’t work on it during the day today.

I can take a look. Going to kick off a build now.
Comment 12 Saam Barati 2019-08-20 12:32:42 PDT
Comment on attachment 376771 [details]
Patch

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

> Source/WTF/wtf/text/StringBuilder.cpp:-304
> -    if (!length || hasOverflowed())

maybe this was not ok to remove?
Comment 13 Saam Barati 2019-08-20 12:38:06 PDT
Comment on attachment 376771 [details]
Patch

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

>>> Tools/TestWebKitAPI/Tests/WTF/StringBuilder.cpp:170
>>> +        builder.append(String("A"), "B", bullseye, "");
>> 
>> Do we want to expect !is8Bit() after here? (Same with other tests.)
> 
> I don’t think that is all that helpful. The final check below will check that the string has the correct contents, and that not be possible if it was still 8-bit. The checks that I did add are necessary to make sure that the test is actually testing what it purports to test.

I don't think it's critical, but I think it's nice when reading testing code like this that all the expectations are explicitly checked, even if they may be redundant. I like to read testing code which asserts that all the invariants are held explicitly.
Comment 14 Saam Barati 2019-08-20 12:42:19 PDT
Comment on attachment 376771 [details]
Patch

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

>> Source/WTF/wtf/text/StringBuilder.cpp:-304
>> -    if (!length || hasOverflowed())
> 
> maybe this was not ok to remove?

Yeah, these two "hasOverflowed()" checks were not valid to remove. I haven't dug into why, but I'll revert their removal.
Comment 15 Saam Barati 2019-08-20 12:46:04 PDT
landed JSC tests fix in:
https://trac.webkit.org/changeset/248913/webkit
Comment 16 Saam Barati 2019-08-20 12:46:16 PDT
(In reply to Darin Adler from comment #5)
> May be a good name is “extend buffer for appending“?

I like this name more.
Comment 17 Darin Adler 2019-08-21 09:33:51 PDT
Thanks for your help, Saam. I'll follow up on the naming changes.
Comment 18 Darin Adler 2019-08-23 11:00:19 PDT
(In reply to Saam Barati from comment #13)
> I don't think it's critical, but I think it's nice when reading testing code
> like this that all the expectations are explicitly checked, even if they may
> be redundant. I like to read testing code which asserts that all the
> invariants are held explicitly.

I’ll consider that, but I’m not convinced it would make the tests better. It would make them brittle to changes that are observable that are not important, and there are many invariants we could assert on the string builder before and after each operation.