Bug 34150 – WebKit needs a mechanism to catch stale HashMap entries

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

Created attachment 47379 [details]
work in progress

Having e.g. HashMap<AtomicStringImpl*, ValueType>, it is extremely difficult to catch stale AtomicStringImpl pointers in map keys. In most cases, they can just sit there making the map larger, and only cause issues if another AtomicStringImpl gets allocated at exactly the same address.

Even running with CHECK_HASHTABLE_CONSISTENCY won't catch this, since pointer's hash is just its value, so the table remains perfectly consistent. And we (I?) don't usually enable CHECK_HASHTABLE_CONSISTENCY, even when hunting down mystery crashers.

Making things more complicated, we often allow stale pointers in such HashMaps to exist for a while (a cache only needs to be reset when accessed, not every time something invalidates it).

Attached is a work in progress patch that I think is a reasonable solution. I need to add checkConsistency() calls to more suspect places, check how it affects test execution time, and maybe add more specialized checks besides the "memory is allocated" one. All kinds of comments are welcome.

[Geoffrey Garen]

+        static void checkValue(const P* p)

Might want to name this "checkValueConsistency" for, well, consistency :).

+            ASSERT(malloc_size(p) >= sizeof(P));

Would this have caught the bug that inspired you to write the patch. I don't think it would. As long as the block is recycled for an object of similar size (which is malloc's custom), this ASSERT won't fire.

[Alexey Proskuryakov]

>Might want to name this "checkValueConsistency" for, well, consistency :).

Indeed!

> Would this have caught the bug that inspired you to write the patch. I don't
> think it would.

The assert does fire on thedorchester.com. For each run when an AtomicStringImpl gets allocated at the same address, there are many runs when something else (or nothing at all) does.

I'm actually considering making this even weaker: ASSERT(malloc_size(p)). Saves the need to include AtomicStringImpl.h where we didn't need to include it before. That also catches the bug at thedorchester.com.

[Alexey Proskuryakov]

Created attachment 47471 [details]
proposed patch

[Alexey Proskuryakov]

An example of how a custom check can be done:

// AtomicStringImpl itself never goes into HashTable; this just adds a check for AtomicStringImpl* keys.
template<> struct HashTraits<WebCore::AtomicStringImpl> {
    static void checkValueConsistency(const WebCore::AtomicStringImpl* impl)
    {
        ASSERT(impl->existingHash() == WebCore::AtomicStringImpl::computeHash(impl->characters(), impl->length()));
    }
};

[Alexey Proskuryakov]

Committed revision 53899.

[Alexey Proskuryakov]

Reverted in r53912. SVG animation tests crash in release mode (and the same tests crash in debug mode in Safari, weird). Need to fix SMIL before I can land.

[Eric Seidel (no email)]

Thank you for all the follow-up and attempted fixerage.  hopefully the bots will go back go green now. :)

[Alexey Proskuryakov]

Created attachment 47562 [details]
updated patch

Reverted changes in ContainerNodeAlgorithms, clear m_elementsById in Document::removedLastRef() instead.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/css/CSSStyleSelector.cpp:364:  More than one command on the same line  [whitespace/newline] [4]
WebCore/css/CSSStyleSelector.cpp:365:  More than one command on the same line  [whitespace/newline] [4]
WebCore/css/CSSStyleSelector.cpp:366:  More than one command on the same line  [whitespace/newline] [4]
WebCore/html/HTMLSelectElement.h:79:  More than one command on the same line  [whitespace/newline] [4]
WebCore/html/CollectionCache.h:65:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 5


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

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/53957>.