16512 2007-12-18 21:14:10 -0800 Valgrind: Invalid read of size 4 2018-09-19 18:36:07 -0700 1 1 1 Unclassified WebKit DOM 528+ (Nightly build) Mac OS X 10.4 RESOLVED CONFIGURATION CHANGED http://www.cnn.com InRadar, NeedsReduction P1 Normal --- 1 klobag webkit-unassigned ap darin ddkilzer feng inferno klobag oldest_to_newest 64692 0 klobag 2007-12-18 21:14:10 -0800 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) 64693 1 17980 klobag 2007-12-18 21:16:16 -0800 Created attachment 17980 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. 64697 2 ddkilzer 2007-12-18 22:21:28 -0800 <rdar://problem/5654994> 64710 3 ap 2007-12-19 00:51:50 -0800 Sounds like a P1. 64731 4 klobag 2007-12-19 08:28:01 -0800 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. 64743 5 darin 2007-12-19 10:43:35 -0800 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. 64744 6 klobag 2007-12-19 10:51:45 -0800 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 103319 7 darin 2008-12-23 13:05:11 -0800 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. 1461207 8 ddkilzer 2018-09-19 18:36:07 -0700 This code has changed in the last 10 years. Moving to Configuration Changed. 17980 2007-12-18 21:16:16 -0800 2010-06-10 16:28:22 -0700 patch to show the problem cnn.txt text/plain 2230 klobag SW5kZXg6IFdlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmNwcAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVudC5jcHAJKHJldmlzaW9uIDI3OTMzKQorKysgV2Vi Q29yZS9odG1sL0hUTUxJbWFnZUVsZW1lbnQuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC00Nyw2ICs0 Nyw3IEBAIEhUTUxJbWFnZUVsZW1lbnQ6OkhUTUxJbWFnZUVsZW1lbnQoRG9jdW0KICAgICAsIG1f Zm9ybShmKQogICAgICwgbV9jb21wb3NpdGVPcGVyYXRvcihDb21wb3NpdGVTb3VyY2VPdmVyKQog eworICAgIG9sZE5hbWVDb3VudCA9IDA7CiAgICAgaWYgKGYpCiAgICAgICAgIGYtPnJlZ2lzdGVy SW1nRWxlbWVudCh0aGlzKTsKIH0KQEAgLTU4LDEwICs1OSwxMyBAQCBIVE1MSW1hZ2VFbGVtZW50 OjpIVE1MSW1hZ2VFbGVtZW50KGNvbnN0CiAgICAgLCBtX2Zvcm0oMCkKICAgICAsIG1fY29tcG9z aXRlT3BlcmF0b3IoQ29tcG9zaXRlU291cmNlT3ZlcikKIHsKKyAgICBvbGROYW1lQ291bnQgPSAw OwogfQogCiBIVE1MSW1hZ2VFbGVtZW50Ojp+SFRNTEltYWdlRWxlbWVudCgpCiB7CisgICAgQVNT RVJUKG9sZE5hbWVDb3VudCA9PSAwKTsKKwogICAgIGlmIChtX2Zvcm0pCiAgICAgICAgIG1fZm9y bS0+cmVtb3ZlSW1nRWxlbWVudCh0aGlzKTsKIH0KQEAgLTEzNSw4ICsxMzksMTIgQEAgdm9pZCBI VE1MSW1hZ2VFbGVtZW50OjpwYXJzZU1hcHBlZEF0dHJpYgogICAgICAgICBTdHJpbmcgbmV3TmFt ZUF0dHIgPSBhdHRyLT52YWx1ZSgpOwogICAgICAgICBpZiAoaW5Eb2N1bWVudCgpICYmIGRvY3Vt ZW50KCktPmlzSFRNTERvY3VtZW50KCkpIHsKICAgICAgICAgICAgIEhUTUxEb2N1bWVudCogZG9j ID0gc3RhdGljX2Nhc3Q8SFRNTERvY3VtZW50Kj4oZG9jdW1lbnQoKSk7CisgICAgICAgICAgICBp ZiAob2xkTmFtZUF0dHIubGVuZ3RoKCkgPiAwKQorICAgICAgICAgICAgICAgIG9sZE5hbWVDb3Vu dC0tOwogICAgICAgICAgICAgZG9jLT5yZW1vdmVOYW1lZEl0ZW0ob2xkTmFtZUF0dHIpOwogICAg ICAgICAgICAgZG9jLT5hZGROYW1lZEl0ZW0obmV3TmFtZUF0dHIpOworICAgICAgICAgICAgaWYg KG5ld05hbWVBdHRyLmxlbmd0aCgpID4gMCkKKyAgICAgICAgICAgICAgICBvbGROYW1lQ291bnQr KzsKICAgICAgICAgfQogICAgICAgICBvbGROYW1lQXR0ciA9IG5ld05hbWVBdHRyOwogICAgIH0g ZWxzZSBpZiAoYXR0ci0+bmFtZSgpID09IGlkQXR0cikgewpAQCAtMTk1LDYgKzIwMyw5IEBAIHZv aWQgSFRNTEltYWdlRWxlbWVudDo6aW5zZXJ0ZWRJbnRvRG9jdW0KIAogICAgICAgICBkb2MtPmFk ZE5hbWVkSXRlbShvbGROYW1lQXR0cik7CiAgICAgICAgIGRvYy0+YWRkRG9jRXh0cmFOYW1lZEl0 ZW0ob2xkSWRBdHRyKTsKKyAgICAgICAgCisgICAgICAgIGlmIChvbGROYW1lQXR0ci5sZW5ndGgo KSA+IDApCisgICAgICAgICAgICBvbGROYW1lQ291bnQrKzsKICAgICB9CiAKICAgICBIVE1MRWxl bWVudDo6aW5zZXJ0ZWRJbnRvRG9jdW1lbnQoKTsKQEAgLTIwNyw2ICsyMTgsOSBAQCB2b2lkIEhU TUxJbWFnZUVsZW1lbnQ6OnJlbW92ZWRGcm9tRG9jdW1lCiAKICAgICAgICAgZG9jLT5yZW1vdmVO YW1lZEl0ZW0ob2xkTmFtZUF0dHIpOwogICAgICAgICBkb2MtPnJlbW92ZURvY0V4dHJhTmFtZWRJ dGVtKG9sZElkQXR0cik7CisKKyAgICAgICAgaWYgKG9sZE5hbWVBdHRyLmxlbmd0aCgpID4gMCkK KyAgICAgICAgICAgIG9sZE5hbWVDb3VudC0tOwogICAgIH0KIAogICAgIEhUTUxFbGVtZW50Ojpy ZW1vdmVkRnJvbURvY3VtZW50KCk7CkluZGV4OiBXZWJDb3JlL2h0bWwvSFRNTEltYWdlRWxlbWVu dC5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvaHRtbC9IVE1MSW1hZ2VFbGVtZW50LmgJKHJldmlz aW9uIDI3OTMzKQorKysgV2ViQ29yZS9odG1sL0hUTUxJbWFnZUVsZW1lbnQuaAkod29ya2luZyBj b3B5KQpAQCAtMTI0LDYgKzEyNCw4IEBAIHByb3RlY3RlZDoKICAgICBTdHJpbmcgb2xkTmFtZUF0 dHI7CiAgICAgU3RyaW5nIG9sZElkQXR0cjsKICAgICBDb21wb3NpdGVPcGVyYXRvciBtX2NvbXBv c2l0ZU9wZXJhdG9yOworICAgIAorICAgIGludCBvbGROYW1lQ291bnQ7CiB9OwogCiB9IC8vbmFt ZXNwYWNlCg==