Bug 74028 - It's semantically incorrect to call notifyNodeListsAttributeChanged in dispatchSubtreeModifiedEvent
Summary: It's semantically incorrect to call notifyNodeListsAttributeChanged in dispat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords:
Depends on:
Blocks: 70810 73969
  Show dependency treegraph
 
Reported: 2011-12-07 15:06 PST by Ryosuke Niwa
Modified: 2011-12-08 21:27 PST (History)
7 users (show)

See Also:


Attachments
work in progress (8.33 KB, patch)
2011-12-07 16:13 PST, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
cleanup (10.96 KB, patch)
2011-12-07 18:06 PST, Ryosuke Niwa
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2011-12-07 15:06:10 PST
Right now we call dispatchSubtreeModifiedEvent in order to invalidate name node list. But this is semantically incorrect. We shouldn't invalidating this cache as a side effect of dispatching DOMSubtreeModified event.
Comment 1 Ryosuke Niwa 2011-12-07 16:12:15 PST
I'm hitting some obscure JS assertion failure/crash in this refactoring when I run DRT on LayoutTests/fast/dom/Attr/child-nodes-cache.html
Comment 2 Ryosuke Niwa 2011-12-07 16:13:01 PST
Created attachment 118287 [details]
work in progress
Comment 3 Ryosuke Niwa 2011-12-07 16:48:37 PST
    class DynamicNodeList : public NodeList {
    public:
        struct Caches : RefCounted<Caches> {
            static PassRefPtr<Caches> create();
            void reset();

            unsigned cachedLength;
            Node* lastItem; // of course, it's always a raw pointer that troubles us.
            unsigned lastItemOffset;
            bool isLengthCacheValid : 1;
            bool isItemCacheValid : 1;
        protected:
            Caches();
Comment 4 Ryosuke Niwa 2011-12-07 17:13:29 PST
It turns out that I can't even make them RefPtr due to the way node's life-cycle is managed :(
Comment 5 Ryosuke Niwa 2011-12-07 18:06:49 PST
Created attachment 118299 [details]
cleanup
Comment 6 Ryosuke Niwa 2011-12-07 18:27:53 PST
An obvious follow up patch would be make invalidateNodeListsCacheAfterAttributeChanges take bit flags or a qualified name so that we only invalidate node lists that are affected. e.g. when adding/removing class names, we don't have to invalidate label node list.
Comment 7 Ryosuke Niwa 2011-12-08 09:10:38 PST
Ping reviewers.
Comment 8 Ryosuke Niwa 2011-12-08 15:59:35 PST
Could someone review this patch? My other work to improve the performance, etc... is blocked on this patch.
Comment 9 Darin Adler 2011-12-08 17:53:13 PST
Comment on attachment 118299 [details]
cleanup

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

What’s the story on test coverage?

> Source/WebCore/dom/ContainerNode.cpp:846
> -        document()->nodeChildrenChanged(this);
> +        document()->updateRangesAfterNodeChanges(this);

It seems awkward to call changes to the children of a node “node changes”. The terminology here is getting less precise in a way that I think makes things more confusing.

> Source/WebCore/dom/ContainerNode.cpp:848
>      if (treeScope()->hasNodeListCaches())
> -        notifyNodeListsChildrenChanged();
> +        invalidateNodeListsCacheAfterNodeChanges();

Why does this need to be called in some cases where updateRangesAfterNodeChanges is not called?

> Source/WebCore/dom/Node.cpp:1041
> +        if (node->isAttributeNode())
> +            data->nodeLists()->invalidateCaches();
> +        else
> +            data->nodeLists()->invalidateCachesThatDependOnAttributes();

I think this needs a why comment. It’s quite confusing even if it’s backwards.
Comment 10 Ryosuke Niwa 2011-12-08 18:02:00 PST
Comment on attachment 118299 [details]
cleanup

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

>> Source/WebCore/dom/ContainerNode.cpp:846
>> +        document()->updateRangesAfterNodeChanges(this);
> 
> It seems awkward to call changes to the children of a node “node changes”. The terminology here is getting less precise in a way that I think makes things more confusing.

Will change to updateRangesAfterChildrenChanged then.

>> Source/WebCore/dom/ContainerNode.cpp:848
>> +        invalidateNodeListsCacheAfterNodeChanges();
> 
> Why does this need to be called in some cases where updateRangesAfterNodeChanges is not called?

That's actually a good point. Will test & make a change if appropriate.

>> Source/WebCore/dom/Node.cpp:1041
>> +            data->nodeLists()->invalidateCachesThatDependOnAttributes();
> 
> I think this needs a why comment. It’s quite confusing even if it’s backwards.

Okay will do.
Comment 11 Ryosuke Niwa 2011-12-08 18:03:55 PST
Thanks for the review!

(In reply to comment #9)
> (From update of attachment 118299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118299&action=review
> 
> What’s the story on test coverage?

There are quite few tests in LayoutTests/fast/dom. e.g. LayoutTests/fast/dom/Attr/child-nodes-cache.html Also, a lot of tests use childNodes, getElementById, etc... so this code is probably sufficiently exercised.
Comment 12 Ryosuke Niwa 2011-12-08 18:52:42 PST
(In reply to comment #10)
> (From update of attachment 118299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118299&action=review
>
> >> Source/WebCore/dom/ContainerNode.cpp:848
> >> +        invalidateNodeListsCacheAfterNodeChanges();
> > 
> > Why does this need to be called in some cases where updateRangesAfterNodeChanges is not called?
> 
> That's actually a good point. Will test & make a change if appropriate.

Turns out this is tested by fast/dom/NodeList/invalidate-node-lists-when-parsing.html

Now I wonder what makes Range special so that we don't have to update upon changes made by the parser...
Comment 13 Ryosuke Niwa 2011-12-08 21:27:02 PST
Committed r102431: <http://trac.webkit.org/changeset/102431>