Bug 70810 – nameNodeListCache should be invalidated when name attribute changes/modified.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Arko Saha]

m_nameNodeListCache is not getting invalidated when name attribute of an element changes/modified.
Attached test page for the issue:

actual result: 
This is a simple div-1
This is a simple div-2
FAIL!!! 

expected result:
This is a simple div-1
This is a simple div-2
PASS!!!

[Arko Saha]

Created attachment 112318 [details]
Simple test-page

[Arko Saha]

Created attachment 112321 [details]
Patch

[Alexey Proskuryakov]

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

[Arko Saha]

(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)

[Alexey Proskuryakov]

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?

[Arko Saha]

(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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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

[Arko Saha]

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.

[Arko Saha]

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

[Arko Saha]

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

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Arko Saha]

Created attachment 118239 [details]
Updated patch

Incorporating review comments.

[Ryosuke Niwa]

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.

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Arko Saha]

Created attachment 118594 [details]
Updated patch

[Arko Saha]

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.

[Ryosuke Niwa]

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.

[Arko Saha]

(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.

[Darin Adler]

(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.

[Ryosuke Niwa]

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.

[Arko Saha]

(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.

[Arko Saha]

Created attachment 118615 [details]
Updated patch

Incorporating review comments.

[Ryosuke Niwa]

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?

[Arko Saha]

(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.

[Arko Saha]

Created attachment 118617 [details]
Updated patch

Removed oldLength variable.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Arko Saha]

(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.

[Ryosuke Niwa]

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?

[Arko Saha]

> > 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.

[Ryosuke Niwa]

(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.

[Arko Saha]

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.

[WebKit Review Bot]

Comment on attachment 118649 [details]
Updated patch

Clearing flags on attachment: 118649

Committed r102511: <http://trac.webkit.org/changeset/102511>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

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.

[Ryosuke Niwa]

(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.