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.
+ 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.
>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.
Created attachment 47471 [details] proposed patch
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())); } };
Committed revision 53899.
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.
Thank you for all the follow-up and attempted fixerage. hopefully the bots will go back go green now. :)
Created attachment 47562 [details] updated patch Reverted changes in ContainerNodeAlgorithms, clear m_elementsById in Document::removedLastRef() instead.
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.
Committed <http://trac.webkit.org/changeset/53957>.