<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - ScrollbarPainters needs to be deallocated on the main thread"
href="https://bugs.webkit.org/show_bug.cgi?id=153932#c5">Comment # 5</a>
on <a class="bz_bug_link
bz_status_RESOLVED bz_closed"
title="RESOLVED FIXED - ScrollbarPainters needs to be deallocated on the main thread"
href="https://bugs.webkit.org/show_bug.cgi?id=153932">bug 153932</a>
from <span class="vcard"><a class="email" href="mailto:darin@apple.com" title="Darin Adler <darin@apple.com>"> <span class="fn">Darin Adler</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=270781&action=diff" name="attach_270781" title="Patch">attachment 270781</a> <a href="attachment.cgi?id=270781&action=edit" title="Patch">[details]</a></span>
Patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=270781&action=review">https://bugs.webkit.org/attachment.cgi?id=270781&action=review</a>
<span class="quote">> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:75
> + RetainPtr<ScrollbarPainter> retainedVerticalScrollbarPainter = m_verticalScrollbarPainter;
> + RetainPtr<ScrollbarPainter> retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter;
> + WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter] { });</span >
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];
});
<span class="quote">> Source/WebCore/page/scrolling/mac/ScrollingTreeFrameScrollingNodeMac.mm:123
> + RetainPtr<ScrollbarPainter> retainedVerticalScrollbarPainter = m_verticalScrollbarPainter;
> + RetainPtr<ScrollbarPainter> retainedHorizontalScrollbarPainter = m_horizontalScrollbarPainter;
> + WTF::callOnMainThread([retainedVerticalScrollbarPainter, retainedHorizontalScrollbarPainter]</span >
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?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>