Bug 148472 – Remove all uses of PassRefPtr in WebCore/svg

• Component: New Bugs
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 259940 [details]
Patch

[Gyuyoung Kim]

Created attachment 259944 [details]
Patch

[Daniel Bates]

Comment on attachment 259944 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:1874
> +    if (!targetNode->EventTarget::addEventListener(eventType, WTF::move(listener), useCapture))

This is not correct because we reference listener later in this function (line 1892) and listener will be nullified on return of the implicitly invoked RefPtr move constructor. We should use listener.copyRef() here.

> Source/WebCore/html/ImageDocument.cpp:242
> +            window->addEventListener("resize", WTF::move(listener), false);

Similarly, we should use listener.copyRef() here.

> Source/WebCore/html/shadow/MediaControlsApple.cpp:512
> +    m_closedCaptionsContainer->addEventListener(eventNames().wheelEvent, WTF::move(listener), true);

Ditto.

> Source/WebCore/html/shadow/MediaControlsApple.cpp:519
> +    document().addEventListener(eventNames().clickEvent, WTF::move(listener), true);

Ditto.

> Source/WebCore/html/shadow/MediaControlsApple.cpp:520
> +    addEventListener(eventNames().clickEvent, WTF::move(listener), true);

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1697
> +    if (!EventTarget::addEventListener(eventType, WTF::move(listener), useCapture))

We may want to consider using listener.copyRef() here to make using listener after this line less error prone given the length of this function.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:90
> +        builder->appendEffectToEffectReferences(WTF::move(effect), element.renderer());

This is not correct since we dereference effect below. We should use effect.copyRef() here.

> Source/WebCore/svg/SVGElement.cpp:579
> +    if (!Node::addEventListener(eventType, WTF::move(listener), useCapture))

Similarly, we should use listener.copyRef() here.

> Source/WebCore/svg/SVGElement.cpp:589
> +        bool result = instance->Node::addEventListener(eventType, WTF::move(listener), useCapture);

Although not strictly necessary, I suggest we use listener.copyRef() here as well.

> Source/WebCore/svg/SVGPathElement.cpp:362
> +            appendSVGPathByteStreamFromSVGPathSeg(WTF::move(m_pathSegList.value.last()), m_pathByteStream.get(), UnalteredParsing);

We should use copyRef() here.

> Source/WebCore/svg/SVGTRefElement.cpp:252
> +        m_targetListener->attach(WTF::move(target));

This is not correct and will cause target.get() to return nullptr on line 254 because target will be nullified on return of the implicitly invoked RefPtr move constructor. We should use target.copyRef().

[Gyuyoung Kim]

Comment on attachment 259944 [details]
Patch

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

>> Source/WebCore/dom/Node.cpp:1874
>> +    if (!targetNode->EventTarget::addEventListener(eventType, WTF::move(listener), useCapture))
> 
> This is not correct because we reference listener later in this function (line 1892) and listener will be nullified on return of the implicitly invoked RefPtr move constructor. We should use listener.copyRef() here.

Nice point out ! Fixed. Let me take care of this in upcoming patch.

>> Source/WebCore/html/ImageDocument.cpp:242
>> +            window->addEventListener("resize", WTF::move(listener), false);
> 
> Similarly, we should use listener.copyRef() here.

done.

>> Source/WebCore/html/shadow/MediaControlsApple.cpp:512
>> +    m_closedCaptionsContainer->addEventListener(eventNames().wheelEvent, WTF::move(listener), true);
> 
> Ditto.

done.

>> Source/WebCore/html/shadow/MediaControlsApple.cpp:520
>> +    addEventListener(eventNames().clickEvent, WTF::move(listener), true);
> 
> Ditto.

This line is last use of *listener*. Should we use copyRef() here as well ?

>> Source/WebCore/svg/SVGElement.cpp:589
>> +        bool result = instance->Node::addEventListener(eventType, WTF::move(listener), useCapture);
> 
> Although not strictly necessary, I suggest we use listener.copyRef() here as well.

*listener* is used in this function lastly. Isn't it WTF::move() better ?

>> Source/WebCore/svg/SVGPathElement.cpp:362
>> +            appendSVGPathByteStreamFromSVGPathSeg(WTF::move(m_pathSegList.value.last()), m_pathByteStream.get(), UnalteredParsing);
> 
> We should use copyRef() here.

done.

>> Source/WebCore/svg/SVGTRefElement.cpp:252
>> +        m_targetListener->attach(WTF::move(target));
> 
> This is not correct and will cause target.get() to return nullptr on line 254 because target will be nullified on return of the implicitly invoked RefPtr move constructor. We should use target.copyRef().

done.

[Gyuyoung Kim]

Created attachment 260034 [details]
Patch

[Gyuyoung Kim]

dydz, could you take a look again ?

[Darin Adler]

Comment on attachment 260034 [details]
Patch

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

> Source/WebCore/dom/EventListenerMap.cpp:190
> +        target->addEventListener(eventType, WTF::move((*listenerVector)[i].listener), (*listenerVector)[i].useCapture);

I’m concerned about this. Is this already how the code behaved, or is this new behavior? I don’t think a behavior change should be mixed in with the rest of the patch. At first this should be copyRef and then we could use WTF::move to optimize and think through whether that’s OK.

> Source/WebCore/dom/EventTarget.cpp:79
>      return ensureEventTargetData().eventListenerMap.add(eventType, listener, useCapture);

Do we need WTF::move here to avoid reference count churn?

> Source/WebCore/dom/Node.cpp:1910
>      return tryAddEventListener(this, eventType, listener, useCapture);

Do we need WTF::move here to avoid reference count churn?

> Source/WebCore/html/HTMLMediaElement.cpp:5045
> +bool HTMLMediaElement::addEventListener(const AtomicString& eventType, RefPtr<EventListener>&& listener, bool useCapture)

Do we need to add some WTF::move to this function to avoid reference count churn?

> Source/WebCore/svg/SVGPathUtilities.cpp:142
> +bool appendSVGPathByteStreamFromSVGPathSeg(RefPtr<SVGPathSeg>&& pathSeg, SVGPathByteStream* result, PathParsingMode parsingMode)

I think we need WTF::move below on the call to append to avoid adding reference count churn.

> Source/WebCore/svg/SVGTRefElement.cpp:89
> +void SVGTRefTargetEventListener::attach(RefPtr<Element>&& target)

Don’t we need WTF::move on the assignment to m_target below?

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:67
>      RefPtr<FilterEffect> effect = prpEffect;

We need WTF::move here. Also seems this function is not taking advantage of the fact that we can take ownership of the effect passed in.

It’s not good to leave things named with a “prp” prefix when they are not using PassRefPtr.

[Gyuyoung Kim]

Created attachment 260399 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #7)
> Comment on attachment 260034 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260034&action=review
> 
> > Source/WebCore/dom/EventListenerMap.cpp:190
> > +        target->addEventListener(eventType, WTF::move((*listenerVector)[i].listener), (*listenerVector)[i].useCapture);
> 
> I’m concerned about this. Is this already how the code behaved, or is this
> new behavior? I don’t think a behavior change should be mixed in with the
> rest of the patch. At first this should be copyRef and then we could use
> WTF::move to optimize and think through whether that’s OK.
> 
> > Source/WebCore/dom/EventTarget.cpp:79
> >      return ensureEventTargetData().eventListenerMap.add(eventType, listener, useCapture);
> 
> Do we need WTF::move here to avoid reference count churn?
> 
> > Source/WebCore/dom/Node.cpp:1910
> >      return tryAddEventListener(this, eventType, listener, useCapture);
> 
> Do we need WTF::move here to avoid reference count churn?
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:5045
> > +bool HTMLMediaElement::addEventListener(const AtomicString& eventType, RefPtr<EventListener>&& listener, bool useCapture)
> 
> Do we need to add some WTF::move to this function to avoid reference count
> churn?
> 
> > Source/WebCore/svg/SVGPathUtilities.cpp:142
> > +bool appendSVGPathByteStreamFromSVGPathSeg(RefPtr<SVGPathSeg>&& pathSeg, SVGPathByteStream* result, PathParsingMode parsingMode)
> 
> I think we need WTF::move below on the call to append to avoid adding
> reference count churn.
> 
> > Source/WebCore/svg/SVGTRefElement.cpp:89
> > +void SVGTRefTargetEventListener::attach(RefPtr<Element>&& target)
> 
> Don’t we need WTF::move on the assignment to m_target below?
> 
> > Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:67
> >      RefPtr<FilterEffect> effect = prpEffect;
> 
> We need WTF::move here. Also seems this function is not taking advantage of
> the fact that we can take ownership of the effect passed in.
> 
> It’s not good to leave things named with a “prp” prefix when they are not
> using PassRefPtr.

Because this is first patch to reduce uses of PassRefPtr in argument, I had unclear understanding in some cases. If there is still wrong changes, please let me know.

[Darin Adler]

(In reply to comment #9)
> Because this is first patch to reduce uses of PassRefPtr in argument, I had
> unclear understanding in some cases. If there is still wrong changes, please
> let me know.

I won’t be able to answer all the questions from just reading the code. We may have to actually test things to make sure we are choosing the correct idioms before we make changes across the entire project!

[Darin Adler]

Comment on attachment 260399 [details]
Patch

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

Some initial comments.

> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:216
> +    bool success = AudioNode::addEventListener(eventType, listener.copyRef(), useCapture);

This is not quite right. The old code passed the listener in without churning the reference count; that’s what passing a PassRefPtr to another function that also takes a PassRefPtr does. But the new code is doing a copyRef; the whole point of taking RefPtr&& is to be able to move the thing in and take ownership without churning the reference count. There are two ways to do this:

1) Use Ref&& or RefPtr&& so we can avoid churning the reference count.
2) Use & or * so the caller doesn’t have to already hold the thing in a smart pointer.

But we should never take a Ref&& or RefPtr&& and then decide to unconditionally do a copyRef. That’s wasteful and even slightly misleading.

I’m pretty sure we need to use WTF::move, not copyRef, here. Or maybe we can pass without an explicit WTF::move. I’m not sure about that. What I do know is that we want the compiled code for this function to not change there reference count of listener at all.

> Source/WebCore/Modules/webaudio/ScriptProcessorNode.cpp:278
> +    bool success = AudioNode::addEventListener(eventType, listener.copyRef(), useCapture);

Same thing.

> Source/WebCore/html/HTMLMediaElement.cpp:5051
>          return Node::addEventListener(eventType, listener, useCapture);

Will this do the right thing without a call to WTF::move or not? We need to know before we land this change.

[Gyuyoung Kim]

Comment on attachment 260399 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/AudioScheduledSourceNode.cpp:216
>> +    bool success = AudioNode::addEventListener(eventType, listener.copyRef(), useCapture);
> 
> This is not quite right. The old code passed the listener in without churning the reference count; that’s what passing a PassRefPtr to another function that also takes a PassRefPtr does. But the new code is doing a copyRef; the whole point of taking RefPtr&& is to be able to move the thing in and take ownership without churning the reference count. There are two ways to do this:
> 
> 1) Use Ref&& or RefPtr&& so we can avoid churning the reference count.
> 2) Use & or * so the caller doesn’t have to already hold the thing in a smart pointer.
> 
> But we should never take a Ref&& or RefPtr&& and then decide to unconditionally do a copyRef. That’s wasteful and even slightly misleading.
> 
> I’m pretty sure we need to use WTF::move, not copyRef, here. Or maybe we can pass without an explicit WTF::move. I’m not sure about that. What I do know is that we want the compiled code for this function to not change there reference count of listener at all.

Now I pretty feel that we should very take care of purging PassRefPtr in argument. To my understanding we should use copyRef() when the argument is used in multiple lines. If the argument is just passed to a new function or code calling RefPtr::release(), we need to pass the ownership to new function using WTF::move(),

for example,

void foo(RefPtr<EventListener>&& listener)
{
    aVar(listener.copyRef());
    ...
    bVar(listener.copyRef()); => I wonder what should we use in last use of function. copyRef() or WTF::move() ?
}


However we have to move ownership of the argument when function just passes the argument to new function even when RefPtr::release() is not used as below, right ?

void foo(RefPtr<EventListener>&& listener)
{
    aVar(WTF::move(listener));
}

[Darin Adler]

(In reply to comment #12)
>     bVar(listener.copyRef()); => I wonder what should we use in last use of
> function. copyRef() or WTF::move() ?

Should use WTF::move, I think.

> However we have to move ownership of the argument when function just passes
> the argument to new function even when RefPtr::release() is not used as
> below, right ?
> 
> void foo(RefPtr<EventListener>&& listener)
> {
>     aVar(WTF::move(listener));
> }

I believe so, but I am not sure; might work without explicit WTF::move. Best way to find out would be to write some unit tests.

[Gyuyoung Kim]

Created attachment 260475 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 260475 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:5054
> +    if (!Node::addEventListener(eventType, listener.copyRef(), useCapture))

I wonder what should we use here. Because here is last use of *listener*.

> Source/WebCore/html/ImageDocument.cpp:243
> +        imageElement->addEventListener("click", WTF::move(listener), false);

I believe we have to use WTF::move() since listener is released in original code.

> Source/WebCore/html/shadow/MediaControlsApple.cpp:520
> +    addEventListener(eventNames().clickEvent, listener.copyRef(), true);

ditto. Last use.

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:94
> +        builder->add(element.result(), WTF::move(effect));

.release() should be replaced with WTF::move() in order to pass an ownership.

> Source/WebCore/svg/SVGPathUtilities.cpp:149
> +    appendedItemList.append(pathSeg.copyRef());

Should we pass ownership of *pathSeg* when appending RefPtr&& ?

> Source/WebCore/svg/SVGTRefElement.cpp:97
> +    m_target = WTF::move(target);

I'm not sure if we have to use WTF::move(). Isn't copyRef() correct ?

[Gyuyoung Kim]

Created attachment 260480 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #13)
> (In reply to comment #12)
> >     bVar(listener.copyRef()); => I wonder what should we use in last use of
> > function. copyRef() or WTF::move() ?
> 
> Should use WTF::move, I think.
> 
> > However we have to move ownership of the argument when function just passes
> > the argument to new function even when RefPtr::release() is not used as
> > below, right ?
> > 
> > void foo(RefPtr<EventListener>&& listener)
> > {
> >     aVar(WTF::move(listener));
> > }
> 
> I believe so, but I am not sure; might work without explicit WTF::move. Best
> way to find out would be to write some unit tests.

Ok, let me prepare some unit tests for this issue soon.

[Darin Adler]

Comment on attachment 260480 [details]
Patch

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

Looks like we missed something in Node.cpp according to iOS. Looks generally good but please consider my comments.

> Source/WebCore/dom/EventListenerMap.cpp:190
> +        target->addEventListener(eventType, ((*listenerVector)[i].listener).copyRef(), (*listenerVector)[i].useCapture);

I don’t think we need those extra parentheses.

> Source/WebCore/html/HTMLMediaElement.cpp:5051
> +        return Node::addEventListener(eventType, listener.copyRef(), useCapture);

This should be WTF::move, not copyRef.

> Source/WebCore/page/DOMWindow.cpp:1697
> +    if (!EventTarget::addEventListener(eventType, listener.copyRef(), useCapture))

This should be WTF::move, not copyRef. None of the code below uses listener.

> Source/WebCore/svg/SVGElement.cpp:574
> +bool SVGElement::addEventListener(const AtomicString& eventType, RefPtr<EventListener>&& prpListener, bool useCapture)

This could just be named listener. No need for the prpListener name.

> Source/WebCore/svg/SVGElement.cpp:576
>      RefPtr<EventListener> listener = prpListener;

Don’t need this local.

> Source/WebCore/svg/SVGPathUtilities.cpp:149
> +    appendedItemList.append(pathSeg.copyRef());

Seems to me that this should be WTF::move, not copyRef.

> Source/WebCore/svg/SVGTRefElement.cpp:68
> +    void attach(RefPtr<Element>&& target);

I think it’s a bit peculiar that this is RefPtr<Element>&& instead of just Element&.

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:65
> +void SVGFilterBuilder::appendEffectToEffectReferences(RefPtr<FilterEffect>&& filterEffect, RenderObject* object)

This should just be named "effect".

> Source/WebCore/svg/graphics/filters/SVGFilterBuilder.cpp:67
> +    RefPtr<FilterEffect> effect = WTF::move(filterEffect);

No need for this.

[Gyuyoung Kim]

Comment on attachment 260480 [details]
Patch

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

>> Source/WebCore/html/HTMLMediaElement.cpp:5051
>> +        return Node::addEventListener(eventType, listener.copyRef(), useCapture);
> 
> This should be WTF::move, not copyRef.

Right. Nice catch !

>> Source/WebCore/page/DOMWindow.cpp:1697
>> +    if (!EventTarget::addEventListener(eventType, listener.copyRef(), useCapture))
> 
> This should be WTF::move, not copyRef. None of the code below uses listener.

Thanks.

>> Source/WebCore/svg/SVGTRefElement.cpp:68
>> +    void attach(RefPtr<Element>&& target);
> 
> I think it’s a bit peculiar that this is RefPtr<Element>&& instead of just Element&.

To change to use Element&, it looks there are some additional changes. I would like to modify this in new bug.

[Gyuyoung Kim]

Created attachment 260844 [details]
Patch

[Gyuyoung Kim]

Created attachment 260848 [details]
Patch

[Gyuyoung Kim]

Created attachment 260850 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 260850 [details]
Patch

Clearing flags on attachment: 260850

Committed r189565: <http://trac.webkit.org/changeset/189565>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.