Bug 16512 – Valgrind: Invalid read of size 4

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Grace Kloba]

Loading www.cnn.com followed by yahoo.com, Valgrind reports the following. And if we run without Valgrind, we get crash eventually by running script to repeatedly loading these two sites.

==9677== Invalid read of size 4
==9677==    at 0x1075AEAD: WebCore::StringImpl::hash() const (StringImpl.h:76)
==9677==    by 0x1075B60E: WTF::StrHash<WebCore::StringImpl*>::hash(WebCore::StringImpl const*) (StringHash.h:34)
==9677==    by 0x10760174: WTF::IdentityHashTranslator<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::StrHash<WebCore::StringImpl*> >::hash(WebCore::StringImpl* const&) (HashTable.h:268)
==9677==    by 0x107628E8: std::pair<std::pair<WebCore::StringImpl*, int>*, bool> WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::lookupForWriting<WebCore::StringImpl*, WTF::IdentityHashTranslator<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::StrHash<WebCore::StringImpl*> > >(WebCore::StringImpl* const&) (HashTable.h:484)
==9677==    by 0x10762A00: WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::lookupForWriting(WebCore::StringImpl* const&) (HashTable.h:340)
==9677==    by 0x10762A8A: WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::reinsert(std::pair<WebCore::StringImpl*, int>&) (HashTable.h:713)
==9677==    by 0x10763C4E: WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::rehash(int) (HashTable.h:850)
==9677==    by 0x108B35F3: WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::shrink() (HashTable.h:350)
==9677==  Address 0xB71E0E8 is 16 bytes inside a block of size 24 free'd
==9677==    at 0x43C7506: operator delete(void*) (vg_replace_malloc.c:244)
==9677==    by 0x1072B5C6: WebCore::Shared<WebCore::StringImpl>::deref() (Shared.h:52)
==9677==    by 0x1072B5F8: WTF::RefPtr<WebCore::StringImpl>::~RefPtr() (RefPtr.h:45)
==9677==    by 0x1072B61C: WebCore::String::~String() (PlatformString.h:56)
==9677==    by 0x1073337E: WebCore::AtomicString::~AtomicString() (AtomicString.h:31)
==9677==    by 0x107F4681: WebCore::Attribute::~Attribute() (Attribute.h:58)
==9677==    by 0x1080E99E: WebCore::MappedAttribute::~MappedAttribute() (MappedAttribute.h:42)
==9677==    by 0x107DA11D: WebCore::Shared<WebCore::Attribute>::deref() (Shared.h:52)

[Grace Kloba]

Created attachment 17980 [details]
patch to show the problem

I created this patch to show the problem I found with Valgrind. If you load cnn.com followed by yahoo.com, you should hit the assertion. If not, reload cnn.com and followed by yahoo.com again.

[David Kilzer (:ddkilzer)]

<rdar://problem/5654994>

[Alexey Proskuryakov]

Sounds like a P1.

[Grace Kloba]

Forgot to mention that the problem is that HTMLDocument holds the Hash<StringImpl*, int>. If the String is deref before pulling itself out of the HashTable, it will cause problem later. 
The question is should we ensure the String is pulled out of the hash table by explicitly checking or should we fully depend on the rest of the system doing the correct way.

[Darin Adler]

If this is due to a string that's in one of the NameCountMap objects and is not removed, then we need to fix that problem. I'd like to see more of the backtrace.

[Grace Kloba]

I was able to reproduce this with Safari with the patch provided. Hope it will help for your debugging. It always happens to the last <img> with name="cookieCrumb". As the String is deref while it is not removed from the Document's HashMap, the program crashed later when the HashMap needs to shrink.

Here is a crash log from our run.

#0  0xaa32d33a in WebCore::StringImpl::computeHash (m_data=0x0, len=1545968) at libs/WebKitLib/WebKit/WebCore/platform/StringImpl.cpp:1119
#1  0xaa0a9310 in WebCore::StringImpl::hash (this=0x539430) at libs/WebKitLib/WebKit/WebCore/platform/StringImpl.h:76
#2  0xaa0a9344 in WTF::StrHash<WebCore::StringImpl*>::hash (key=0x539430) at libs/WebKitLib/WebKit/WebCore/platform/StringHash.h:34
#3  0xaa0a9372 in WTF::IdentityHashTranslator<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::StrHash<WebCore::StringImpl*> >::hash (key=@0x195268)
   at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:268
#4  0xaa0a96e4 in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::lookupForWriting<WebCore::StringImpl*, WTF::IdentityHashTranslator<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::StrHash<WebCore::StringImpl*> > > (this=0x1ea4fc, key=@0x195268)
   at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:484
#5  0xaa0a983e in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::lookupForWriting (this=0x1ea4fc,
   key=@0x195268) at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:340
#6  0xaa0a987c in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::reinsert (this=0x1ea4fc, entry=@0x195268)
   at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:719
#7  0xaa0a9950 in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::rehash (this=0x1ea4fc, newTableSize=64)
   at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:850
#8  0xaa21675c in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::shrink (this=0x1ea4fc)
   at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:350
#9  0xaa2167de in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::remove (this=0x1ea4fc, pos=0x194f98)
   at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:775
#10 0xaa21685c in WTF::HashTable<WebCore::StringImpl*, std::pair<WebCore::StringImpl*, int>, WTF::PairFirstExtractor<std::pair<WebCore::StringImpl*, int> >, WTF::StrHash<WebCore::StringImpl*>, WTF::PairHashTraits<WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >, WTF::HashTraits<WebCore::StringImpl*> >::remove (this=0x1ea4fc, it=
       {m_iterator = {m_position = 0x194f98, m_endPosition = 0x195370}}) at out/target/product/sooner/obj/include/JavaScriptCore/HashTable.h:786
#11 0xaa2168e0 in WTF::HashMap<WebCore::StringImpl*, int, WTF::StrHash<WebCore::StringImpl*>, WTF::HashTraits<WebCore::StringImpl*>, WTF::HashTraits<int> >::remove (this=0x1ea4fc,
   it={m_impl = {m_iterator = {m_position = 0x194f98, m_endPosition = 0x195370}}}) at out/target/product/sooner/obj/include/JavaScriptCore/HashMap.h:311
#12 0xaa21431a in removeItemFromMap (map=@0x1ea4fc, name=@0x1292f4) at libs/WebKitLib/WebKit/WebCore/html/HTMLDocument.cpp:314
#13 0xaa214374 in WebCore::HTMLDocument::removeDocExtraNamedItem (this=0x1e9da0, name=@0x1292f4) at libs/WebKitLib/WebKit/WebCore/html/HTMLDocument.cpp:341
#14 0xaa2334f8 in WebCore::HTMLImageElement::removedFromDocument (this=0x129290) at libs/WebKitLib/WebKit/WebCore/html/HTMLImageElement.cpp:209
#15 0xaa133f6e in WebCore::ContainerNode::removedFromDocument (this=0x20f6f8) at libs/WebKitLib/WebKit/WebCore/dom/ContainerNode.cpp:648
#16 0xaa15a372 in WebCore::Element::removedFromDocument (this=0x20f6f8) at libs/WebKitLib/WebKit/WebCore/dom/Element.cpp:668
#17 0xaa133f6e in WebCore::ContainerNode::removedFromDocument (this=0x58f5a8) at libs/WebKitLib/WebKit/WebCore/dom/ContainerNode.cpp:648

[Darin Adler]

I can't tell from the backtrace if this crash is happening inside the document destructor; the trace pasted here doesn't go far enough back.

I believe one correct fix would be code like this in the destructors of the various classes such as HTMLObjectElement:

    if (inDocument() && isDocNamedItem() && document()->isHTMLDocument()) {
        HTMLDocument* document = static_cast<HTMLDocument*>(this->document());
        document->removeNamedItem(m_name);
        document->removeExtraNamedItem(m_id);
    }

This code catches the case where the object is being destroyed while it's still in the document. However, it may be that the only way this can happen is when the nodes are being destroyed inside ~Document. If so, then we can do a more efficient fix by adding this code to ~HTMLDocument:

    m_namedItemCounts.clear();
    m_extraNamedItemCounts.clear();

If this is inside ~HTMLDocument, it's strange that some elements are being removed from the document and others are simply being destroyed in place. I'd still like to understand that.

We shouldn't need to add any new data members in any case.

[David Kilzer (:ddkilzer)]

This code has changed in the last 10 years.  Moving to Configuration Changed.