[Webkit-unassigned] [Bug 153932] ScrollbarPainters needs to be deallocated on the main thread

[bugzilla-daemon at webkit.org]

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

--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 270781
  --> https://bugs.webkit.org/attachment.cgi?id=270781
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160206/0e156913/attachment.html>