Bug 74028 – It's semantically incorrect to call notifyNodeListsAttributeChanged in dispatchSubtreeModifiedEvent

• Component: DOM
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 118287 [details]
work in progress

[Ryosuke Niwa]

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

[Ryosuke Niwa]

It turns out that I can't even make them RefPtr due to the way node's life-cycle is managed :(

[Ryosuke Niwa]

Created attachment 118299 [details]
cleanup

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Ping reviewers.

[Ryosuke Niwa]

Could someone review this patch? My other work to improve the performance, etc... is blocked on this patch.

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Committed r102431: <http://trac.webkit.org/changeset/102431>