Bug 73995 – StringBuilderTest.Append and StringBuilderTest.ToStringPreserveCapacity are failing.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Vsevolod Vlasov]

StringBuilderTest.Append StringBuilderTest.ToStringPreserveCapacity are failing on Chromium Linux since http://trac.webkit.org/changeset/102017.

[Vsevolod Vlasov]

First failure:
http://build.webkit.org/builders/Chromium%20Linux%20Release%20%28Tests%29/builds/26827

[Vsevolod Vlasov]

They are actually failing everywhere.
http://build.webkit.org/builders/Chromium%20Mac%20Release%20%28Tests%29/builds/3151
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29/builds/3432

[Michael Saboff]

*** Bug 73983 has been marked as a duplicate of this bug. ***

[Michael Saboff]

The problem is that when an 8 bit StringBuilder is accessed using the "characters()" method, a 16 bit copy is created. If more data is appended to the StringBuilder, the 16 bit shadow copy is not updated or invalidated.

[Michael Saboff]

Created attachment 118293 [details]
Patch with fix and test re-enabling

[Darin Adler]

Comment on attachment 118293 [details]
Patch with fix and test re-enabling

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

> Source/JavaScriptCore/wtf/text/StringBuilder.h:209
> +    mutable unsigned m_valid16BitShadowlen;

I’d spell out length and capitalize it:

    m_valid16BitShadowLength

> Source/JavaScriptCore/wtf/text/StringImpl.cpp:223
>      if (hasTerminatingNullCharacter()) {
> -        len--;
> -        m_copyData16[len] = '\0';
> +        end--;
> +        m_copyData16[end] = '\0';
>      }

Should this really go inside the helper function? Is this covered by test cases?

Also, why not just up convert the 8-bit terminating null to the 16-bit terminating null.

[Ryosuke Niwa]

Comment on attachment 118293 [details]
Patch with fix and test re-enabling

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

> Source/JavaScriptCore/ChangeLog:14
> +        Reviewed by NOBODY (OOPS!).

This line should appear above the change description.

[Ryosuke Niwa]

Oops, sorry. It appears that I've cleared Darin's r+.

[Michael Saboff]

Committed r102298: <http://trac.webkit.org/changeset/102298>