Bug 153932 - ScrollbarPainters needs to be deallocated on the main thread
Summary: ScrollbarPainters needs to be deallocated on the main thread
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-02-05 17:01 PST by Beth Dakin
Modified: 2016-02-06 15:42 PST (History)
13 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2016-02-05 17:04 PST, Beth Dakin
thorton: review+
Details | Formatted Diff | Diff
Patch, v3 (4.52 KB, patch)
2016-02-06 15:10 PST, Beth Dakin
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2016-02-05 17:01:22 PST
ScrollbarPainters needs to be deallocated on the main thread

rdar://problem/24015483
Comment 1 Beth Dakin 2016-02-05 17:04:03 PST
Created attachment 270781 [details]
Patch
Comment 2 Tim Horton 2016-02-05 17:05:47 PST
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.
Comment 3 WebKit Commit Bot 2016-02-05 17:05:59 PST
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.
Comment 4 Beth Dakin 2016-02-05 17:09:21 PST
http://trac.webkit.org/changeset/196206
Comment 5 Darin Adler 2016-02-06 08:40:51 PST
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?
Comment 6 Beth Dakin 2016-02-06 15:00:50 PST
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?
Comment 7 Beth Dakin 2016-02-06 15:10:45 PST
Created attachment 270806 [details]
Patch, v3
Comment 8 mitz 2016-02-06 15:37:01 PST
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”
Comment 9 Beth Dakin 2016-02-06 15:42:52 PST
the d!

thanks Dan!

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