Bug 153932 – ScrollbarPainters needs to be deallocated on the main thread

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

ScrollbarPainters needs to be deallocated on the main thread

rdar://problem/24015483

[Beth Dakin]

Created attachment 270781 [details]
Patch

[Tim Horton]

Comment on attachment 270781 [details]
Patch

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:124
> +                {});

Curly brace style.

[WebKit Commit Bot]

Attachment 270781 [details] did not pass style-queue:


ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:118:  This { should be at the end of the previous line  [whitespace/braces] [4]
ERROR: Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:124:  Missing space inside { }.  [whitespace/braces] [5]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Beth Dakin]

http://trac.webkit.org/changeset/196206

[Darin Adler]

Comment on attachment 270781 [details]
Patch

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

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:75
> +        RetainPtr<ScrollbarPainter> retainedVerticalScrollbarPainter = m_verticalScrollbarPainter;
> +        RetainPtr<ScrollbarPainter> retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter;
> +        WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter] { });

I don’t think this is guaranteed to work. There will be six RetainPtr objects, two in the data members, two local variables, and two captured by the lambda. That seems to set up a race condition. If the call on the main thread completes before the ScrollingTreeFrameScrollingNodeMac is finished being destroyed, then the NSScrollerImp objects will still be deallocated on this thread.

Actually I see in the code that was actually landed we use WTFMove. This gets rid of two of the six RetainPtr objects, but there is still a race condition between the destruction of the two local variables and the destruction of the lambda on the main thread.

I’m not sure there is a way to do this with RetainPtr without more advanced C++ lambda support than we currently have. But we can do it safely if we don’t use RetainPtr. Here’s one way of writing it without the race condition:

    auto* retainedVerticalScrollbarPainter = m_verticalScrollbarPainter.leakRef();
    auto* retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter.leakRef();
    WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter] {
        [retainedVerticalScrollbarPainter release];
        [retainedHorizontalScrollbarPainter release];
    });

> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:123
> +            RetainPtr<ScrollbarPainter> retainedVerticalScrollbarPainter = m_verticalScrollbarPainter;
> +            RetainPtr<ScrollbarPainter> retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter;
> +            WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter]

Same issue here, which can be fixed the same way. Also unclear on why we are not doing the null check here that we are doing in the destructor. Maybe share a helper function?

[Beth Dakin]

Yup, you are totally right. Okay, I will post a new patch.

(In reply to comment #5)
> Comment on attachment 270781 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270781&action=review
> 
> > Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:75
> > +        RetainPtr<ScrollbarPainter> retainedVerticalScrollbarPainter = m_verticalScrollbarPainter;
> > +        RetainPtr<ScrollbarPainter> retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter;
> > +        WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter] { });
> 
> I don’t think this is guaranteed to work. There will be six RetainPtr
> objects, two in the data members, two local variables, and two captured by
> the lambda. That seems to set up a race condition. If the call on the main
> thread completes before the ScrollingTreeFrameScrollingNodeMac is finished
> being destroyed, then the NSScrollerImp objects will still be deallocated on
> this thread.
> 
> Actually I see in the code that was actually landed we use WTFMove. This
> gets rid of two of the six RetainPtr objects, but there is still a race
> condition between the destruction of the two local variables and the
> destruction of the lambda on the main thread.
> 
> I’m not sure there is a way to do this with RetainPtr without more advanced
> C++ lambda support than we currently have. But we can do it safely if we
> don’t use RetainPtr. Here’s one way of writing it without the race condition:
> 
>     auto* retainedVerticalScrollbarPainter =
> m_verticalScrollbarPainter.leakRef();
>     auto* retainedHorizontalScrollbarPainter =
> m_horizontalScrollbarPainter.leakRef();
>     WTF::callOnMainThread([retainedVerticalScrollbarPainter,
> retainedHorizontalScrollbarPainter] {
>         [retainedVerticalScrollbarPainter release];
>         [retainedHorizontalScrollbarPainter release];
>     });
> 
> > Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:123
> > +            RetainPtr<ScrollbarPainter> retainedVerticalScrollbarPainter = m_verticalScrollbarPainter;
> > +            RetainPtr<ScrollbarPainter> retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter;
> > +            WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter]
> 
> Same issue here, which can be fixed the same way. Also unclear on why we are
> not doing the null check here that we are doing in the destructor. Maybe
> share a helper function?

[Beth Dakin]

Created attachment 270806 [details]
Patch, v3

Comment on attachment 270806 [details]
Patch, v3

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

> Source/WebCore/ChangeLog:11
> +        condition between the destruction of the two local variables and the d

“the d”

[Beth Dakin]

the d!

thanks Dan!

http://trac.webkit.org/changeset/196226