Bug 183723 – Disconnect the SVGPathSegList items from their SVGPathElement before rebuilding a new list

• Component: New Bugs
• Status: RESOLVED FIXED

[Dean Jackson]

Allow reuse of a SVGPathSeg after the path's d attribute has been reset

[Dean Jackson]

<rdar://problem/38517871>

[Dean Jackson]

Created attachment 336012 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 336012 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        the path, but the path couldn't find it.

I do not think this is the right approach to fix this bug. After changing the d poverty of the SVGPathElement, the relationship between the old SVGPathSeg and the animated tear off should be removed. The change here is accepting this bug because the assertion is removed and it is adding the new assumption: If the SVGPathSeg is linked to an animated property tear off but the animated property tear off does not have a link back to SVGPathSeg, then this is okay. 

I think we need to break the relationship between the old SVGPathSeg and its animated tear off if the animated tear off is going to delete it from its list. How about adding this code to SVGPathElement::svgAttributeChanged()

            buildSVGPathSegListValuesFromByteStream(m_pathByteStream, *this, newList, UnalteredParsing);
            for (auto& pathSeg :  m_pathSegList.value) {
                SVGPathSegWithContext* itemWithContext = static_cast<SVGPathSegWithContext*>(pathSeg.get());
                itemWithContext->setContextAndRole(this, PathSegUndefinedRole);
            }
            m_pathSegList.value = WTFMove(newList);

This should break the relationship between the old SVGPathSeg and the animated tear off object. When SVGPathSegList::processIncomingListItemValue() is called to reinsert the same SVGPathSeg, it will make an early return because if (!animatedPropertyOfItem) is true. When adding the SVGPathSeg the first time, SVGPathSegList::processIncomingListItemValue() makes the same early return because the role of the SVGPathSeg is not set when it calls newItemWithContext->animatedProperty().

[Said Abou-Hallawa]

Created attachment 336081 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 336081 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        segments should be disconnected from the path element. We already do 

"should be" is ambiguous: it could mean "they are not disconnected but they need to be", or "if things are working correctly they will be". Maybe say "they need to get disconnected".

[Said Abou-Hallawa]

Created attachment 336134 [details]
Patch

[Daniel Bates]

Comment on attachment 336134 [details]
Patch

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

> Source/WebCore/svg/SVGPathElement.cpp:262
> +            m_pathSegList.value.clearContextAndRoles();
> +            m_pathSegList.value = WTFMove(newList);

Does it ever make sense to copy-construct, copy-assign, move-assign, or move construct a SVGPathSegListValues and not clear the context and roles of all its existing elements before copying or moving the new elements into it? If it never makes sense then I suggest we override these operators of SVGPathSegListValues to do this for us.

> Source/WebCore/svg/SVGPathSegList.cpp:3
> + * Copyright (C) 2018 Apple Inc.  All rights reserved.

There should only be one space character after the '.' and before the "All".

> Source/WebCore/svg/SVGPathSegListValues.cpp:6
> + * Copyright (C) 2018 Apple Inc.  All rights reserved.

There should only be one space character after the '.' and before the "All".

> Source/WebCore/svg/SVGPathSegListValues.cpp:49
> +    ASSERT(item);
> +    static_cast<SVGPathSegWithContext*>(item.get())->setContextAndRole(nullptr, PathSegUndefinedRole);

Notice that RefPtr::operator*() asserts that the contained pointer is non-null before dereferencing it and returning a reference. We can take advantage of this to simplify this code to read:

static_cast<SVGPathSegWithContext&>(*item).setContextAndRole(nullptr, PathSegUndefinedRole);

> Source/WebCore/svg/SVGPathSegListValues.cpp:56
> +    for (unsigned limit = size(), index = 0; index < limit; ++index)
> +        clearItemContextAndRole(index);
> +}

As far as I can tell it is unnecessary to clear the context and role of each item in the list in order. We can take advantage of this fact to remove a local variable and simplify this code to read:

auto count = size();
while (count--)
    clearItemContextAndRole(count);

> Source/WebCore/svg/SVGPathSegListValues.h:3
> + * Copyright (C) 2018 Apple Inc.  All rights reserved.

There should only be one space character after the '.' and before the "All".

> LayoutTests/svg/dom/reuse-pathseg-after-changing-d.html:1
> +<script>

I do not see the need for this test to use quirks mode. Please add a <!DOCTYPE html> at the top of this file.

[zalan]

Comment on attachment 336134 [details]
Patch

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

> Source/WebCore/svg/SVGPathSegListValues.cpp:46
> +    ASSERT(index < size());

This is redundant. I am sure Vector<> asserts for out-of-bound.

>> Source/WebCore/svg/SVGPathSegListValues.cpp:56
>> +}
> 
> As far as I can tell it is unnecessary to clear the context and role of each item in the list in order. We can take advantage of this fact to remove a local variable and simplify this code to read:
> 
> auto count = size();
> while (count--)
>     clearItemContextAndRole(count);

Absolutely. That for loop is confusing.

[Dean Jackson]

Comment on attachment 336012 [details]
Patch

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

>> Source/WebCore/ChangeLog:15
>> +        the path, but the path couldn't find it.
> 
> I do not think this is the right approach to fix this bug. After changing the d poverty of the SVGPathElement, the relationship between the old SVGPathSeg and the animated tear off should be removed. The change here is accepting this bug because the assertion is removed and it is adding the new assumption: If the SVGPathSeg is linked to an animated property tear off but the animated property tear off does not have a link back to SVGPathSeg, then this is okay. 
> 
> I think we need to break the relationship between the old SVGPathSeg and its animated tear off if the animated tear off is going to delete it from its list. How about adding this code to SVGPathElement::svgAttributeChanged()
> 
>             buildSVGPathSegListValuesFromByteStream(m_pathByteStream, *this, newList, UnalteredParsing);
>             for (auto& pathSeg :  m_pathSegList.value) {
>                 SVGPathSegWithContext* itemWithContext = static_cast<SVGPathSegWithContext*>(pathSeg.get());
>                 itemWithContext->setContextAndRole(this, PathSegUndefinedRole);
>             }
>             m_pathSegList.value = WTFMove(newList);
> 
> This should break the relationship between the old SVGPathSeg and the animated tear off object. When SVGPathSegList::processIncomingListItemValue() is called to reinsert the same SVGPathSeg, it will make an early return because if (!animatedPropertyOfItem) is true. When adding the SVGPathSeg the first time, SVGPathSegList::processIncomingListItemValue() makes the same early return because the role of the SVGPathSeg is not set when it calls newItemWithContext->animatedProperty().

This is what I originally implemented but then I realised it wasn't necessary (unless I missed something).

The SVGPathSeg has a weak pointer back to the path, so it's not holding ownership. The DOM-side interface does not reference the path, or the pathseg, so that's ok too. The only thing keeping the SVGPathSeg alive is the JS reference. The Path keeps the same SVGPathSegList object, and just changes its underlying values.

I looked through the interfaces and the only place where we use the back reference is here, when we are trying to insert the SVGPathSeg into another path (or in this case, the same path).

[Said Abou-Hallawa]

Created attachment 336152 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 336134 [details]
Patch

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

>> Source/WebCore/svg/SVGPathElement.cpp:262
>> +            m_pathSegList.value = WTFMove(newList);
> 
> Does it ever make sense to copy-construct, copy-assign, move-assign, or move construct a SVGPathSegListValues and not clear the context and roles of all its existing elements before copying or moving the new elements into it? If it never makes sense then I suggest we override these operators of SVGPathSegListValues to do this for us.

I thought about but this change I did not want to do it because of two reasons:

(1) The change is more involving. I have to override the copy and move assignments. I have to define the copy the move contractor since the copy and move assignments will delete them. I have to override the clear() method as well.
(2) Calling clearContextAndRoles() from the copy and move assignments does not make the solution complete. clearItemContextAndRole() has to still be called from SVGPathSegList::replaceItem() and SVGPathSegList::removeItem(). There is nothing I can override to call clearItemContextAndRole() implicitly.

But the suggested change is done anyway although I do not like it much.

>> Source/WebCore/svg/SVGPathSegList.cpp:3
>> + * Copyright (C) 2018 Apple Inc.  All rights reserved.
> 
> There should only be one space character after the '.' and before the "All".

Done.

>> Source/WebCore/svg/SVGPathSegListValues.cpp:6
>> + * Copyright (C) 2018 Apple Inc.  All rights reserved.
> 
> There should only be one space character after the '.' and before the "All".

Done.

>> Source/WebCore/svg/SVGPathSegListValues.cpp:46
>> +    ASSERT(index < size());
> 
> This is redundant. I am sure Vector<> asserts for out-of-bound.

The assertion is removed since Vector::at() makes the same assertion.

>> Source/WebCore/svg/SVGPathSegListValues.cpp:49
>> +    static_cast<SVGPathSegWithContext*>(item.get())->setContextAndRole(nullptr, PathSegUndefinedRole);
> 
> Notice that RefPtr::operator*() asserts that the contained pointer is non-null before dereferencing it and returning a reference. We can take advantage of this to simplify this code to read:
> 
> static_cast<SVGPathSegWithContext&>(*item).setContextAndRole(nullptr, PathSegUndefinedRole);

Done.

>>> Source/WebCore/svg/SVGPathSegListValues.cpp:56
>>> +}
>> 
>> As far as I can tell it is unnecessary to clear the context and role of each item in the list in order. We can take advantage of this fact to remove a local variable and simplify this code to read:
>> 
>> auto count = size();
>> while (count--)
>>     clearItemContextAndRole(count);
> 
> Absolutely. That for loop is confusing.

Done. It is true that the while-loop removes the local variable. But I don't think the for-loop was confusing :).

>> Source/WebCore/svg/SVGPathSegListValues.h:3
>> + * Copyright (C) 2018 Apple Inc.  All rights reserved.
> 
> There should only be one space character after the '.' and before the "All".

Done.

>> LayoutTests/svg/dom/reuse-pathseg-after-changing-d.html:1
>> +<script>
> 
> I do not see the need for this test to use quirks mode. Please add a <!DOCTYPE html> at the top of this file.

Done.

[Said Abou-Hallawa]

(In reply to Dean Jackson from comment #9)
> Comment on attachment 336012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336012&action=review
> 
> >> Source/WebCore/ChangeLog:15
> >> +        the path, but the path couldn't find it.
> > 
> > I do not think this is the right approach to fix this bug. After changing the d poverty of the SVGPathElement, the relationship between the old SVGPathSeg and the animated tear off should be removed. The change here is accepting this bug because the assertion is removed and it is adding the new assumption: If the SVGPathSeg is linked to an animated property tear off but the animated property tear off does not have a link back to SVGPathSeg, then this is okay. 
> > 
> > I think we need to break the relationship between the old SVGPathSeg and its animated tear off if the animated tear off is going to delete it from its list. How about adding this code to SVGPathElement::svgAttributeChanged()
> > 
> >             buildSVGPathSegListValuesFromByteStream(m_pathByteStream, *this, newList, UnalteredParsing);
> >             for (auto& pathSeg :  m_pathSegList.value) {
> >                 SVGPathSegWithContext* itemWithContext = static_cast<SVGPathSegWithContext*>(pathSeg.get());
> >                 itemWithContext->setContextAndRole(this, PathSegUndefinedRole);
> >             }
> >             m_pathSegList.value = WTFMove(newList);
> > 
> > This should break the relationship between the old SVGPathSeg and the animated tear off object. When SVGPathSegList::processIncomingListItemValue() is called to reinsert the same SVGPathSeg, it will make an early return because if (!animatedPropertyOfItem) is true. When adding the SVGPathSeg the first time, SVGPathSegList::processIncomingListItemValue() makes the same early return because the role of the SVGPathSeg is not set when it calls newItemWithContext->animatedProperty().
> 
> This is what I originally implemented but then I realised it wasn't
> necessary (unless I missed something).
> 
> The SVGPathSeg has a weak pointer back to the path, so it's not holding
> ownership. The DOM-side interface does not reference the path, or the
> pathseg, so that's ok too. The only thing keeping the SVGPathSeg alive is
> the JS reference. The Path keeps the same SVGPathSegList object, and just
> changes its underlying values.
> 
> I looked through the interfaces and the only place where we use the back
> reference is here, when we are trying to insert the SVGPathSeg into another
> path (or in this case, the same path).

Two things I noticed:

-- SVGPathSegList::replaceItem() and SVGPathSegList::removeItem() clears the context and the role of the replaced/removed item. SVGPathSegList::clear() clears the context and the roles of all the removed items. So I think it makes sense to do the same thing when the whole list is replaced by another list as a the result of changing the 'd' attribute of the path element.

-- When SVGPathSegList::processIncomingListItemValue() is called the first time for adding an SVGPathSeg, it makes an early return because SVGPathSegWithContext::animatedProperty() returns nullptr. The reason for having a nullptr animatedProperty() is the context and the role is not set. But before SVGPathSegList::processIncomingListItemValue() returns, it sets the context and the role. If SVGPathSegWithContext::animatedProperty() is called after that point, it will return the wrapper cache lookup result. If this SVGPathSeg is removed from the SVGPathSegList then re-added, its state has to be reset such that SVGPathSegList::processIncomingListItemValue() should behave the same way when it is first added.

[Said Abou-Hallawa]

Created attachment 336222 [details]
Patch

[Daniel Bates]

Comment on attachment 336222 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 336222 [details]
Patch

Clearing flags on attachment: 336222

Committed r229830: <https://trac.webkit.org/changeset/229830>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.