Commit 1474d6b5 authored by Findit's avatar Findit

Revert "Mojo: NodeChannel should require a minimum size when using multiple versions"

This reverts commit 8ad25003.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 828478 as the
culprit for flakes in the build cycles as shown on:
https://analysis.chromium.org/p/chromium/flake-portal/analysis/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vOGFkMjUwMDNkZTgwODIxMjhjM2IzNDU2NWVhMjg4ZGM3OGZhODhkYgw

Sample Failed Build: https://ci.chromium.org/b/8863329295603494640

Sample Failed Step: mojo_unittests on (none) GPU on Mac on Mac-10.14.6

Sample Flaky Test: NodeChannelTest.MessagesCannotBeSmallerThanOldestVersion

Original change's description:
> Mojo: NodeChannel should require a minimum size when using multiple versions
> 
> When we added support for versioned message we accidentally introduced a
> behavior where messages can be shorter than expected. This is what the
> behavior should be to allow older messages to be received. However,
> we know the minimum size is the oldest message version. We should
> never be allowed to go smaller than that.
> 
> BUG=chromium:1149805
> 
> Change-Id: I3a24dd62b6fafd742cd7ac155f863d33bb5c6de8
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2545207
> Reviewed-by: Ken Rockot <rockot@google.com>
> Commit-Queue: Brian Geffon <bgeffon@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#828478}


No-Presubmit: true
No-Tree-Checks: true
No-Try: true
BUG=chromium:1149805

Change-Id: I0bb4a4605a1f705c4388769fae4559e0835d1980
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2546460
Cr-Commit-Position: refs/heads/master@{#828581}
parent ef609df7
...@@ -190,16 +190,13 @@ Channel::MessagePtr CreateMessage(MessageType type, ...@@ -190,16 +190,13 @@ Channel::MessagePtr CreateMessage(MessageType type,
return msg_ptr; return msg_ptr;
} }
// This method takes a second template argument which is another datatype which template <typename DataType>
// represents the smallest size this payload can be to be considered valid this bool GetMessagePayload(const void* bytes,
// MUST be used when there is more than one version of a message to specify the size_t num_bytes,
// oldest version of the message. DataType* out_data) {
template <typename DataType, typename MinSizedDataType>
bool GetMessagePayloadMinimumSized(const void* bytes,
size_t num_bytes,
DataType* out_data) {
static_assert(sizeof(DataType) > 0, "DataType must have non-zero size."); static_assert(sizeof(DataType) > 0, "DataType must have non-zero size.");
if (num_bytes < sizeof(Header) + sizeof(MinSizedDataType)) // We should have at least 1 byte to contribute towards DataType.
if (num_bytes <= sizeof(Header))
return false; return false;
// Always make sure that the full object is zeored and default constructed as // Always make sure that the full object is zeored and default constructed as
...@@ -214,14 +211,6 @@ bool GetMessagePayloadMinimumSized(const void* bytes, ...@@ -214,14 +211,6 @@ bool GetMessagePayloadMinimumSized(const void* bytes,
return true; return true;
} }
template <typename DataType>
bool GetMessagePayload(const void* bytes,
size_t num_bytes,
DataType* out_data) {
return GetMessagePayloadMinimumSized<DataType, DataType>(bytes, num_bytes,
out_data);
}
} // namespace } // namespace
// static // static
......
...@@ -5,7 +5,6 @@ ...@@ -5,7 +5,6 @@
#include "mojo/core/node_channel.h" #include "mojo/core/node_channel.h"
#include "base/callback_helpers.h" #include "base/callback_helpers.h"
#include "base/logging.h"
#include "base/memory/scoped_refptr.h" #include "base/memory/scoped_refptr.h"
#include "base/message_loop/message_pump_type.h" #include "base/message_loop/message_pump_type.h"
#include "base/test/task_environment.h" #include "base/test/task_environment.h"
...@@ -22,7 +21,6 @@ namespace { ...@@ -22,7 +21,6 @@ namespace {
using NodeChannelTest = testing::Test; using NodeChannelTest = testing::Test;
using ports::NodeName; using ports::NodeName;
using testing::_;
scoped_refptr<NodeChannel> CreateNodeChannel(NodeChannel::Delegate* delegate, scoped_refptr<NodeChannel> CreateNodeChannel(NodeChannel::Delegate* delegate,
PlatformChannelEndpoint endpoint) { PlatformChannelEndpoint endpoint) {
...@@ -69,48 +67,6 @@ TEST_F(NodeChannelTest, DestructionIsSafe) { ...@@ -69,48 +67,6 @@ TEST_F(NodeChannelTest, DestructionIsSafe) {
error_loop.Run(); error_loop.Run();
} }
TEST_F(NodeChannelTest, MessagesCannotBeSmallerThanOldestVersion) {
base::test::TaskEnvironment task_environment;
PlatformChannel channel;
MockNodeChannelDelegate local_delegate;
auto local_channel =
CreateNodeChannel(&local_delegate, channel.TakeLocalEndpoint());
local_channel->Start();
MockNodeChannelDelegate remote_delegate;
auto remote_channel =
CreateNodeChannel(&remote_delegate, channel.TakeRemoteEndpoint());
remote_channel->Start();
base::RunLoop loop;
// It's a bad message and shouldn't be passed to the delegate.
EXPECT_CALL(local_delegate, OnRequestPortMerge(_, _, _)).Times(0);
// This good message should go through after.
const NodeName kRemoteNodeName{123, 456};
const NodeName kToken{987, 654};
EXPECT_CALL(local_delegate,
OnAcceptInvitee(ports::kInvalidNodeName, kRemoteNodeName, kToken))
.WillRepeatedly([&] {
loop.Quit(); });
// 1 byte is not enough to contain the oldest version of the request port
// merge payload, it should be discarded.
int payload_size = 1;
int capacity = /*sizeof(header)=*/8 + payload_size;
auto message =
std::make_unique<Channel::Message>(capacity, capacity, /*num_handles=*/0);
// Set the type of this message as REQUEST_PORT_MERGE (6)
*reinterpret_cast<uint32_t*>(message->mutable_payload()) = 6;
// This short message should be ignored.
remote_channel->SendChannelMessage(std::move(message));
remote_channel->AcceptInvitee(kRemoteNodeName, kToken);
loop.Run();
}
} // namespace } // namespace
} // namespace core } // namespace core
} // namespace mojo } // namespace mojo
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment