Summary: | ASSERT in Document::unregisterCollection reloading apple.com | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||
Component: | DOM | Assignee: | Zan Dobersek <zan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | cdumez, commit-queue, darin, dbates, esprehn+autocc, joepeck, kangil.han, mitz, rniwa, zan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2014-07-22 12:22:25 PDT
FWIW, the collection is empty: (lldb) p m_collectionsInvalidatedAtDocument.isEmpty() (bool) $6 = true 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. 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? Also: http://alistapart.com/blog/post/ten-css-one-liners-to-replace-native-apps/ <rdar://problem/17778184> I'll try relaxing the assertion. (In reply to comment #3) > Opinions? Concept sounds fine. I didn’t read your new assertion carefully to be sure I understood the logic. Created attachment 235932 [details]
Patch
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.
Created attachment 235933 [details]
Patch
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. Created attachment 236017 [details]
Patch
This patch removes the assertion altogether.
Comment on attachment 236017 [details] Patch Clearing flags on attachment: 236017 Committed r172210: <http://trac.webkit.org/changeset/172210> All reviewed patches have been landed. Closing bug. Mass moving XML DOM bugs to the "DOM" Component. |