BlinkCloneableMessage and BlinkTransferableMessage claim to be move-only, but are copied |
|||
Issue description
The mojo typemaps for BlinkCloneableMessage [1] and BlinkTransferableMessage [2] claim that the structs are move-only, so the code in the mojo bindings uses std::move, as intended. However, since the structs declare a custom constructor (the constructor is defaulted in the .cpp, but the compiler can't see this from the .h), no move constructor or move-assignment is generated, and the structs are copied rather than being moved.
The mismatch causes extra reference increments / decrements. When the underlying type uses thread-safe reference counter (like SerializedScriptValue does), increments and decrements use atomic operations, which are not cheap on ARM.
The claims here can be verified by adding DISALLOW_COPY_AND_ASSIGN(BlinkCloneableMessage) to BlinkCloneableMessage.h and attempting to build Chrome. This should result in the error below.
../../third_party/WebKit/Source/modules/broadcastchannel/BroadcastChannel.cpp:72:29: error: call to deleted constructor of '::blink::BlinkCloneableMessage'
remote_client_->OnMessage(std::move(msg));
^~~~~~~~~~~~~~
../../third_party/WebKit/Source/core/dom/BlinkCloneableMessage.h:25:28: note: 'BlinkCloneableMessage' has been explicitly marked deleted here
DISALLOW_COPY_AND_ASSIGN(BlinkCloneableMessage);
^
gen/third_party/WebKit/public/platform/modules/broadcastchannel/broadcast_channel.mojom-blink.h:178:49: note: passing argument to parameter 'message' here
void OnMessage(::blink::BlinkCloneableMessage message) final;
^
I will submit a CL fixing this soon.
[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/BlinkCloneableMessage.typemap
[2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/BlinkTransferableMessage.typemap
,
Dec 14 2017
Whoops, sorry. Yeah, that's a mistake. I think I did do the right thing (with explicit move constructor/assignment) for the "non-blink" versions of those structs in WebKit/common/message_port but then somehow messed up here. Thanks for spotting/fixing.
,
Dec 14 2017
No worries, these things are for compilers (not humans) to worry about. If you'd like to take over / help with the review for the fix CL, it's at https://crrev.com/c/826260
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8c14ad2e43c2e7ae309ad337f691281090ad4b01 commit 8c14ad2e43c2e7ae309ad337f691281090ad4b01 Author: Victor Costan <pwnall@chromium.org> Date: Thu Dec 14 16:39:22 2017 Make BlinkCloneableMessage and BlinkTransferableMessage move-only. The mojo typemaps for BlinkCloneableMessage and BlinkTransferableMessage specify that the structs are move-only, so the code in the mojo bindings uses std::move. However, since the structs declare a custom constructor (the constructor is defaulted in the .cpp, but the compiler can't see this from the .h), no move constructor or move-assignment is generated, and the structs are copied rather than being moved. The mismatch causes extra reference increments / decrements. When the underlying type uses thread-safe reference counter (like SerializedScriptValue does), increments and decrements use atomic operations, which are not cheap on ARM. This CL adds move constructors and move-assignment operators to BlinkCloneableMessage to allow moving, and adds DISALLOW_COPY_AND_ASSIGN to prevent accidental copying. This CL also adds DISALLOW_COPY_AND_ASSIGN to the browser-side structures corresponding to Blink{Cloneable,Transferable}Message, to make it obvious that they are move-only. Bug: 794791 Change-Id: Iffe8f1e65bf9ccc2cd98082c341c2bdc3246fa33 Reviewed-on: https://chromium-review.googlesource.com/826260 Commit-Queue: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/master@{#524085} [modify] https://crrev.com/8c14ad2e43c2e7ae309ad337f691281090ad4b01/third_party/WebKit/Source/core/dom/BlinkCloneableMessage.cpp [modify] https://crrev.com/8c14ad2e43c2e7ae309ad337f691281090ad4b01/third_party/WebKit/Source/core/dom/BlinkCloneableMessage.h [modify] https://crrev.com/8c14ad2e43c2e7ae309ad337f691281090ad4b01/third_party/WebKit/Source/core/dom/BlinkTransferableMessage.cpp [modify] https://crrev.com/8c14ad2e43c2e7ae309ad337f691281090ad4b01/third_party/WebKit/Source/core/dom/BlinkTransferableMessage.h [modify] https://crrev.com/8c14ad2e43c2e7ae309ad337f691281090ad4b01/third_party/WebKit/common/message_port/cloneable_message.h [modify] https://crrev.com/8c14ad2e43c2e7ae309ad337f691281090ad4b01/third_party/WebKit/common/message_port/transferable_message.h
,
Dec 14 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by pwnall@chromium.org
, Dec 14 2017