Bug 34150 - WebKit needs a mechanism to catch stale HashMap entries
Summary: WebKit needs a mechanism to catch stale HashMap entries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-25 17:54 PST by Alexey Proskuryakov
Modified: 2010-01-27 15:42 PST (History)
5 users (show)

See Also:


Attachments
work in progress (10.37 KB, patch)
2010-01-25 17:54 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed patch (36.63 KB, patch)
2010-01-26 17:18 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
updated patch (31.34 KB, patch)
2010-01-27 13:43 PST, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 2010-01-25 17:54:38 PST
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.
Comment 1 Geoffrey Garen 2010-01-25 19:19:48 PST
+        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.
Comment 2 Alexey Proskuryakov 2010-01-25 20:10:07 PST
>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.
Comment 3 Alexey Proskuryakov 2010-01-26 17:18:15 PST
Created attachment 47471 [details]
proposed patch
Comment 4 Alexey Proskuryakov 2010-01-26 17:22:26 PST
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()));
    }
};
Comment 5 Alexey Proskuryakov 2010-01-26 21:58:02 PST
Committed revision 53899.
Comment 6 Alexey Proskuryakov 2010-01-27 01:08:37 PST
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.
Comment 7 Eric Seidel (no email) 2010-01-27 01:09:48 PST
Thank you for all the follow-up and attempted fixerage.  hopefully the bots will go back go green now. :)
Comment 8 Alexey Proskuryakov 2010-01-27 13:43:06 PST
Created attachment 47562 [details]
updated patch

Reverted changes in ContainerNodeAlgorithms, clear m_elementsById in Document::removedLastRef() instead.
Comment 9 WebKit Review Bot 2010-01-27 13:48:31 PST
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.
Comment 10 Alexey Proskuryakov 2010-01-27 15:42:18 PST
Committed <http://trac.webkit.org/changeset/53957>.