Bug 137565 – Avoid defaulting to capture-by-value for C++11 lambdas in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Zan Dobersek]

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.

[Zan Dobersek]

Created attachment 239563 [details]
Patch

[WebKit Commit Bot]

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.

[Zan Dobersek]

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.

[Darin Adler]

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.

[Zan Dobersek]

Comment on attachment 239563 [details]
Patch

Clearing flags on attachment: 239563

Committed r174581: <http://trac.webkit.org/changeset/174581>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.