RESOLVED FIXED 137565
Avoid defaulting to capture-by-value for C++11 lambdas in WebCore
https://bugs.webkit.org/show_bug.cgi?id=137565
Summary Avoid defaulting to capture-by-value for C++11 lambdas in WebCore
Zan Dobersek
Reported 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.
Attachments
Patch (40.48 KB, patch)
2014-10-09 13:25 PDT, Zan Dobersek
no flags
Zan Dobersek
Comment 1 2014-10-09 13:25:36 PDT
WebKit Commit Bot
Comment 2 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.
Zan Dobersek
Comment 3 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.
Darin Adler
Comment 4 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.
Zan Dobersek
Comment 5 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>
Zan Dobersek
Comment 6 2014-10-10 03:28:52 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.