Bug 110171 – StringHasher functions require alignment that call sites do not all guarantee

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

[Darin Adler]

StringHasher functions require alignment that call sites do not all guarantee

[Darin Adler]

Created attachment 188973 [details]
Patch

[Darin Adler]

Comment on attachment 188973 [details]
Patch

Oops, looks like I uploaded something that doesn’t build.

[Early Warning System Bot]

Comment on attachment 188973 [details]
Patch

Attachment 188973 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16613441

[Darin Adler]

Created attachment 188974 [details]
Patch

[Benjamin Poulain]

Comment on attachment 188974 [details]
Patch

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

The new methods addCharactersAssumingAligned() are public. Wouldn't it be better to have them private until they are needed to be public by client code? Exposing those low level functions increases the risk of misuses like the one you fix with this patch.

If you keep the new methods public, it would be nice to have new test coverage for them.

> Source/WTF/wtf/StringHasher.h:77
> -            addCharactersToHash(m_pendingCharacter, Converter(*data++));
> -            m_hasPendingCharacter = false;
> -            --length;
> +            addCharactersInternal(m_pendingCharacter, a);
> +            m_pendingCharacter = b;
> +            return;

It would be nice to get rid of addCharactersInternal to simplify the class.
Is there really any extra cost to?:
    m_hasPendingCharacter = false;
    addCharactersInternal(m_pendingCharacter, a);
    m_pendingCharacter = b;
    m_hasPendingCharacter = true;

[Darin Adler]

(In reply to comment #5)
> The new methods addCharactersAssumingAligned() are public. Wouldn't it be better to have them private until they are needed to be public by client code?

I suppose they could be private; my intention is that they are part of the public interface.

> Exposing those low level functions increases the risk of misuses like the one you fix with this patch.

Sure, but given their names it seems unlikely they’d be misused. The issue before was the hidden precondition that was not part of the name. I don’t think Geoff would have gotten it wrong if it was part of the name.

> If you keep the new methods public, it would be nice to have new test coverage for them.

That’s a good point. Whether public or not it would be good to have tests for the functions in this class. I don’t know exactly how to do unit tests; I’m sure it’s easy but I don’t know where to put them.

> It would be nice to get rid of addCharactersInternal to simplify the class.

Will do.

[Darin Adler]

Created attachment 191122 [details]
Patch

[Darin Adler]

The patch was actually already reviewed by Ben Poulain. This new version makes most of the changes he suggested, except for the “make more private” one, which we could still do. And I added lots of tests.

[Benjamin Poulain]

Comment on attachment 191122 [details]
Patch

Thanks for doing all the tests!

My only comment is it would be useful to have a comments for each group of test.
E.g.:
    // addCharactersAssumingAligned with zero characters should produce an emptyStringHash.
    // addCharactersAssumingAligned with \0.
    // addCharactersAssumingAligned with 5 characters at once.
etc.

Otherwise it takes some reading to find what is tested in each paragraph.

[Darin Adler]

Committed r144552: <http://trac.webkit.org/changeset/144552>

[Thiago Marcos P. Santos]

(In reply to comment #10)
> Committed r144552: <http://trac.webkit.org/changeset/144552>

This test is failing on the EFL Debug bots from the moment it was introduced (although it is passing on the Release bots):

/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:173: Failure
Value of: hasher.hash()
  Actual: 2754808611
Expected: testBHash5
Which is: 2032145993
[  FAILED  ] WTF.StringHasher_addCharacters (0 ms)
[ RUN      ] WTF.StringHasher_addCharactersAssumingAligned
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Tools/TestWebKitAPI/Tests/WTF/StringHasher.cpp:328: Failure
Value of: hasher.hash()
  Actual: 2754808611
Expected: testBHash5
Which is: 2032145993
[  FAILED  ] WTF.StringHasher_addCharactersAssumingAligned (0 ms


http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/10028/steps/API%20tests/logs/stdio

[Darin Adler]

I see the problem. I forgot to null-terminate testBUChars!

[Darin Adler]

Geoff Garen fixed the failing tests problem in r145412.

[Darin Adler]

http://trac.webkit.org/projects/webkit/changeset/145412