StringBuilder is fallible

• Component: Web Template Framework
• Status: RESOLVED WONTFIX

[JF Bastien]

StringBuilder can fail when allocating memory. StringBuilder is mostly used with user input, which often means that just crashing isn't the right outcome. Our fuzzers often encounter such things for JavaScriptCore, and so far we've applied point solutions. No more!

[JF Bastien]

Created attachment 338914 [details]
patch

From the WTF ChangeLog:

StringBuilder is fallible

StringBuilder can fail when allocating memory. StringBuilder is mostly used with user input, which often means that just crashing isn't the right outcome. Our fuzzers often encounter such things for JavaScriptCore, and so far we've applied point solutions. No more! This patch makes StringBuilder fallible and enforces this in two ways:

  * Each step of building a string can be checked for failure because they now return a "Fallible<T>". This can be ignored at every step!
  * The final step of StringBuilder, getting the string out, returns a "Fallible<String>" which is marked with WARN_UNUSED_RETURN. Failure is communicated if any of the building steps failed.

Doing this two-step approach means that code can continue operating as it did before (append a bunch and then get a string) and only needs to care about the string potentially not being available. This is enforced by the type system: Fallible<T> is std::expected<T, StringBuilder::Failure>.

The following intermediate functions can now be observed to fail (or ignored):

  * append
  * appendUninitialized
  * appendUninitializedSlow
  * appendQuotedJSONString used to return false on failure, it now returns Fallible<>
  * resize
  * allocateBuffer
  * allocateBufferUpConvert
  * reallocateBuffer
  * reserveCapacity
  * shrinkToFit
  * operator[]
  * characters8
  * characters16

Failure in these methods comes in two forms: out of memory, and would overflow. When an operation would overflow it leaves the StringBuilder in its previous state, allowing users to resume work, keep the string as it existed before failure. When we just fail allocating it means we expected to succeed, so clearing the StringBuilder is the right thing to do.

Supporting failure in StringBuilder required adding "try" variants of some methods in StringImpl.

This patch updates all WebKit code which uses StringBuilder. It leaves all intermediate function usage alone (code still appends back-to-back, blissfully unaware of failure), and only checks for failure when retrieving the string. The pattern used is usually:

  if (auto string = builder.toString())
    foo(*string); // Use string as before...
  else
    CRASH();

This makes failure extremely explicit. There's no true behavior change: what would have crashed when appending now crashes when retrieving the string. Pulling CRASH() out will give slightly better crash signatures, and it makes the code very obvious about failure. I purposefully don't "fix" any failure because these should be done in a targeted manner with repro tests whenever possible. I'm very wary of introducing bugs!

When someone inevitably blames code back to this commit (because hey the code is pretty ugly, and you might be surprised at seeing CRASH() in your beautiful code) there are a few things which they can do:

  1. Change the else clause to handle failure rather than crash. In JavaScriptCore that's usually throwOutOfMemoryError.
  2. Change the code to directly use Fallible's .value() member, and let *that* crash instead. This makes the crash non-obvious in the code, which might be what you want.
  3. Change the code to detect failure at the earliest step of string building (likely the append calls), and do something sensible there.

In no circumstance should code just dereference a Fallible without knowing that its state holds a valid string. This code is undefined behavior according to the std::expected specification (whether our implementation should do something else is out of scope for this patch):

  foo(*builder.toString()); // UB when allocation fails.

[EWS Watchlist]

Attachment 338914 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:70:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:78:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:92:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:99:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:126:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:129:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:141:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:149:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:154:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:167:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/API/tests/PingPongStackOverflowTest.cpp:69:  Use nullptr instead of NULL.  [readability/null] [5]
ERROR: Source/WTF/wtf/DateMath.cpp:1190:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:364:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:395:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/dfg/DFGStrengthReductionPhase.cpp:892:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/inspector/agents/InspectorApplicationCacheAgent.cpp:175:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:18:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:32:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:33:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:34:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:35:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:36:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:37:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:38:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:39:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:40:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:41:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:42:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:43:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:44:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WTF/wtf/text/StringBuilderJSON.cpp:123:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/testRegExp.cpp:332:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WebCore/html/HTMLAnchorElement.cpp:404:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/editing/MarkupAccumulator.cpp:495:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebCore/html/DOMTokenList.cpp:282:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WebCore/page/CaptionUserPreferencesMediaAF.cpp:704:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:83:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:102:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:112:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:132:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:150:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:173:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:197:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:217:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:248:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:256:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:313:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:352:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:358:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:382:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:392:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:391:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:406:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:469:  Missing space inside { }.  [whitespace/braces] [5]
ERROR: Source/WebCore/html/track/WebVTTTokenizer.cpp:217:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/html/track/WebVTTTokenizer.cpp:228:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/html/track/WebVTTTokenizer.cpp:229:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/platform/mac/PasteboardMac.mm:291:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/WebCore/xml/XMLErrors.cpp:152:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/WebCore/html/track/BufferedLineReader.cpp:95:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 69 in 287 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 338937 [details]
patch

Fix build failures on non-Mac bots, and some style things.

[EWS Watchlist]

Attachment 338937 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 10 in 290 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

I'm going to ignore the remaining style nits:

> ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using
> unsigned  [runtime/unsigned] [1]
> ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using
> unsigned  [runtime/unsigned] [1]

Pre-existing code. I don't really see why we'd change it.

> ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be
> removed when the prior "if" concludes with a return, break, continue or goto
> statement.  [readability/control_flow] [4]
> ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be
> removed when the prior "if" concludes with a return, break, continue or goto
> statement.  [readability/control_flow] [4]
> ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be
> removed when the prior "if" concludes with a return, break, continue or goto
> statement.  [readability/control_flow] [4]

Not in these cases.

> ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of
> security-sensitive phrasing could help someone exploit WebKit: fuzzer 
> [changelog/unwantedsecurityterms] [3]

Considered and ignored.

> ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More
> than one command on the same line  [whitespace/newline] [4]

Pre-existing code, and it's a lambda.

[JF Bastien]

Created attachment 339004 [details]
patch

Fix a few more non-Mac build issues.

[EWS Watchlist]

Attachment 339004 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 295 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339010 [details]
patch

More build fixes.

[EWS Watchlist]

Attachment 339010 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 297 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 339010 [details]
patch

Attachment 339010 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/7482931

New failing tests:
css3/filters/backdrop/add-remove-add-backdrop-filter.html

[EWS Watchlist]

Created attachment 339016 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[JF Bastien]

Created attachment 339022 [details]
patch

More build fixes... I wish the other platforms just continued compiling until they had more failures...

[EWS Watchlist]

Attachment 339022 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 300 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339026 [details]
patch

[EWS Watchlist]

Attachment 339026 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 302 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339151 [details]
patch

Fix a few more build failures on Linux and Windows.

[EWS Watchlist]

Attachment 339151 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 305 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339222 [details]
patch

Handle more failures, and rebase.

[EWS Watchlist]

Attachment 339222 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 305 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339227 [details]
patch

More Linux fixes.

[EWS Watchlist]

Attachment 339227 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 307 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339231 [details]
patch

More Linux build.

[EWS Watchlist]

Attachment 339231 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 309 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 339235 [details]
patch

More Linux.

[EWS Watchlist]

Attachment 339235 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilder.h:193:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WTF/wtf/text/StringBuilder.h:206:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/graphics/texmap/TextureMapperShaderProgram.cpp:448:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WTF/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Tools/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
ERROR: Source/JavaScriptCore/API/tests/ExecutionTimeLimitTest.cpp:480:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:110:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:390:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WTF/wtf/text/StringBuilder.cpp:405:  Omit int when using unsigned  [runtime/unsigned] [1]
ERROR: Source/WebCore/ChangeLog:10:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 11 in 312 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sam Weinig]

I am not a huge fan of littering the CRASH() macro all over, and requiring every user of toString() to remember to add it if they can't handle the failure case. As an alternative, what do you think about this?

Add two toString() overloads (note, code below is totally untested).

1. Can't handle overflow case:
template<typename SuccessFunctor>
void StringBuilder::toString(const SuccessFunctor& successFunctor)
{
    if (auto string = internalToString())
        successFunctor(*string);
    else
        CRASH();
}

2. Can handle overflow:
template<typename SuccessFunctor, typename FailureFunctor>
void StringBuilder::toString(const SuccessFunctor& successFunctor, const FailureFunctor& failureFunctor)
{
    auto string = internalToString()
    if (string)
        successFunctor(*string);
    else
        failureFunctor(string.error());
}

Then, at call sites, users would do something like:

stringBuilder.toString([] (String result) {
    // use result.
});

or, in the case they could handle the overflow,

stringBuilder.toString([] (String result) {
    // use result.
}, [] (StringBuilder::Failure failureKind) {
    // handle failure.
});


What do you think?

[JF Bastien]

(In reply to Sam Weinig from comment #26)
> I am not a huge fan of littering the CRASH() macro all over, and requiring
> every user of toString() to remember to add it if they can't handle the
> failure case. As an alternative, what do you think about this?

Users have to extract the result from the std::expected. They don't have to remember to crash, because that might not be what they want. The only reason I structured the code this way is to keep it at "feature" parity (used to crash, still does), but surface the crash locations so that we can then progressively figure out what should be done for each location (and so that any crash reports sent our way are unambiguous about why the crash occurred).

> Add two toString() overloads (note, code below is totally untested).
> 
> 1. Can't handle overflow case:
> template<typename SuccessFunctor>
> void StringBuilder::toString(const SuccessFunctor& successFunctor)
> {
>     if (auto string = internalToString())
>         successFunctor(*string);
>     else
>         CRASH();
> }

With what I have now, that's the same as:

  StringBuilder builder;
  // ... build build build ...
  String string = builder.toString().value();

I think that's better, when you know you want to crash on failure. I don't want this change to outright do this though, because without vetting that crashing is the right thing for each of these locations I'd just be sweeping the crash under the rug. The goal should be, in most cases, to do something else than crash (and if crashing is right then use .value()).


> 2. Can handle overflow:
> template<typename SuccessFunctor, typename FailureFunctor>
> void StringBuilder::toString(const SuccessFunctor& successFunctor, const
> FailureFunctor& failureFunctor)
> {
>     auto string = internalToString()
>     if (string)
>         successFunctor(*string);
>     else
>         failureFunctor(string.error());
> }
> 
> Then, at call sites, users would do something like:
> 
> stringBuilder.toString([] (String result) {
>     // use result.
> });
> 
> or, in the case they could handle the overflow,
> 
> stringBuilder.toString([] (String result) {
>     // use result.
> }, [] (StringBuilder::Failure failureKind) {
>     // handle failure.
> });

It kind of seems like you want std::expected to offer a fully monadic interface, hehehe. It doesn't right now, but in the future we might see something like:

  https://vittorioromeo.info/index/blog/adts_over_exceptions.html



Anyhow, I like surfacing the crashes location with the current pattern. We could instead just sweep all of them under the rug by having this patch do .value() everywhere. I think it's a bad idea, but you seem to think otherwise?

[JF Bastien]

Doesn't look like this will happen. FWIW the C++ committee decided to go in this direction when it comes to bad_alloc (terminate instead of throw, add try_* variants of allocating STL functions).