Bug 70810

Created attachment 112318 [details] Simple test-page

Created attachment 112321 [details] Patch

Could you please check how this affects DOM performance on Dromaeo?

(In reply to comment #3) > Could you please check how this affects DOM performance on Dromaeo? Without the patch: http://dromaeo.com/?id=153672 - 1074.13runs/s (Total) With the patch: http://dromaeo.com/?id=153677 - 1073.77runs/s (Total)

Comment on attachment 112321 [details] Patch Something seems wrong here. The notifyNodeListsAttributeChanged call should be made from Node::dispatchSubtreeModifiedEvent() - there is even a comment saying that it's there to handle the name attribute. Why doesn't that work? Is DOMSubtreeModified event broken, too?

(In reply to comment #5) > (From update of attachment 112321 [details]) > Something seems wrong here. > > The notifyNodeListsAttributeChanged call should be made from Node::dispatchSubtreeModifiedEvent() - there is even a comment saying that it's there to handle the name attribute. Why doesn't that work? Is DOMSubtreeModified event broken, too? Currently DOMSubtreeModified is not getting fired in case of name attribute. After debugging class attributes behavior below is my observation : When classAttr changes it calls classAttributeChanged() from StyledElement::parseMappedAttribute(). Now classAttributeChanged() invokes dispatchSubtreeModifiedEvent() which internally sends DOMSubtreeModified event and calls Node::notifyNodeListsAttributeChanged() to invalidate m_classNodeListCache.

Hmm, I can't easily tell if the fix is in the right place, deferring to other reviewers. There is a lot of fishy stuff going on - an first of all, the existing code in Node::dispatchSubtreeModifiedEvent() seems misplaced. A function that's named to dispatch an unimportant deprecated event shouldn't be doing super important work like flushing caches! Interestingly, if the element doesn't have a name attribute yet, DOMSubtreeModified gets dispatched through NamedNodeMap::addAttribute(). And adding class attribute results in two DOMSubtreeModified events.

For reference, the code in Node::dispatchSubtreeModifiedEvent() was added in <http://trac.webkit.org/changeset/9990>.

After investigating more, following are the observations: When we do: element.className = 'myClass'; If the element doesn't have a content attribute "class" specified, then it fires two DOMSubTreeMidified events. If specified then it fires only one event. 1. In Element::setAttribute() if element doesn't have a content attribute "class" present, then it calls NamedNodeMap::addAttribute() which fires the first DOMSubTreeMidified event. And second event is fired from StyledElement::classAttributeChanged(). 2. Now if the element has a content attribute "class" present, then Element::setAttribute() calls setValue() to update the value of the old "class" attribute. And only one DOMSubTreeMidified gets fired from StyledElement::classAttributeChanged(). In the case of "name" attribute DOMSubTreeMidified event is fired only if element doesn't have a content attribute "name" specified. So, we should not send the DOMSubTreeMidified event from StyledElement::classAttributeChanged(). Rather we need to send the event irrespective of "adding attribute" or "updating attribute's value". This will fix both class and name attribute issue. Please let me know your comments.

Created attachment 112854 [details] Simple test-page for class attr issue

For reference, the code for sending DOMSubTreeMidified event from StyledElement::classAttributeChanged() was add in http://trac.webkit.org/changeset/37800

Comment on attachment 112321 [details] Patch This test case only covers the <div> element. The patch will work for that element, but not for the <a>, <applet>, <embed>, <frame>, <form>, <iframe>, <img>, <map>, <meta>, <object>, or <param> element, for example, due to code in those classes’ parseMappedAttribute functions that does not call through to the base class parseMappedAttribute function.

Comment on attachment 112321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112321&action=review > Source/WebCore/html/HTMLElement.cpp:205 > + } else if (attr->name() == nameAttr) { > + notifyNodeListsAttributeChanged(); We should be doing this where enqueueAttributesMutationRecord is called in Element::setAttribute.

Created attachment 118239 [details] Updated patch Incorporating review comments.

So... it seems like we have a bug in Node::notifyLocalNodeListsAttributeChanged: if (!isAttributeNode()) data->nodeLists()->invalidateCachesThatDependOnAttributes(); else data->nodeLists()->invalidateCaches(); We should be clearing name/label cache when the node is NOT attribute node, not clearing when it's not.

Comment on attachment 118239 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118239&action=review > Source/WebCore/dom/Element.cpp:652 > + if (old && old->name() == nameAttr) > + notifyNodeListsAttributeChanged(); This does not look right. Responses to changes to specific attributes do not normally belong in this function.

Comment on attachment 112321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112321&action=review >> Source/WebCore/html/HTMLElement.cpp:205 >> + notifyNodeListsAttributeChanged(); > > We should be doing this where enqueueAttributesMutationRecord is called in Element::setAttribute. Upon more investigation, calling notifyNodeListsAttributeChanged() here seems fine. Sorry to lead you to a wrong direction :( This is similar to how we end up calling notifyNodeListsAttributeChanged in StyledElement::classAttributeChanged via dispatchSubtreeModifiedEvent.

Created attachment 118594 [details] Updated patch

Calling invalidateNodeListsCacheAfterAttributeChanged() from HTMLElement::parseMappedAttribute() to invalidate NodeListCache. Also did the change for the <a>, <applet>, <embed>, <frame>, <form>, <iframe>, <img>, <map>, <meta>, <object>, or <param> element as code in those classes parseMappedAttribute functions that does not call through to the base class i.e, HTMLElement parseMappedAttribute() function.

Comment on attachment 118594 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118594&action=review > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:32 > + shouldBeTrue("document.getElementsByName('fullname').length != oldLength"); You could create a function like createElement() that takes a tag name and returned the element. That way, shouldBeTrue will include the tag name and you don't need to add extra lines by debug.

(In reply to comment #20) > (From update of attachment 118594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118594&action=review > > > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:32 > > + shouldBeTrue("document.getElementsByName('fullname').length != oldLength"); > > You could create a function like createElement() that takes a tag name and returned the element. That way, shouldBeTrue will include the tag name and you don't need to add extra lines by debug. I am facing some problem here. Suppose, getElement(tagName) is the function which return me the element based on the tag name. Now when we do shouldBeTrue("getElement(); document.getElementsByName('fullname').length != oldLength") it gives "PASS getElement(); document.getElementsByName('fullname').length != oldLength is true" in the expected output rather giving me the actual tag name in the expected result. Can you please correct me if I am missing something here. Thanks.

(In reply to comment #21) > Suppose, getElement(tagName) is the function which return me the element based on the tag name. > Now when we do shouldBeTrue("getElement(); document.getElementsByName('fullname').length != oldLength") it gives "PASS getElement(); document.getElementsByName('fullname').length != oldLength is true" in the expected output rather giving me the actual tag name in the expected result. > > Can you please correct me if I am missing something here. Thanks. The correct way to write this is more like this: shouldBe("getElement(); document.getElementsByName('" + fullname + "').length", "oldLength") There are other refinements you can make too, but that's the basic pattern that works well.

Comment on attachment 118594 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118594&action=review >>> LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:32 >>> + shouldBeTrue("document.getElementsByName('fullname').length != oldLength"); >> >> You could create a function like createElement() that takes a tag name and returned the element. That way, shouldBeTrue will include the tag name and you don't need to add extra lines by debug. > > I am facing some problem here. > Suppose, getElement(tagName) is the function which return me the element based on the tag name. > Now when we do shouldBeTrue("getElement(); document.getElementsByName('fullname').length != oldLength") it gives "PASS getElement(); document.getElementsByName('fullname').length != oldLength is true" in the expected output rather giving me the actual tag name in the expected result. > > Can you please correct me if I am missing something here. Thanks. You can do: shouldBeTrue("CreateTwoElements('" + tagName + "); document.getElementsByName('fullname').length", "2"); shouldBeTrue("document.querySelector("' + tagName + '"); document.getElementsByName('fullname').length", "1"); // Remove those two elements here.

(In reply to comment #23) > (From update of attachment 118594 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118594&action=review > > >>> LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:32 > >>> + shouldBeTrue("document.getElementsByName('fullname').length != oldLength"); > >> > >> You could create a function like createElement() that takes a tag name and returned the element. That way, shouldBeTrue will include the tag name and you don't need to add extra lines by debug. > > > > I am facing some problem here. > > Suppose, getElement(tagName) is the function which return me the element based on the tag name. > > Now when we do shouldBeTrue("getElement(); document.getElementsByName('fullname').length != oldLength") it gives "PASS getElement(); document.getElementsByName('fullname').length != oldLength is true" in the expected output rather giving me the actual tag name in the expected result. > > > > Can you please correct me if I am missing something here. Thanks. > > You can do: > shouldBeTrue("CreateTwoElements('" + tagName + "); document.getElementsByName('fullname').length", "2"); > shouldBeTrue("document.querySelector("' + tagName + '"); document.getElementsByName('fullname').length", "1"); > // Remove those two elements here. Darin/Ryosuke thanks for the help. I will upload the modified patch.

Created attachment 118615 [details] Updated patch Incorporating review comments.

Comment on attachment 118615 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118615&action=review > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:34 > + oldLength = document.getElementsByName('fullname').length; We don't need this variable anymore, right?

(In reply to comment #26) > (From update of attachment 118615 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118615&action=review > > > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:34 > > + oldLength = document.getElementsByName('fullname').length; > > We don't need this variable anymore, right? Yes you are right. I will remove it.

Created attachment 118617 [details] Updated patch Removed oldLength variable.

Comment on attachment 118617 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118617&action=review > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:35 > + document.getElementById('testElement').setAttribute('name', 'changed-name'); > + shouldBe("document.querySelector('" + tagName + "'); document.getElementsByName('fullname').length", "1"); Wait... what's the point of querySelector here? You should be doing document.querySelector('" + tagName + "').setAttribute('name', 'changed-name') here instead.

Comment on attachment 118617 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118617&action=review > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:27 > + element1 = createElement(tagName, { id: 'testElement', name: 'fullname'}); Once you do that, you don't even need to add id here.

(In reply to comment #30) > (From update of attachment 118617 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118617&action=review > > > LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:27 > > + element1 = createElement(tagName, { id: 'testElement', name: 'fullname'}); > > Once you do that, you don't even need to add id here. After doing this change test case for div is failing. PASS createTwoElements('div'); document.getElementsByName('fullname').length is 2 FAIL document.querySelector('div').setAttribute('name', 'changed-name'); document.getElementsByName('fullname').length should be 1. Was 2. I am not sure what could be the reason behind it. Also observed the same behavior in case of class attribute.

Comment on attachment 118617 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=118617&action=review >>> LayoutTests/fast/dom/getelementsbyname-invalidation-cache.html:27 >> >> Once you do that, you don't even need to add id here. > > After doing this change test case for div is failing. > PASS createTwoElements('div'); document.getElementsByName('fullname').length is 2 > FAIL document.querySelector('div').setAttribute('name', 'changed-name'); document.getElementsByName('fullname').length should be 1. Was 2. > > I am not sure what could be the reason behind it. Also observed the same behavior in case of class attribute. Is parseMappedAttribute called when you set the attribute before getElementsByName is called?

> > I am not sure what could be the reason behind it. Also observed the same behavior in case of class attribute. > > Is parseMappedAttribute called when you set the attribute before getElementsByName is called? Yes. parseMappedAttribute() is called when we set attribute before getElementsByName(). I analyzed further, if we don't use js-test-pre.js script then it is working as expected. It causing some issue when we use shouldBe() function or add <div id="console"></div> in the test page. I have tested this with a small page.

(In reply to comment #33) > Yes. parseMappedAttribute() is called when we set attribute before getElementsByName(). > I analyzed further, if we don't use js-test-pre.js script then it is working as expected. It causing some issue when we use shouldBe() function or add <div id="console"></div> in the test page. I have tested this with a small page. That seems odd. We probably have some bug there. We're probably not clearing cache properly in some edge cases.

Created attachment 118649 [details] Updated patch Incorporated review comments. Use section element instead of div, as js-test-pre.js inserts div#console in the test page, which is the main reason for failing div test case.

Comment on attachment 118649 [details] Updated patch Clearing flags on attachment: 118649 Committed r102511: <http://trac.webkit.org/changeset/102511>

All reviewed patches have been landed. Closing bug.

Someone really ought to follow this up with a patch that refactors so that these functions call through to the base class instead of having a copy of the logic from the base class in every derived class. As many as possible, at least.

(In reply to comment #38) > Someone really ought to follow this up with a patch that refactors so that these functions call through to the base class instead of having a copy of the logic from the base class in every derived class. As many as possible, at least. I was going through the code with Adam (Klein) this afternoon in regards to their mutation observer API and I think we've figured out the root of this bug. The issue appears to be that Attr::setValue isn't calling Attr:childrenChanged.