Bug 221243

Created attachment 418955 [details]
WIP

<rdar://problem/73719386>

Created attachment 419109 [details]
Patch

Comment on attachment 419109 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        it has a significant runtime and memory cost - the latter is because we never collect the JS wrappers of
> +        DOM nodes once they're created.

Surely it's not *never*?

> Source/WebCore/ChangeLog:21
> +        won't be observable by scripts in the future. The current heuristics is to check if the removed node

heuristic

> Source/WebCore/ChangeLog:24
> +        into existence, which increments the reference counnt. The two notable exceptions are StaticRange and

count

I think the explanation for why this approach is sufficient need not be tied to any statement about JS wrappers. The explanation is simply that "if there is no reference count, then there is no external reference" -- because those are the rules of reference counting.

> Source/WebCore/ChangeLog:39
> +        condition is refCount() > 2 because this function collects the list of child nodes to be removed in
> +        Vector<Ref<Node>> and the removeBetween loop itself stores it again in a RefPtr, the latter of which is

The refCount() > 2 comment probably belongs inside the function. That's a little subtle.

> Source/WebCore/dom/ContainerNode.cpp:116
> +    bool hasChildListChanged = false;
>      while (RefPtr<Node> child = m_firstChild) {
> +        if (i >= children.size() || children[i++].ptr() != child)
> +            hasChildListChanged = true;

I don't understand why we care if children have changed.

If a child has been removed, that is not an external reference to this subtree.

If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.

If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.

(If we do have some reason to care about children changing, then we need to think a little bit about the ABA case where children change, and then change back. Our hasChildListChanged would not be able to detect that case.)

Comment on attachment 419109 [details]
Patch

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

>> Source/WebCore/dom/ContainerNode.cpp:116
>> +            hasChildListChanged = true;
> 
> I don't understand why we care if children have changed.
> 
> If a child has been removed, that is not an external reference to this subtree.
> 
> If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.
> 
> If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.

I guess the issue here is that, if an event fired above, and then added a node to the subtree, we don't know what refCount check to do. Nodes in the children set should be checked for > 2, and other nodes should be checked for > 1, and we don't have an obvious way to tell the difference.

Comment on attachment 419109 [details]
Patch

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

r=me

>>> Source/WebCore/dom/ContainerNode.cpp:116
>>> +            hasChildListChanged = true;
>> 
>> I don't understand why we care if children have changed.
>> 
>> If a child has been removed, that is not an external reference to this subtree.
>> 
>> If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.
>> 
>> If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.
> 
> I guess the issue here is that, if an event fired above, and then added a node to the subtree, we don't know what refCount check to do. Nodes in the children set should be checked for > 2, and other nodes should be checked for > 1, and we don't have an obvious way to tell the difference.

^ I guess this is worth a comment next to hasChildListChanged; but I can't think of an obviously better alternative.

(In reply to Geoffrey Garen from comment #7)
> Comment on attachment 419109 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=419109&action=review
> 
> r=me
> 
> >>> Source/WebCore/dom/ContainerNode.cpp:116
> >>> +            hasChildListChanged = true;
> >> 
> >> I don't understand why we care if children have changed.
> >> 
> >> If a child has been removed, that is not an external reference to this subtree.
> >> 
> >> If a child has been added, that is not an external reference to this subtree unless the child has an external reference, which the refcount check will cover.
> >> 
> >> If a child has been moved, that is just a removal followed by an addition, so it is covered by the logic above.
> > 
> > I guess the issue here is that, if an event fired above, and then added a node to the subtree, we don't know what refCount check to do. Nodes in the children set should be checked for > 2, and other nodes should be checked for > 1, and we don't have an obvious way to tell the difference.
> 
> ^ I guess this is worth a comment next to hasChildListChanged; but I can't
> think of an obviously better alternative.

Turns out that we don't actually need this check after all.

Created attachment 419226 [details]
Patch

Created attachment 419228 [details]
Patch

Comment on attachment 419228 [details]
Patch

So much better! :P

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 419228 [details]
> Patch
> 
> So much better! :P

Indeed!

commit-queue failed to commit attachment 419228 [details] to WebKit repository. To retry, please set cq+ flag again.

Comment on attachment 419228 [details]
Patch

Clearing flags on attachment: 419228

Committed r272394: <https://trac.webkit.org/changeset/272394>

All reviewed patches have been landed.  Closing bug.