AtomicStringHash does not work with null atomic string

[Benjamin Poulain]

The current implementation of AtomicStringHash crashes with null string.

The reason is the hashing, done like this: 
        static unsigned hash(const AtomicString& key)
        {
            return key.impl()->existingHash();
        }
In that code, key.impl() can be null.


This causes a crash with the following code (because of the null string family):

#include <QtGui>
#include <QtWebKit>

int main(int argc, char *argv[])
{
    QApplication app(argc, argv);
    QWebSettings *settings = QWebSettings::globalSettings();
    settings->setFontFamily(QWebSettings::StandardFont, QString());

    QWebView view;
    view.load(QUrl("http://www.pro-linux.de/news/1/15917/opensuse-113-veroeffentlicht.html"));
    view.show();
    return app.exec();
}

[Benjamin Poulain]

Created attachment 63208 [details]
Use the pointer to the implementation as the hash value of AtomicString.

We know the point is unique by the definition of AtomicString, which makes it a good candidate for a hash key.

This makes AtomicStringHash works with null key, which is safer in my opinion. :)

[Darin Adler]

Comment on attachment 63208 [details]
Use the pointer to the implementation as the hash value of AtomicString.

Making the hash work will null isn’t sufficient. Hash tables still need both an empty value and a deleted value. A null pointer is the empty value and so won't work in a hash table even with this hash function.

We used to hash the pointers and switched to using the hash of the string instead as a measurable speedup. The hash of the string is higher quality than a hash of the pointer bits.

What you have here is not even a hash of pointer bits, but rather just taking the low bits. This will be a far-inferior hash function and will almost certainly result in a measurable slowdown.

The best approach here is to check explicitly for null and not use it with hash tables. Or to do a more-major redesign.

[Benjamin Poulain]

(In reply to comment #2)
> The best approach here is to check explicitly for null and not use it with hash tables.

Fair enough.

I thought it would be safer to avoid a crash if somebody forget to check the input for null...