Bug 73995 - StringBuilderTest.Append and StringBuilderTest.ToStringPreserveCapacity are failing.
Summary: StringBuilderTest.Append and StringBuilderTest.ToStringPreserveCapacity are f...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
: 73983 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-12-07 04:26 PST by Vsevolod Vlasov
Modified: 2011-12-07 18:20 PST (History)
6 users (show)

See Also:


Attachments
Patch with fix and test re-enabling (7.10 KB, patch)
2011-12-07 17:42 PST, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vsevolod Vlasov 2011-12-07 04:26:54 PST
StringBuilderTest.Append StringBuilderTest.ToStringPreserveCapacity are failing on Chromium Linux since http://trac.webkit.org/changeset/102017.
Comment 3 Michael Saboff 2011-12-07 10:19:47 PST
*** Bug 73983 has been marked as a duplicate of this bug. ***
Comment 4 Michael Saboff 2011-12-07 10:27:56 PST
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.
Comment 5 Michael Saboff 2011-12-07 17:42:09 PST
Created attachment 118293 [details]
Patch with fix and test re-enabling
Comment 6 Darin Adler 2011-12-07 17:45:23 PST
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.
Comment 7 Ryosuke Niwa 2011-12-07 17:45:49 PST
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.
Comment 8 Ryosuke Niwa 2011-12-07 17:51:29 PST
Oops, sorry. It appears that I've cleared Darin's r+.
Comment 9 Michael Saboff 2011-12-07 18:20:15 PST
Committed r102298: <http://trac.webkit.org/changeset/102298>