[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 #6 from Beth Dakin <bdakin at apple.com> ---
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?

-- 
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/ff0bf07b/attachment.html>