Bug 70810 - nameNodeListCache should be invalidated when name attribute changes/modified.
Summary: nameNodeListCache should be invalidated when name attribute changes/modified.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 74028
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-25 04:45 PDT by Arko Saha
Modified: 2011-12-12 14:57 PST (History)
10 users (show)

See Also:


Attachments
Simple test-page (490 bytes, text/html)
2011-10-25 04:46 PDT, Arko Saha
no flags Details
Patch (3.56 KB, patch)
2011-10-25 04:58 PDT, Arko Saha
darin: review-
Details | Formatted Diff | Diff
Simple test-page for class attr issue (397 bytes, text/html)
2011-10-28 06:28 PDT, Arko Saha
no flags Details
Updated patch (5.40 KB, patch)
2011-12-07 11:24 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (13.05 KB, patch)
2011-12-09 10:29 PST, Arko Saha
rniwa: review+
Details | Formatted Diff | Diff
Updated patch (13.94 KB, patch)
2011-12-09 12:43 PST, Arko Saha
no flags Details | Formatted Diff | Diff
Updated patch (13.86 KB, patch)
2011-12-09 12:51 PST, Arko Saha
rniwa: review-
rniwa: commit-queue-
Details | Formatted Diff | Diff
Updated patch (14.29 KB, patch)
2011-12-09 15:02 PST, Arko Saha
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arko Saha 2011-10-25 04:45:44 PDT
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!!!
Comment 1 Arko Saha 2011-10-25 04:46:40 PDT
Created attachment 112318 [details]
Simple test-page
Comment 2 Arko Saha 2011-10-25 04:58:27 PDT
Created attachment 112321 [details]
Patch
Comment 3 Alexey Proskuryakov 2011-10-25 11:06:41 PDT
Could you please check how this affects DOM performance on Dromaeo?
Comment 4 Arko Saha 2011-10-26 03:51:17 PDT
(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 5 Alexey Proskuryakov 2011-10-26 09:29:57 PDT
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?
Comment 6 Arko Saha 2011-10-26 12:59:38 PDT
(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.
Comment 7 Alexey Proskuryakov 2011-10-26 13:38:48 PDT
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.
Comment 8 Alexey Proskuryakov 2011-10-26 13:40:16 PDT
For reference, the code in Node::dispatchSubtreeModifiedEvent() was added in <http://trac.webkit.org/changeset/9990>.
Comment 9 Arko Saha 2011-10-28 06:26:18 PDT
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.
Comment 10 Arko Saha 2011-10-28 06:28:28 PDT
Created attachment 112854 [details]
Simple test-page for class attr issue
Comment 11 Arko Saha 2011-10-28 06:34:33 PDT
For reference, the code for sending DOMSubTreeMidified event from StyledElement::classAttributeChanged() was add in http://trac.webkit.org/changeset/37800
Comment 12 Darin Adler 2011-11-30 12:20:05 PST
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 13 Ryosuke Niwa 2011-12-06 17:06:30 PST
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.
Comment 14 Arko Saha 2011-12-07 11:24:37 PST
Created attachment 118239 [details]
Updated patch

Incorporating review comments.
Comment 15 Ryosuke Niwa 2011-12-07 13:54:03 PST
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 16 Darin Adler 2011-12-07 14:43:14 PST
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 17 Ryosuke Niwa 2011-12-07 18:23:26 PST
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.
Comment 18 Arko Saha 2011-12-09 10:29:27 PST
Created attachment 118594 [details]
Updated patch
Comment 19 Arko Saha 2011-12-09 10:32:46 PST
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 20 Ryosuke Niwa 2011-12-09 10:49:21 PST
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.
Comment 21 Arko Saha 2011-12-09 11:39:58 PST
(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.
Comment 22 Darin Adler 2011-12-09 11:44:56 PST
(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 23 Ryosuke Niwa 2011-12-09 12:11:31 PST
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.
Comment 24 Arko Saha 2011-12-09 12:34:26 PST
(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.
Comment 25 Arko Saha 2011-12-09 12:43:31 PST
Created attachment 118615 [details]
Updated patch

Incorporating review comments.
Comment 26 Ryosuke Niwa 2011-12-09 12:45:00 PST
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?
Comment 27 Arko Saha 2011-12-09 12:47:38 PST
(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.
Comment 28 Arko Saha 2011-12-09 12:51:06 PST
Created attachment 118617 [details]
Updated patch

Removed oldLength variable.
Comment 29 Ryosuke Niwa 2011-12-09 12:53:28 PST
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 30 Ryosuke Niwa 2011-12-09 12:54:22 PST
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.
Comment 31 Arko Saha 2011-12-09 13:19:39 PST
(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 32 Ryosuke Niwa 2011-12-09 13:40:03 PST
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?
Comment 33 Arko Saha 2011-12-09 14:16:29 PST
> > 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.
Comment 34 Ryosuke Niwa 2011-12-09 14:18:26 PST
(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.
Comment 35 Arko Saha 2011-12-09 15:02:35 PST
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 36 WebKit Review Bot 2011-12-09 18:30:07 PST
Comment on attachment 118649 [details]
Updated patch

Clearing flags on attachment: 118649

Committed r102511: <http://trac.webkit.org/changeset/102511>
Comment 37 WebKit Review Bot 2011-12-09 18:30:14 PST
All reviewed patches have been landed.  Closing bug.
Comment 38 Darin Adler 2011-12-12 14:54:40 PST
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.
Comment 39 Ryosuke Niwa 2011-12-12 14:57:23 PST
(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.