Bug 158166 – callOnMainThread() should not copy captured lambda variables

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

callOnMainThread() should not copy captured lambda variables to reduce the risk of thread-related races.

[Chris Dumez]

Created attachment 280012 [details]
WIP patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:132:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:314:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaSourceAVFObjC.mm:333:  Missing space before {  [whitespace/braces] [5]
Total errors found: 3 in 54 files


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

[Chris Dumez]

Created attachment 280015 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/MemoryPressureHandler.cpp:175:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/Storage/StorageThread.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 61 files


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

[Chris Dumez]

Created attachment 280016 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/MemoryPressureHandler.cpp:175:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/Storage/StorageThread.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 61 files


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

[Chris Dumez]

Created attachment 280017 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/MemoryPressureHandler.cpp:175:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/Storage/StorageThread.cpp:112:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 61 files


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

[Chris Dumez]

Patch is ready for review. It is building everywhere and passing the tests.

[Brady Eidson]

Comment on attachment 280017 [details]
Patch

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

This is so great.

r-, but the changes are just mechanical replacements.

This patch makes it clear to me I'll need to try to update the style checker rules for lambda captures :)

Imagine the style checker caught all these protectors - Now would be a great time to update them! :)

> Source/WTF/wtf/FunctionDispatcher.h:30
> +#include <wtf/Functional.h>

Should we call the new header NoncopyableFunction.h, as that is its only contents?

> Source/WebCore/platform/MemoryPressureHandler.cpp:175
> +        ScrollingThread::dispatch([]() { WTF::releaseFastMallocFreeMemory(); });

Please put this on two lines - I don't think there should ever be an exception for any lambda, no matter how simple it is, as it is important for them to stand out at a glance.

> Source/WebCore/platform/audio/ios/MediaSessionManagerIOS.mm:435
> +    callOnMainThread([strongSelf = RetainPtr<WebMediaSessionHelper>(self)] () {

protectedSelf (Here, and everywhere else where we touch a "strongSelf")

> Source/WebCore/platform/graphics/avfoundation/AudioSourceProviderAVFObjC.mm:326
> +    callOnMainThread([strongThis = Ref<AudioSourceProviderAVFObjC>(*this), numberOfChannels, sampleRate] {

protectedThis (here, and everywhere else we touch a "strongThis")

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:279
> +    callOnMainThread([strongSelf = WTFMove(strongSelf), strongData = WTFMove(strongData), trackID, hasSessionSemaphore = WTFMove(hasSessionSemaphore)] {

protectedInitData

> Source/WebKit/Storage/StorageThread.cpp:112
> +        (*it)->dispatch([]() { WTF::releaseFastMallocFreeMemory(); });

Please put this on two lines - I don't think there should ever be an exception for any lambda, no matter how simple it is, as it is important for them to stand out at a glance.

[Chris Dumez]

Created attachment 280019 [details]
Patch

[Brady Eidson]

MockSourceBufferPrivate - exit_code: 65

huh.

[Brady Eidson]

Comment on attachment 280019 [details]
Patch

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

r+ (with one nit) assuming you get the build working.

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:279
> -    RetainPtr<WebAVStreamDataParserListener> strongSelf = self;
> +    RetainPtr<WebAVStreamDataParserListener> protectedSelf = self;
>  
> -    RetainPtr<NSData> strongData = initData;
> +    RetainPtr<NSData> protectedInitData = initData;
>      OSObjectPtr<dispatch_semaphore_t> hasSessionSemaphore = adoptOSObject(dispatch_semaphore_create(0));
> -    callOnMainThread([strongSelf, strongData, trackID,  hasSessionSemaphore] {
> -        if (strongSelf->_parent)
> -            strongSelf->_parent->didProvideContentKeyRequestInitializationDataForTrackID(strongData.get(), trackID, hasSessionSemaphore);
> +    callOnMainThread([protectedSelf = WTFMove(protectedSelf), protectedInitData = WTFMove(protectedInitData), trackID, hasSessionSemaphore = WTFMove(hasSessionSemaphore)] {

If you have to take another pass to get the builds working, you can initialize the two RetainPtrs here inside the lambda capture instead of having the local variables.

[Chris Dumez]

Created attachment 280021 [details]
Patch

[Chris Dumez]

Comment on attachment 280021 [details]
Patch

Clearing flags on attachment: 280021

Committed r201482: <http://trac.webkit.org/changeset/201482>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #15)
> Comment on attachment 280021 [details]
> Patch
> 
> Clearing flags on attachment: 280021
> 
> Committed r201482: <http://trac.webkit.org/changeset/201482>

It broke the WinCairo (curl) build, see build.webkit.org for details.
(cc-ing port maintainers)

[Alex Christensen]

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