Bug 135168 – ASSERT in Document::unregisterCollection reloading apple.com

• Component: DOM
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
ASSERT reloading <http://www.apple.com>.

* STEPS TO REPRODUCE
1. Load http://www.apple.com
2. Reload
  => ASSERT

ASSERTION FAILED: m_inInvalidateNodeListAndCollectionCaches ? m_collectionsInvalidatedAtDocument.isEmpty() : m_collectionsInvalidatedAtDocument.contains(&collection)

(lldb) bt
* thread #1: tid = 0x6a39bc, 0x00000001133be3aa JavaScriptCore`WTFCrash + 42 at Assertions.cpp:329, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
  * frame #0: 0x00000001133be3aa JavaScriptCore`WTFCrash + 42 at Assertions.cpp:329
    frame #1: 0x0000000114bbb6c7 WebCore`WebCore::Document::unregisterCollection(this=0x00007fd55c8b4000, collection=0x00007fd55c386f10) + 279 at Document.cpp:3516
    frame #2: 0x0000000115037f1e WebCore`WebCore::HTMLCollection::~HTMLCollection(this=0x00007fd55c386f10) + 94 at HTMLCollection.cpp:156
    frame #3: 0x00000001150380c5 WebCore`WebCore::HTMLCollection::~HTMLCollection(this=0x00007fd55c386f10) + 21 at HTMLCollection.cpp:154
    frame #4: 0x00000001150380e9 WebCore`WebCore::HTMLCollection::~HTMLCollection(this=0x00007fd55c386f10) + 25 at HTMLCollection.cpp:154
    frame #5: 0x0000000114693123 WebCore`WTF::RefCounted<WebCore::HTMLCollection>::deref(this=0x00007fd55c386f20) + 83 at RefCounted.h:146
    frame #6: 0x00000001155eb039 WebCore`WebCore::JSHTMLCollection::releaseImplIfNotNull(this=0x000000011e048e10) + 57 at JSHTMLCollection.h:63
    frame #7: 0x00000001155ea615 WebCore`WebCore::JSHTMLCollection::~JSHTMLCollection(this=0x000000011e048e10) + 21 at JSHTMLCollection.cpp:172
    frame #8: 0x00000001155ea5f5 WebCore`WebCore::JSHTMLCollection::~JSHTMLCollection(this=0x000000011e048e10) + 21 at JSHTMLCollection.cpp:171
    frame #9: 0x00000001155e9b3d WebCore`WebCore::JSHTMLCollection::destroy(cell=0x000000011e048e10) + 29 at JSHTMLCollection.cpp:167
    frame #10: 0x00000001131ccf86 JavaScriptCore`void JSC::MarkedBlock::callDestructor<(this=0x000000011e040000, cell=0x000000011e048e10)2>(JSC::JSCell*) + 70 at MarkedBlock.cpp:65
    frame #11: 0x00000001131ccd38 JavaScriptCore`JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(this=0x000000011e040000)3, (JSC::MarkedBlock::SweepMode)1, (JSC::MarkedBlock::DestructorType)2>() + 216 at MarkedBlock.cpp:89
    frame #12: 0x00000001131cbe9a JavaScriptCore`JSC::MarkedBlock::FreeList JSC::MarkedBlock::sweepHelper<(this=0x000000011e040000, sweepMode=SweepToFreeList)2>(JSC::MarkedBlock::SweepMode) + 282 at MarkedBlock.cpp:142
    frame #13: 0x00000001131cb689 JavaScriptCore`JSC::MarkedBlock::sweep(this=0x000000011e040000, sweepMode=SweepToFreeList) + 233 at MarkedBlock.cpp:121
    frame #14: 0x00000001131cab7e JavaScriptCore`JSC::MarkedAllocator::tryAllocateHelper(this=0x00007fd55f831990, bytes=32) + 270 at MarkedAllocator.cpp:80
    frame #15: 0x00000001131c9122 JavaScriptCore`JSC::MarkedAllocator::tryAllocate(this=0x00007fd55f831990, bytes=32) + 114 at MarkedAllocator.cpp:129
    frame #16: 0x00000001131c8a0e JavaScriptCore`JSC::MarkedAllocator::allocateSlowCase(this=0x00007fd55f831990, bytes=32) + 254 at MarkedAllocator.cpp:171
    frame #17: 0x0000000114771b41 WebCore`JSC::MarkedAllocator::allocate(this=0x00007fd55f831990, bytes=32) + 81 at MarkedAllocator.h:95
    frame #18: 0x00000001147f77f9 WebCore`JSC::MarkedSpace::allocateWithNormalDestructor(this=0x00007fd55f831928, bytes=32) + 41 at MarkedSpace.h:251
    frame #19: 0x00000001147f77c6 WebCore`JSC::Heap::allocateWithNormalDestructor(this=0x00007fd55f831618, bytes=32) + 118 at HeapInlines.h:187
    frame #20: 0x00000001155a0d37 WebCore`void* JSC::allocateCell<WebCore::JSEvent>(heap=0x00007fd55f831618, size=32) + 151 at JSCellInlines.h:122
    frame #21: 0x00000001155a0c8f WebCore`void* JSC::allocateCell<WebCore::JSEvent>(heap=0x00007fd55f831618) + 31 at JSCellInlines.h:136
    frame #22: 0x00000001155a0ade WebCore`WebCore::JSEvent::create(structure=0x0000000123852570, globalObject=0x000000011e02e870, impl=PassRefPtr<WebCore::Event> at 0x00007fff56892bb0) + 46 at JSEvent.h:36
    frame #23: 0x0000000115592a66 WebCore`WebCore::JSDOMWrapper* WebCore::createWrapper<WebCore::JSEvent, WebCore::Event>(globalObject=0x000000011e02e870, node=0x00007fd55c2bcf70) + 214 at JSDOMBinding.h:229
    frame #24: 0x0000000115591c87 WebCore`WebCore::toJS((null)=0x000000011e02e8b0, globalObject=0x000000011e02e870, event=0x00007fd55c2bcf70) + 455 at JSEventCustom.cpp:68
    frame #25: 0x00000001155a56d6 WebCore`WebCore::JSEventListener::handleEvent(this=0x00007fd55c3b9af0, scriptExecutionContext=0x00007fd55c8b40a0, event=0x00007fd55c2bcf70) + 774 at JSEventListener.cpp:114
    frame #26: 0x0000000114dac9cf WebCore`WebCore::EventTarget::fireEventListeners(this=0x00007fd55c0e61c0, event=0x00007fd55c2bcf70, d=0x00007fd55c0e61c8, entry=0x00007fd55c3cd980) + 1503 at EventTarget.cpp:246
    frame #27: 0x0000000114dac29e WebCore`WebCore::EventTarget::fireEventListeners(this=0x00007fd55c0e61c0, event=0x00007fd55c2bcf70) + 334 at EventTarget.cpp:197
    frame #28: 0x0000000114cd5c5b WebCore`WebCore::DOMWindow::dispatchEvent(this=0x00007fd55c0e61c0, prpEvent=<unavailable>, prpTarget=(m_ptr = WebCore::EventTarget * = 0x0000000000000000)) + 539 at DOMWindow.cpp:1897
    frame #29: 0x0000000114f11667 WebCore`WebCore::FrameLoader::stopLoading(this=0x00007fd55c02a0b0, unloadEventPolicy=UnloadEventPolicyUnloadAndPageHide) + 983 at FrameLoader.cpp:464
    frame #30: 0x0000000114f11c9f WebCore`WebCore::FrameLoader::closeURL(this=0x00007fd55c02a0b0) + 111 at FrameLoader.cpp:543
    frame #31: 0x0000000114f1a2a9 WebCore`WebCore::FrameLoader::transitionToCommitted(this=0x00007fd55c02a0b0, cachedPage=0x0000000000000000) + 345 at FrameLoader.cpp:1901
    frame #32: 0x0000000114f1993f WebCore`WebCore::FrameLoader::commitProvisionalLoad(this=0x00007fd55c02a0b0) + 2143 at FrameLoader.cpp:1798
    frame #33: 0x0000000114c051ec WebCore`WebCore::DocumentLoader::commitIfReady(this=0x00007fd55b829400) + 60 at DocumentLoader.cpp:355
    frame #34: 0x0000000114c0825c WebCore`WebCore::DocumentLoader::commitLoad(this=0x00007fd55b829400, data=0x00007fd55e18e000, length=10161) + 76 at DocumentLoader.cpp:763
    frame #35: 0x0000000114c08b7d WebCore`WebCore::DocumentLoader::dataReceived(this=0x00007fd55b829400, resource=0x00007fd565eba3f0, data=0x00007fd55e18e000, length=10161) + 1693 at DocumentLoader.cpp:888
    frame #36: 0x0000000114815031 WebCore`WebCore::CachedRawResource::notifyClientsDataWasReceived(this=0x00007fd565eba3f0, data=0x00007fd55e18e000, length=10161) + 161 at CachedRawResource.cpp:114
    frame #37: 0x0000000114814f17 WebCore`WebCore::CachedRawResource::addDataBuffer(this=0x00007fd565eba3f0, data=0x00007fd55c2e5c80) + 215 at CachedRawResource.cpp:70
    frame #38: 0x00000001162c07fe WebCore`WebCore::SubresourceLoader::didReceiveDataOrBuffer(this=0x00007fd565c9f8c0, data=0x00007fd55f916a50, length=10161, prpBuffer=PassRefPtr<WebCore::SharedBuffer> at 0x00007fff56893e60, encodedDataLength=-1, dataPayloadType=DataPayloadBytes) + 478 at SubresourceLoader.cpp:274
    frame #39: 0x00000001162c0606 WebCore`WebCore::SubresourceLoader::didReceiveData(this=0x00007fd565c9f8c0, data=0x00007fd55f916a50, length=10161, encodedDataLength=-1, dataPayloadType=DataPayloadBytes) + 102 at SubresourceLoader.cpp:250
    frame #40: 0x00000001109a77ff WebKit`WebKit::WebResourceLoader::didReceiveData(this=0x00007fd568644f50, data=0x00007fff56894020, encodedDataLength=-1) + 223 at WebResourceLoader.cpp:148
    frame #41: 0x00000001109ab175 WebKit`void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (object=0x00007fd568644f50, function=0x00000001109a7720, args=0x00007fff56894020, (null)=index_sequence<0, 1> at 0x00007fff56893f38)(IPC::DataReference const&, long long), std::__1::tuple<IPC::DataReference, long long>, 0ul, 1ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(IPC::DataReference const&, long long), std::__1::tuple<IPC::DataReference, long long>&&, std::index_sequence<0ul, 1ul>) + 181 at HandleMessage.h:16
    frame #42: 0x00000001109ab018 WebKit`void IPC::callMemberFunction<WebKit::WebResourceLoader, void (args=0x00007fff56894020, object=0x00007fd568644f50, function=0x00000001109a7720)(IPC::DataReference const&, long long), std::__1::tuple<IPC::DataReference, long long>, std::make_index_sequence<2ul> >(std::__1::tuple<IPC::DataReference, long long>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(IPC::DataReference const&, long long)) + 88 at HandleMessage.h:22
    frame #43: 0x00000001109aa1eb WebKit`void IPC::handleMessage<Messages::WebResourceLoader::DidReceiveData, WebKit::WebResourceLoader, void (decoder=0x00007fd565f18d80, object=0x00007fd568644f50, function=0x00000001109a7720)(IPC::DataReference const&, long long)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(IPC::DataReference const&, long long)) + 283 at HandleMessage.h:120
    frame #44: 0x00000001109a97fc WebKit`WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(this=0x00007fd568644f50, (null)=0x00007fd55c3170f0, decoder=0x00007fd565f18d80) + 636 at WebResourceLoaderMessageReceiver.cpp:68
    frame #45: 0x000000011046bf60 WebKit`WebKit::NetworkProcessConnection::didReceiveMessage(this=0x00007fd55c3090f0, connection=0x00007fd55c3170f0, decoder=0x00007fd565f18d80) + 160 at NetworkProcessConnection.cpp:61
    frame #46: 0x00000001102698c3 WebKit`IPC::Connection::dispatchMessage(this=0x00007fd55c3170f0, decoder=0x00007fd565f18d80) + 51 at Connection.cpp:809
    frame #47: 0x0000000110261ba0 WebKit`IPC::Connection::dispatchMessage(this=0x00007fd55c3170f0, message=unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> > at 0x00007fff568944a8) + 368 at Connection.cpp:828
    frame #48: 0x000000011026969f WebKit`IPC::Connection::dispatchOneMessage(this=0x00007fd55c3170f0) + 1439 at Connection.cpp:856
    frame #49: 0x000000011027a8f2 WebKit`WTF::FunctionWrapper<void (this=0x00007fd55c5f5c30, c=0x00007fd55c3170f0)()>::operator()(IPC::Connection*) + 114 at Functional.h:218
    frame #50: 0x000000011027a875 WebKit`WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (this=0x00007fd55c5f5c20)()>, void (IPC::Connection*)>::operator()() + 53 at Functional.h:496
    frame #51: 0x0000000110280a62 WebKit`WTF::Function<void (this=0x00007fd565f288d8)>::operator()() const + 114 at Functional.h:704
    frame #52: 0x00000001102809dc WebKit`std::__1::__function::__func<WTF::Function<void ()>, std::__1::allocator<WTF::Function<void ()> >, void ()>::operator()() [inlined] decltype(this=0x00007fd565f288d8, __f=0x00007fd565f288d8)>&>(fp)(std::__1::forward<>(fp0))) std::__1::__invoke<WTF::Function<void ()>&>(WTF::Function<void ()>&&&) + 60 at __functional_base:413
    frame #53: 0x00000001102809cb WebKit`std::__1::__function::__func<WTF::Function<void (this=0x00007fd565f288d0)>, std::__1::allocator<WTF::Function<void ()> >, void ()>::operator()() + 43 at functional:1370
    frame #54: 0x00000001133e747a JavaScriptCore`std::__1::function<void (this=0x00007fff568949e0)>::operator()() const + 26 at functional:1755
    frame #55: 0x00000001133f6e12 JavaScriptCore`WTF::RunLoop::performWork(this=0x00007fd55c30c460) + 306 at RunLoop.cpp:104
    frame #56: 0x00000001133f8304 JavaScriptCore`WTF::RunLoop::performWork(context=0x00007fd55c30c460) + 36 at RunLoopCF.cpp:38
    frame #57: 0x00007fff88c06111 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
    frame #58: 0x00007fff88bf7c8d CoreFoundation`__CFRunLoopDoSources0 + 269
    frame #59: 0x00007fff88bf72db CoreFoundation`__CFRunLoopRun + 923
    frame #60: 0x00007fff88bf6cf8 CoreFoundation`CFRunLoopRunSpecific + 296
    ...

(lldb) up
frame #1: 0x0000000114bbb6c7 WebCore`WebCore::Document::unregisterCollection(this=0x00007fd55c8b4000, collection=0x00007fd55c386f10) + 279 at Document.cpp:3516
   3513	    if (!collection.isRootedAtDocument())
   3514	        return;
   3515	
-> 3516	    ASSERT(m_inInvalidateNodeListAndCollectionCaches
   3517	        ? m_collectionsInvalidatedAtDocument.isEmpty()
   3518	        : m_collectionsInvalidatedAtDocument.contains(&collection));
   3519	    m_collectionsInvalidatedAtDocument.remove(&collection);

(lldb) p m_inInvalidateNodeListAndCollectionCaches
(bool) $0 = false

[Joseph Pecoraro]

FWIW, the collection is empty:

(lldb) p m_collectionsInvalidatedAtDocument.isEmpty()
(bool) $6 = true

[Zan Dobersek]

The issue is not with the assertion itself, but with the HashSet being properly zeroed out in Document::invalidateNodeListAndCollectionCaches() after HashTable gained proper move constructor and assignment operator.

The problematic HTMLCollection is of the DocLinks type. After it is created, the first change in any class attribute on www.apple.com (like hovering on/off a menu) results in the following call chain:

#0  WebCore::HTMLCollection::invalidateCache (this=0x3936000, attrName=0x7ffe59fb0de8 <WebCore::HTMLNames::classAttr>) at ../../Source/WebCore/html/HTMLCollection.h:114
#1  0x00007ffe56d016b9 in WebCore::Document::invalidateNodeListAndCollectionCaches (this=0x2b80a40, attrName=0x7ffe59fb0de8 <WebCore::HTMLNames::classAttr>)
    at ../../Source/WebCore/dom/Node.cpp:722
#2  0x00007ffe56d017e5 in WebCore::Node::invalidateNodeListAndCollectionCachesInAncestors (this=0x2788210, attrName=0x7ffe59fb0de8 <WebCore::HTMLNames::classAttr>, 
    attributeOwnerElement=0x2788210) at ../../Source/WebCore/dom/Node.cpp:742
#3  0x00007ffe56caeb78 in WebCore::Element::attributeChanged (this=0x2788210, name=..., oldValue=..., newValue=...) at ../../Source/WebCore/dom/Element.cpp:1103
#4  0x00007ffe56d52218 in WebCore::StyledElement::attributeChanged (this=0x2788210, name=..., oldValue=..., newValue=..., reason=WebCore::Element::ModifiedDirectly)
    at ../../Source/WebCore/dom/StyledElement.cpp:162
#5  0x00007ffe56cb2826 in WebCore::Element::didAddAttribute (this=0x2788210, name=..., value=...) at ../../Source/WebCore/dom/Element.cpp:2771
#6  0x00007ffe56cb27bb in WebCore::Element::addAttributeInternal (this=0x2788210, name=..., value=..., 
    inSynchronizationOfLazyAttribute=WebCore::Element::NotInSynchronizationOfLazyAttribute) at ../../Source/WebCore/dom/Element.cpp:1842
#7  0x00007ffe56cb83fb in WebCore::Element::setAttributeInternal (this=0x2788210, index=4294967295, name=..., newValue=..., 
    inSynchronizationOfLazyAttribute=WebCore::Element::NotInSynchronizationOfLazyAttribute) at ../../Source/WebCore/dom/Element.cpp:1032
#8  0x00007ffe56cae80a in WebCore::Element::setAttributeWithoutSynchronization (this=0x2788210, name=..., value=...) at ../../Source/WebCore/dom/Element.cpp:1014
#9  0x00007ffe57e489ff in WebCore::setJSElementClassName (exec=0x7fff008e45a0, baseObject=0x7ffe000bfdd0, thisValue=140728898809168, encodedValue=140728901566416)
    at DerivedSources/WebCore/JSElement.cpp:2276
#10 0x00007ffe5ac3e7c2 in JSC::callCustomSetter (exec=0x7fff008e45a0, customGetterSetter=..., base=0x7ffe000bfdd0, thisValue=..., value=...)
    at ../../Source/JavaScriptCore/runtime/CustomGetterSetter.cpp:44
#11 0x00007ffe5acb475e in JSC::JSObject::put (cell=0x7ffe0005ed50, exec=0x7fff008e45a0, propertyName=..., value=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSObject.cpp:393
#12 0x00007ffe5a99c312 in JSC::JSValue::put (this=0x7fff008e4488, exec=0x7fff008e45a0, propertyName=..., value=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSCJSValueInlines.h:729
#13 0x00007ffe5adbce45 in llint_slow_path_put_by_id (exec=0x7fff008e45a0, pc=0x3515a40) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:637
#14 0x00007ffe5adc88a9 in llint_entry () from /home/zan/dvt/webkit/hackhack/WebKitBuild/Debug/lib/libjavascriptcoregtk-3.0.so.0
#15 0x00007ffe5adcbf2b in llint_entry () from /home/zan/dvt/webkit/hackhack/WebKitBuild/Debug/lib/libjavascriptcoregtk-3.0.so.0
#16 0x00007ffe5adc5706 in callToJavaScript () from /home/zan/dvt/webkit/hackhack/WebKitBuild/Debug/lib/libjavascriptcoregtk-3.0.so.0

HTMLCollection::invalidateCache() then does nothing since the collection won't be invalidated for any change to a class attribute (like it would be in case of a changed href attribute).

The Document::invalidateNodeListAndCollectionCaches() has already zeroed out the m_collectionsInvalidatedAtDocument HashSet so the assertion in Document::unregisterCollection() fails when it gets called during the HTMLCollection's destruction.

This wasn't a problem before the HashTable changes since the entries were simply copied, and the assertion wouldn't be hit.

[Zan Dobersek]

I think that Document::invalidateNodeListAndCollectionCaches() should be considered as working as intended.

I'd recommend relaxing the assertion in Document::unregisterCollection() instead. It should only check that the HashSet object is empty if the unregister action is coming from invalidateNodeListAndCollectionCaches().

Something like this:
    ASSERT(m_inInvalidateNodeListAndCollectionCaches && !m_listsInvalidatedAtDocument.isEmpty()
        || !m_inInvalidateNodeListAndCollectionCaches);

Opinions?

[Alexey Proskuryakov]

Also: http://alistapart.com/blog/post/ten-css-one-liners-to-replace-native-apps/

<rdar://problem/17778184>

[Zan Dobersek]

I'll try relaxing the assertion.

[Darin Adler]

(In reply to comment #3)
> Opinions?

Concept sounds fine. I didn’t read your new assertion carefully to be sure I understood the logic.

[Zan Dobersek]

Created attachment 235932 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 2 files


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

[Zan Dobersek]

Created attachment 235933 [details]
Patch

[Darin Adler]

Comment on attachment 235933 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=235933&action=review

I’m going to say review+, but I think removing the assertion entirely would be better.

> Source/WebCore/dom/Document.cpp:3527
> -    ASSERT(m_inInvalidateNodeListAndCollectionCaches
> -        ? m_collectionsInvalidatedAtDocument.isEmpty()
> -        : m_collectionsInvalidatedAtDocument.contains(&collection));
> +    // This only ensures that Document::invalidateNodeListAndCollectionCaches() empties out
> +    // the m_collectionsInvalidatedAtDocument HashSet but shouldn't assert that the collection
> +    // is contianed in the HashSet otherwise. See https://webkit.org/b/135168.
> +    ASSERT(m_inInvalidateNodeListAndCollectionCaches && m_collectionsInvalidatedAtDocument.isEmpty()
> +        || !m_inInvalidateNodeListAndCollectionCaches);

I don’t think we need an assertion at all. This asserts so little that it’s pretty much useless. Also, lets not land the misspelling contianed.

[Zan Dobersek]

Created attachment 236017 [details]
Patch

This patch removes the assertion altogether.

[Zan Dobersek]

Comment on attachment 236017 [details]
Patch

Clearing flags on attachment: 236017

Committed r172210: <http://trac.webkit.org/changeset/172210>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Lucas Forschler]

Mass moving XML DOM bugs to the "DOM" Component.