Skip to content

Commit

Permalink
Revert "Use BigBuffer for legacy IPC messages"
Browse files Browse the repository at this point in the history
This reverts commit 3e12619.

Reason for revert: Seems to be causing test failures:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Mac10.13%20Tests%20%28dbg%29
https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8941340516261567504%2F%2B%2Fsteps%2Fipc_tests%2F0%2Flogs%2FIPCChannelMojoTest.SendFailWithPendingMessages%2F0

Original change's description:
> Use BigBuffer for legacy IPC messages
> 
> This replaces use of ReadOnlyBuffer with a new mojom Message type
> specific to //ipc. This type maps to another new C++ MessageView type
> which in turn wraps a BigBufferView.
> 
> This allows us to transparently fall back onto shared memory for large
> IPC messages without increasing the number of copies during send or
> receive in any (small- or large-message) cases.
> 
> In order to avoid introducing more mojo-base targets, this also removes
> the remaining [Native] structs from mojo_base mojom (LOGFONT and
> FileInfo) and replaces them with real mojom structures + StructTraits,
> thus allowing //ipc to depend on mojo/public/*/base in its entirety.
> 
> Also fixes random missing public_deps entries for a
> chrome/services/file_util typemap, because it decided to start breaking
> all of my local builds. :3
> 
> Bug: 784069
> Change-Id: I359b964ffc1fe44ffd6aa704405ea63156f4fbc9
> Reviewed-on: https://chromium-review.googlesource.com/1131685
> Commit-Queue: Ken Rockot <rockot@chromium.org>
> Reviewed-by: Daniel Cheng <dcheng@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#573956}

TBR=dcheng@chromium.org,rockot@chromium.org

Change-Id: Id30a64213605fb645e46632589d5ce2a4fbc0077
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 784069
Reviewed-on: https://chromium-review.googlesource.com/1132495
Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
Commit-Queue: Alice Boxhall <aboxhall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574057}
  • Loading branch information
Alice Boxhall authored and Commit Bot committed Jul 11, 2018
1 parent f3d6cdd commit b0cfd1d
Show file tree
Hide file tree
Showing 27 changed files with 79 additions and 391 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public_headers = [ "//chrome/common/safe_browsing/zip_analyzer.h" ]

traits_headers = [ "//chrome/services/file_util/public/mojom/safe_archive_analyzer_param_traits.h" ]

public_deps = [
deps = [
"//chrome/common/safe_browsing:proto",
"//components/safe_browsing:csd_proto",
]
Expand Down
3 changes: 1 addition & 2 deletions ipc/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ component("ipc") {
":mojom",
":native_handle_type_converters",
":param_traits",
"//mojo/public/cpp/base",
"//mojo/public/cpp/bindings",
"//mojo/public/cpp/system",
]
Expand Down Expand Up @@ -183,7 +182,7 @@ mojom_component("mojom") {
]
public_deps = [
"//mojo/public/interfaces/bindings",
"//mojo/public/mojom/base",
"//mojo/public/mojom/base:read_only_buffer",
]

# Don't generate a variant sources since we depend on generated internal
Expand Down
4 changes: 0 additions & 4 deletions ipc/OWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,8 @@ per-file *_messages*.h=set noparent
per-file *_messages*.h=file://ipc/SECURITY_OWNERS
per-file *.mojom=set noparent
per-file *.mojom=file://ipc/SECURITY_OWNERS
per-file *_mojom_traits*.*=set noparent
per-file *_mojom_traits*.*=file://ipc/SECURITY_OWNERS
per-file *_param_traits*.*=set noparent
per-file *_param_traits*.*=file://ipc/SECURITY_OWNERS
per-file *.typemap=set noparent
per-file *.typemap=file://ipc/SECURITY_OWNERS
per-file *_type_converter*.*=set noparent
per-file *_type_converter*.*=file://ipc/SECURITY_OWNERS

Expand Down
12 changes: 3 additions & 9 deletions ipc/ipc.mojom
Original file line number Diff line number Diff line change
Expand Up @@ -4,27 +4,21 @@

module IPC.mojom;

import "mojo/public/mojom/base/big_buffer.mojom";
import "mojo/public/mojom/base/read_only_buffer.mojom";
import "mojo/public/interfaces/bindings/native_struct.mojom";

// A placeholder interface type since we don't yet support generic associated
// message pipe handles.
interface GenericInterface {};

// Typemapped such that arbitrarily large IPC::Message objects can be sent and
// received with minimal copying.
struct Message {
mojo_base.mojom.BigBuffer buffer;
array<mojo.native.SerializedHandle>? handles;
};

interface Channel {
// Informs the remote end of this client's PID. Must be called exactly once,
// before any calls to Receive() below.
SetPeerPid(int32 pid);

// Transmits a classical Chrome IPC message.
Receive(Message message);
Receive(mojo_base.mojom.ReadOnlyBuffer data,
array<mojo.native.SerializedHandle>? handles);

// Requests a Channel-associated interface.
GetAssociatedInterface(string name, associated GenericInterface& request);
Expand Down
18 changes: 12 additions & 6 deletions ipc/ipc_message_pipe_reader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,10 @@ bool MessagePipeReader::Send(std::unique_ptr<Message> message) {
if (!sender_)
return false;

sender_->Receive(MessageView(*message, std::move(handles)));
sender_->Receive(base::make_span(static_cast<const uint8_t*>(message->data()),
message->size()),
std::move(handles));

DVLOG(4) << "Send " << message->type() << ": " << message->size();
return true;
}
Expand All @@ -81,20 +84,23 @@ void MessagePipeReader::SetPeerPid(int32_t peer_pid) {
delegate_->OnPeerPidReceived(peer_pid);
}

void MessagePipeReader::Receive(MessageView message_view) {
if (!message_view.size()) {
void MessagePipeReader::Receive(
base::span<const uint8_t> data,
base::Optional<std::vector<mojo::native::SerializedHandlePtr>> handles) {
if (data.empty()) {
delegate_->OnBrokenDataReceived();
return;
}
Message message(message_view.data(), message_view.size());
Message message(reinterpret_cast<const char*>(data.data()),
static_cast<uint32_t>(data.size()));
if (!message.IsValid()) {
delegate_->OnBrokenDataReceived();
return;
}

DVLOG(4) << "Receive " << message.type() << ": " << message.size();
MojoResult write_result = ChannelMojo::WriteToMessageAttachmentSet(
message_view.TakeHandles(), &message);
MojoResult write_result =
ChannelMojo::WriteToMessageAttachmentSet(std::move(handles), &message);
if (write_result != MOJO_RESULT_OK) {
OnPipeError(write_result);
return;
Expand Down
5 changes: 4 additions & 1 deletion ipc/ipc_message_pipe_reader.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "mojo/public/cpp/bindings/scoped_interface_endpoint_handle.h"
#include "mojo/public/cpp/system/core.h"
#include "mojo/public/cpp/system/message_pipe.h"
#include "mojo/public/interfaces/bindings/native_struct.mojom.h"

namespace IPC {
namespace internal {
Expand Down Expand Up @@ -94,7 +95,9 @@ class COMPONENT_EXPORT(IPC) MessagePipeReader : public mojom::Channel {
private:
// mojom::Channel:
void SetPeerPid(int32_t peer_pid) override;
void Receive(MessageView message_view) override;
void Receive(base::span<const uint8_t> data,
base::Optional<std::vector<mojo::native::SerializedHandlePtr>>
handles) override;
void GetAssociatedInterface(
const std::string& name,
mojom::GenericInterfaceAssociatedRequest request) override;
Expand Down
25 changes: 25 additions & 0 deletions ipc/ipc_message_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1414,6 +1414,31 @@ void ParamTraits<HANDLE>::Log(const param_type& p, std::string* l) {
l->append(base::StringPrintf("0x%p", p));
}

void ParamTraits<LOGFONT>::Write(base::Pickle* m, const param_type& p) {
m->WriteData(reinterpret_cast<const char*>(&p), sizeof(LOGFONT));
}

bool ParamTraits<LOGFONT>::Read(const base::Pickle* m,
base::PickleIterator* iter,
param_type* r) {
const char *data;
int data_size = 0;
if (iter->ReadData(&data, &data_size) && data_size == sizeof(LOGFONT)) {
const LOGFONT *font = reinterpret_cast<LOGFONT*>(const_cast<char*>(data));
if (_tcsnlen(font->lfFaceName, LF_FACESIZE) < LF_FACESIZE) {
memcpy(r, data, sizeof(LOGFONT));
return true;
}
}

NOTREACHED();
return false;
}

void ParamTraits<LOGFONT>::Log(const param_type& p, std::string* l) {
l->append(base::StringPrintf("<LOGFONT>"));
}

void ParamTraits<MSG>::Write(base::Pickle* m, const param_type& p) {
m->WriteData(reinterpret_cast<const char*>(&p), sizeof(MSG));
}
Expand Down
10 changes: 10 additions & 0 deletions ipc/ipc_message_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -1018,6 +1018,16 @@ struct COMPONENT_EXPORT(IPC) ParamTraits<HANDLE> {
static void Log(const param_type& p, std::string* l);
};

template <>
struct COMPONENT_EXPORT(IPC) ParamTraits<LOGFONT> {
typedef LOGFONT param_type;
static void Write(base::Pickle* m, const param_type& p);
static bool Read(const base::Pickle* m,
base::PickleIterator* iter,
param_type* r);
static void Log(const param_type& p, std::string* l);
};

template <>
struct COMPONENT_EXPORT(IPC) ParamTraits<MSG> {
typedef MSG param_type;
Expand Down
11 changes: 6 additions & 5 deletions ipc/ipc_mojo_bootstrap_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,14 @@ class PeerPidReceiver : public IPC::mojom::Channel {
on_peer_pid_set_.Run();
}

void Receive(IPC::MessageView message_view) override {
void Receive(base::span<const uint8_t> data,
base::Optional<std::vector<mojo::native::SerializedHandlePtr>>
handles) override {
ASSERT_NE(MessageExpectation::kNotExpected, message_expectation_);
received_message_ = true;

IPC::Message message(message_view.data(), message_view.size());
IPC::Message message(reinterpret_cast<const char*>(data.data()),
static_cast<uint32_t>(data.size()));
bool expected_valid =
message_expectation_ == MessageExpectation::kExpectedValid;
EXPECT_EQ(expected_valid, message.IsValid());
Expand Down Expand Up @@ -193,9 +196,7 @@ MULTIPROCESS_TEST_MAIN_WITH_SETUP(
auto& sender = connection.GetSender();

uint8_t data = 0;
sender->Receive(
IPC::MessageView(mojo_base::BigBufferView(base::make_span(&data, 0)),
base::nullopt /* handles */));
sender->Receive(base::make_span(&data, 0), {});

base::RunLoop run_loop;
PeerPidReceiver impl(std::move(receiver), run_loop.QuitClosure());
Expand Down
18 changes: 0 additions & 18 deletions ipc/message.typemap

This file was deleted.

40 changes: 0 additions & 40 deletions ipc/message_mojom_traits.cc

This file was deleted.

31 changes: 0 additions & 31 deletions ipc/message_mojom_traits.h

This file was deleted.

30 changes: 0 additions & 30 deletions ipc/message_view.cc

This file was deleted.

56 changes: 0 additions & 56 deletions ipc/message_view.h

This file was deleted.

5 changes: 0 additions & 5 deletions ipc/typemaps.gni

This file was deleted.

Loading

0 comments on commit b0cfd1d

Please sign in to comment.