[Webkit-unassigned] [Bug 222720] REGRESSION(r272900): Nullptr crash in ComposedTreeIterator::traverseNextInShadowTree() via ShadowRoot::hostChildElementDidChange

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=222720

--- Comment #28 from Carlos Garcia Campos <cgarcia at igalia.com> ---
(In reply to Darin Adler from comment #25)
> Comment on attachment 423315 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=423315&action=review
> 
> > Source/WebCore/dom/ContainerNode.cpp:131
> > +            // Ensure we keep all removed children in the children Vector. There might be new nodes added
> > +            // while dispatching other child removal events, after the initial collection.
> > +            if (i < children.size())
> > +                children[i] = *child;
> > +            else
> > +                children.append(*child);
> > +            i++;
> 
> I’m not sure this code is right. The comment is subtly different from what
> the code does.
> 
> What it does, when source == ChildChange::Source::API, is completely
> re-create the children vector, overwriting what’s already there. Except that
> if the size of the vector happens to be higher than the number of children,
> it keeps any extras that were there.

It's confusing because I tried to make it efficiently. The problem is that children vector doesn't contain all the children at this point, but the while loop is iterating all the children. This can happen when a new node is added to the container in a removal event callback. Those new nodes added are already in the container and will be removed too, but they are not part of the children vector that collected the children before emitting the removal events. What we want here is to protect the nodes that are being removed at least until the end of the function. That will prevent for sure that only one of the slot assigned nodes becomes nullptr earlier. There are other cleaner ways of doing this:

a) Collect children again before the while loop to build a new children vector. That would mean iterating all the children one more time.

b) Inside the while loop we could check if every child is in the children node and add it to the vector if not present. This is even worse, since we need to iterate the vector to find the current child on every loop iteration.

c) Create a new vector, something like a protected children vector and add all children to it in the while loop. This doesn't iterate again, it just requires a bit more memory. Probably not a big deal for most of the cases.

So, what I did was c) but reusing the children vector. Note that the order of children in the vector doesn't realy matter in this case, because this is only done for the cases in which the returned vector is not used.

> I am not sure that it keeps "new nodes added"; it keeps nodes at the end of
> the vector, but I don’t see what guarantees those are only "new nodes added".

They are not only the new nodes, it just ensures that all nodes that are going to be removed are kept in the vector. Currently the children vector is only "protecting" the children collected at the beginning.

> I’m not sure that it "keeps all removed children". If they are not at the
> end of the vector, then they will be overwritten.

It doesn't really matter if nodes are overritten.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210317/55a7f043/attachment-0001.htm>