| Summary: | [WK2] Use C++ lambdas in IPC::Connection | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Zan Dobersek <zan> | ||||||||||||||||||
| Component: | New Bugs | Assignee: | Zan Dobersek <zan> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | commit-queue, koivisto | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| Bug Depends on: | 138666 | ||||||||||||||||||||
| Bug Blocks: | |||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Zan Dobersek
2014-10-23 13:31:04 PDT
Created attachment 240365 [details]
Patch
Created attachment 240621 [details]
Patch
Comment on attachment 240621 [details] Patch Clearing flags on attachment: 240621 Committed r175806: <http://trac.webkit.org/changeset/175806> All reviewed patches have been landed. Closing bug. Comment on attachment 240621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240621&action=review > Source/WebKit2/Platform/IPC/Connection.cpp:647 > + CString messageReceiverName = message->messageReceiverName().isEmpty() ? "<unknown message>" : message->messageReceiverName().toString(); > + CString messageName = message->messageName().isEmpty() ? String::format("<message length: %zu bytes>", message->length()).utf8() : message->messageReceiverName().toString(); > + > + RefPtr<Connection> protectedThis(this); > + m_clientRunLoop.dispatch([protectedThis, messageReceiverName, messageName] { Is m_clientRunLoop the current run loop? If not the capture of messageReceiverName and messageName does not look thread safe. You could use StringCapture instead. > Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm:516 > + CString messageReceiverNameString = decoder->messageReceiverName().toString(); > + CString messageNameString = decoder->messageName().toString(); > + m_clientRunLoop.dispatch([protectedThis, messageReceiverNameString, messageNameString] { > + protectedThis->dispatchDidReceiveInvalidMessage(messageReceiverNameString, messageNameString); > + }); Here too. Comment on attachment 240621 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240621&action=review >> Source/WebKit2/Platform/IPC/Connection.cpp:647 >> + m_clientRunLoop.dispatch([protectedThis, messageReceiverName, messageName] { > > Is m_clientRunLoop the current run loop? If not the capture of messageReceiverName and messageName does not look thread safe. You could use StringCapture instead. Can we avoid formatting the "<message length ...>" string and use something static instead? That way we wouldn't have to handle CStrings at all, and could capture StringReference objects instead. Re-opened since this is blocked by bug 138666 Created attachment 241571 [details]
Patch
Created attachment 244888 [details]
Patch
Comment on attachment 244888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244888&action=review > Source/WebKit2/Platform/IPC/Connection.cpp:646 > + m_clientRunLoop.dispatch([protectedThis, messageReceiverName, messageName] { > + protectedThis->dispatchDidReceiveInvalidMessage(messageReceiverName, messageName); > + }); messageReceiverName and messageName could point to into a MessageDecoder that has been freed here. Created attachment 248196 [details]
Patch
Comment on attachment 248196 [details]
Patch
Doesn't yet build.
Created attachment 248241 [details]
Patch
Created attachment 248321 [details]
Patch
Created attachment 248326 [details]
Patch
Comment on attachment 248326 [details]
Patch
This finally compiles everywhere, so I'm setting the r? flag.
Comment on attachment 248326 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248326&action=review > Source/WebKit2/Platform/IPC/Connection.cpp:348 > // Reset the client. Can remove this comment. Committed r181630: <http://trac.webkit.org/changeset/181630> |