Bug 137565 - Avoid defaulting to capture-by-value for C++11 lambdas in WebCore
Summary: Avoid defaulting to capture-by-value for C++11 lambdas in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-09 13:12 PDT by Zan Dobersek
Modified: 2014-10-10 03:28 PDT (History)
1 user (show)

See Also:


Attachments
Patch (40.48 KB, patch)
2014-10-09 13:25 PDT, Zan Dobersek
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2014-10-09 13:12:49 PDT
Using the default capture-by-value should be avoided as it can produce various negative side effects, e.g. capturing the this pointer when a member variable is used inside the lambda, and thus failing to make the lambda object self-contained.

It also requires the author to explicitly list the variables she intends to use inside the lambda, providing one additional checkpoint at ensuring that the correct variables are used.
Comment 1 Zan Dobersek 2014-10-09 13:25:36 PDT
Created attachment 239563 [details]
Patch
Comment 2 WebKit Commit Bot 2014-10-09 13:28:10 PDT
Attachment 239563 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/websockets/ThreadableWebSocketChannelClientWrapper.cpp:227:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:333:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:375:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/bindings/js/JSDOMGlobalObjectTask.cpp:86:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebCore/Modules/webdatabase/SQLCallbackWrapper.h:78:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 5 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Zan Dobersek 2014-10-09 13:28:37 PDT
Comment on attachment 239563 [details]
Patch

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

Outlined a few problems that could have been avoided, most likely of my doing.

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:422
> -    m_loaderProxy.postTaskToLoader([=](ScriptExecutionContext& context) {
> +    m_loaderProxy.postTaskToLoader([peer, urlCopy, protocolCopy] (ScriptExecutionContext& context) {
>          ASSERT(isMainThread());
>          ASSERT_UNUSED(context, context.isDocument());
>          ASSERT(peer);
>  
> -        peer->connect(url, protocol);
> +        peer->connect(urlCopy, protocolCopy);

Case in point.

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:439
> -    m_loaderProxy.postTaskToLoader([=](ScriptExecutionContext& context) {
> +    m_loaderProxy.postTaskToLoader([peer, messageCopy] (ScriptExecutionContext& context) {
>          ASSERT(isMainThread());
>          ASSERT_UNUSED(context, context.isDocument());
>          ASSERT(peer);
>  
> -        peer->send(message);
> +        peer->send(messageCopy);

Ditto.

> Source/WebCore/Modules/websockets/WorkerThreadableWebSocketChannel.cpp:558
> -    m_loaderProxy.postTaskToLoader([=] (ScriptExecutionContext& context) {
> +    m_loaderProxy.postTaskToLoader([peer, reasonCopy] (ScriptExecutionContext& context) {
>          ASSERT(isMainThread());
>          ASSERT_UNUSED(context, context.isDocument());
>          ASSERT(peer);
>  
> -        peer->fail(reason);
> +        peer->fail(reasonCopy);

Ditto.

> Source/WebCore/dom/StringCallback.cpp:43
>      RefPtr<StringCallback> protector(this);
> -    context->postTask([=] (ScriptExecutionContext&) {
> -        this->handleEvent(data);
> +    context->postTask([protector, data] (ScriptExecutionContext&) {
> +        protector->handleEvent(data);

Ditto.
Comment 4 Darin Adler 2014-10-09 14:52:26 PDT
Comment on attachment 239563 [details]
Patch

I like this better. I remember being a little puzzled by the [=] syntax earlier, since I have always used the exhaustive list of what to capture.
Comment 5 Zan Dobersek 2014-10-10 03:28:43 PDT
Comment on attachment 239563 [details]
Patch

Clearing flags on attachment: 239563

Committed r174581: <http://trac.webkit.org/changeset/174581>
Comment 6 Zan Dobersek 2014-10-10 03:28:52 PDT
All reviewed patches have been landed.  Closing bug.