Skip to content

Commit

Permalink
Reland "Migrate call-sites away from un-managed PlatformHandle[Vector…
Browse files Browse the repository at this point in the history 
…]s."

This is a reland of 7df6fea, which was
speculatively reverted as possible cause of crbug.com/788192, since the
CL originally landed only after that regression.

Original change's description:
> Migrate call-sites away from un-managed PlatformHandle[Vector]s.
>
> As the first step to merging ScopedPlatformHandle into PlatformHandle
> to have a single, always-managed container for platform handles, we:
>
> - Replace ScopedPlatformHandleVectorPtr and PlatformHandleVector
>   with vector<ScopedPlatformHandle>.
> - Replace PlatformHandle with ScopedPlatformHandle in some core APIs.
> - Use const& to pass handles to APIs without passing ownership, for
>   both individual platform handles, and vectors.
>
> Bug: 753541
> Change-Id: Id965dcf3770ef11bf36f78277fa7199ea779599e
> Reviewed-on: https://chromium-review.googlesource.com/742309
> Reviewed-by: Ricky Liang <jcliang@chromium.org>
> Reviewed-by: Ken Rockot <rockot@chromium.org>
> Reviewed-by: Dominick Ng <dominickn@chromium.org>
> Reviewed-by: Luis Hector Chavez <lhchavez@chromium.org>
> Commit-Queue: Wez <wez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#518343}

Bug: 753541
TBR: lhchavez, jcliang, dominickn, rockot
Change-Id: I9d50f06052b2118aec8872ca149504fbed245f1a
Reviewed-on: https://chromium-review.googlesource.com/792255
Reviewed-by: Wez <wez@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519505}
  • Loading branch information
Wez authored and Commit Bot committed Nov 28, 2017
1 parent 6d1a552 commit 6d83a0b
Show file tree
Hide file tree
Showing 41 changed files with 475 additions and 610 deletions.
3 changes: 1 addition & 2 deletions chrome/browser/apps/app_shim/unix_domain_socket_acceptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,7 @@ bool UnixDomainSocketAcceptor::Listen() {
void UnixDomainSocketAcceptor::OnFileCanReadWithoutBlocking(int fd) {
DCHECK(fd == listen_handle_.get().handle);
mojo::edk::ScopedPlatformHandle connection_handle;
if (!mojo::edk::ServerAcceptConnection(listen_handle_.get(),
&connection_handle)) {
if (!mojo::edk::ServerAcceptConnection(listen_handle_, &connection_handle)) {
Close();
delegate_->OnListenError();
return;
Expand Down
12 changes: 5 additions & 7 deletions components/arc/arc_session_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <unistd.h>

#include <utility>
#include <vector>

#include "base/location.h"
#include "base/posix/eintr_wrapper.h"
Expand All @@ -27,7 +28,6 @@
#include "mojo/edk/embedder/outgoing_broker_client_invitation.h"
#include "mojo/edk/embedder/platform_channel_pair.h"
#include "mojo/edk/embedder/platform_channel_utils_posix.h"
#include "mojo/edk/embedder/platform_handle_vector.h"
#include "mojo/edk/embedder/scoped_platform_handle.h"
#include "mojo/public/cpp/bindings/binding.h"

Expand Down Expand Up @@ -183,7 +183,7 @@ mojo::ScopedMessagePipeHandle ArcSessionDelegateImpl::ConnectMojoInternal(
}

mojo::edk::ScopedPlatformHandle scoped_fd;
if (!mojo::edk::ServerAcceptConnection(socket_fd.get(), &scoped_fd,
if (!mojo::edk::ServerAcceptConnection(socket_fd, &scoped_fd,
/* check_peer_user = */ false) ||
!scoped_fd.is_valid()) {
return mojo::ScopedMessagePipeHandle();
Expand All @@ -202,9 +202,8 @@ mojo::ScopedMessagePipeHandle ArcSessionDelegateImpl::ConnectMojoInternal(
mojo::edk::ConnectionParams(mojo::edk::TransportProtocol::kLegacy,
channel_pair.PassServerHandle()));

mojo::edk::ScopedPlatformHandleVectorPtr handles(
new mojo::edk::PlatformHandleVector{
channel_pair.PassClientHandle().release()});
std::vector<mojo::edk::ScopedPlatformHandle> handles;
handles.emplace_back(channel_pair.PassClientHandle());

// We need to send the length of the message as a single byte, so make sure it
// fits.
Expand All @@ -213,8 +212,7 @@ mojo::ScopedMessagePipeHandle ArcSessionDelegateImpl::ConnectMojoInternal(
struct iovec iov[] = {{&message_length, sizeof(message_length)},
{const_cast<char*>(token.c_str()), token.size()}};
ssize_t result = mojo::edk::PlatformChannelSendmsgWithHandles(
scoped_fd.get(), iov, sizeof(iov) / sizeof(iov[0]), handles->data(),
handles->size());
scoped_fd, iov, sizeof(iov) / sizeof(iov[0]), handles);
if (result == -1) {
PLOG(ERROR) << "sendmsg";
return mojo::ScopedMessagePipeHandle();
Expand Down
13 changes: 6 additions & 7 deletions media/capture/video/chromeos/camera_hal_dispatcher_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include <poll.h>
#include <sys/uio.h>

#include <vector>

#include "base/files/file_path.h"
#include "base/files/file_util.h"
#include "base/posix/eintr_wrapper.h"
Expand All @@ -20,7 +22,6 @@
#include "mojo/edk/embedder/outgoing_broker_client_invitation.h"
#include "mojo/edk/embedder/platform_channel_pair.h"
#include "mojo/edk/embedder/platform_channel_utils_posix.h"
#include "mojo/edk/embedder/platform_handle_vector.h"
#include "mojo/edk/embedder/scoped_platform_handle.h"

namespace media {
Expand Down Expand Up @@ -265,8 +266,7 @@ void CameraHalDispatcherImpl::StartServiceLoop(
}

mojo::edk::ScopedPlatformHandle accepted_fd;
if (mojo::edk::ServerAcceptConnection(proxy_fd_.get(), &accepted_fd,
false) &&
if (mojo::edk::ServerAcceptConnection(proxy_fd_, &accepted_fd, false) &&
accepted_fd.is_valid()) {
VLOG(1) << "Accepted a connection";
// Hardcode pid 0 since it is unused in mojo.
Expand All @@ -282,13 +282,12 @@ void CameraHalDispatcherImpl::StartServiceLoop(
mojo::edk::ConnectionParams(mojo::edk::TransportProtocol::kLegacy,
channel_pair.PassServerHandle()));

mojo::edk::ScopedPlatformHandleVectorPtr handles(
new mojo::edk::PlatformHandleVector{
channel_pair.PassClientHandle().release()});
std::vector<mojo::edk::ScopedPlatformHandle> handles;
handles.emplace_back(channel_pair.PassClientHandle());

struct iovec iov = {const_cast<char*>(token.c_str()), token.length()};
ssize_t result = mojo::edk::PlatformChannelSendmsgWithHandles(
accepted_fd.get(), &iov, 1, handles->data(), handles->size());
accepted_fd, &iov, 1, handles);
if (result == -1) {
PLOG(ERROR) << "sendmsg()";
} else {
Expand Down
1 change: 0 additions & 1 deletion mojo/edk/embedder/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,6 @@ source_set("platform") {
"platform_handle.h",
"platform_handle_utils.h",
"platform_handle_utils_win.cc",
"platform_handle_vector.h",
"platform_shared_buffer.cc",
"platform_shared_buffer.h",
"scoped_platform_handle.h",
Expand Down
59 changes: 27 additions & 32 deletions mojo/edk/embedder/platform_channel_pair_posix_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <sys/uio.h>
#include <unistd.h>
#include <utility>
#include <vector>

#include "base/containers/circular_deque.h"
#include "base/files/file_path.h"
Expand All @@ -24,7 +25,6 @@
#include "base/macros.h"
#include "mojo/edk/embedder/platform_channel_utils_posix.h"
#include "mojo/edk/embedder/platform_handle.h"
#include "mojo/edk/embedder/platform_handle_vector.h"
#include "mojo/edk/embedder/scoped_platform_handle.h"
#include "mojo/edk/test/test_utils.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -90,15 +90,15 @@ TEST_F(PlatformChannelPairPosixTest, NoSigPipe) {
PLOG(WARNING) << "read (expected 0 for EOF)";

// Test our replacement for |write()|/|send()|.
result = PlatformChannelWrite(server_handle.get(), kHello, sizeof(kHello));
result = PlatformChannelWrite(server_handle, kHello, sizeof(kHello));
EXPECT_EQ(-1, result);
if (errno != EPIPE)
PLOG(WARNING) << "write (expected EPIPE)";

// Test our replacement for |writev()|/|sendv()|.
struct iovec iov[2] = {{const_cast<char*>(kHello), sizeof(kHello)},
{const_cast<char*>(kHello), sizeof(kHello)}};
result = PlatformChannelWritev(server_handle.get(), iov, 2);
result = PlatformChannelWritev(server_handle, iov, 2);
EXPECT_EQ(-1, result);
if (errno != EPIPE)
PLOG(WARNING) << "write (expected EPIPE)";
Expand All @@ -113,15 +113,15 @@ TEST_F(PlatformChannelPairPosixTest, SendReceiveData) {
std::string send_string(1 << i, 'A' + i);

EXPECT_EQ(static_cast<ssize_t>(send_string.size()),
PlatformChannelWrite(server_handle.get(), send_string.data(),
PlatformChannelWrite(server_handle, send_string.data(),
send_string.size()));

WaitReadable(client_handle.get());

char buf[10000] = {};
base::circular_deque<PlatformHandle> received_handles;
ssize_t result = PlatformChannelRecvmsg(client_handle.get(), buf,
sizeof(buf), &received_handles);
base::circular_deque<ScopedPlatformHandle> received_handles;
ssize_t result = PlatformChannelRecvmsg(client_handle, buf, sizeof(buf),
&received_handles);
EXPECT_EQ(static_cast<ssize_t>(send_string.size()), result);
EXPECT_EQ(send_string, std::string(buf, static_cast<size_t>(result)));
EXPECT_TRUE(received_handles.empty());
Expand Down Expand Up @@ -149,40 +149,38 @@ TEST_F(PlatformChannelPairPosixTest, SendReceiveFDs) {
// Make |i| files, with the j-th file consisting of j copies of the digit
// |c|.
const char c = '0' + (i % 10);
ScopedPlatformHandleVectorPtr platform_handles(new PlatformHandleVector);
std::vector<ScopedPlatformHandle> platform_handles;
for (size_t j = 1; j <= i; j++) {
base::FilePath unused;
base::ScopedFILE fp(
base::CreateAndOpenTemporaryFileInDir(temp_dir.GetPath(), &unused));
ASSERT_TRUE(fp);
ASSERT_EQ(j, fwrite(std::string(j, c).data(), 1, j, fp.get()));
platform_handles->push_back(
test::PlatformHandleFromFILE(std::move(fp)).release());
ASSERT_TRUE(platform_handles->back().is_valid());
platform_handles.push_back(test::PlatformHandleFromFILE(std::move(fp)));
ASSERT_TRUE(platform_handles.back().is_valid());
}

// Send the FDs (+ "hello").
struct iovec iov = {const_cast<char*>(kHello), sizeof(kHello)};
// We assume that the |sendmsg()| actually sends all the data.
EXPECT_EQ(static_cast<ssize_t>(sizeof(kHello)),
PlatformChannelSendmsgWithHandles(server_handle.get(), &iov, 1,
&platform_handles->at(0),
platform_handles->size()));
PlatformChannelSendmsgWithHandles(server_handle, &iov, 1,
std::move(platform_handles)));

WaitReadable(client_handle.get());

char buf[10000] = {};
base::circular_deque<PlatformHandle> received_handles;
base::circular_deque<ScopedPlatformHandle> received_handles;
// We assume that the |recvmsg()| actually reads all the data.
EXPECT_EQ(static_cast<ssize_t>(sizeof(kHello)),
PlatformChannelRecvmsg(client_handle.get(), buf, sizeof(buf),
PlatformChannelRecvmsg(client_handle, buf, sizeof(buf),
&received_handles));
EXPECT_STREQ(kHello, buf);
EXPECT_EQ(i, received_handles.size());

for (size_t j = 0; !received_handles.empty(); j++) {
for (size_t j = 0; j < received_handles.size(); j++) {
base::ScopedFILE fp(test::FILEFromPlatformHandle(
ScopedPlatformHandle(received_handles.front()), "rb"));
std::move(received_handles.front()), "rb"));
received_handles.pop_front();
ASSERT_TRUE(fp);
rewind(fp.get());
Expand Down Expand Up @@ -213,40 +211,37 @@ TEST_F(PlatformChannelPairPosixTest, AppendReceivedFDs) {
ASSERT_TRUE(fp);
ASSERT_EQ(file_contents.size(),
fwrite(file_contents.data(), 1, file_contents.size(), fp.get()));
ScopedPlatformHandleVectorPtr platform_handles(new PlatformHandleVector);
platform_handles->push_back(
test::PlatformHandleFromFILE(std::move(fp)).release());
ASSERT_TRUE(platform_handles->back().is_valid());
std::vector<ScopedPlatformHandle> platform_handles(1);
platform_handles[0] = test::PlatformHandleFromFILE(std::move(fp));
ASSERT_TRUE(platform_handles.back().is_valid());

// Send the FD (+ "hello").
struct iovec iov = {const_cast<char*>(kHello), sizeof(kHello)};
// We assume that the |sendmsg()| actually sends all the data.
EXPECT_EQ(static_cast<ssize_t>(sizeof(kHello)),
PlatformChannelSendmsgWithHandles(server_handle.get(), &iov, 1,
&platform_handles->at(0),
platform_handles->size()));
PlatformChannelSendmsgWithHandles(server_handle, &iov, 1,
std::move(platform_handles)));
}

WaitReadable(client_handle.get());

// Start with an invalid handle in the deque.
base::circular_deque<PlatformHandle> received_handles;
received_handles.push_back(PlatformHandle());
// Start with an invalid handle in the vector.
base::circular_deque<ScopedPlatformHandle> received_handles;
received_handles.push_back(ScopedPlatformHandle());

char buf[100] = {};
// We assume that the |recvmsg()| actually reads all the data.
EXPECT_EQ(static_cast<ssize_t>(sizeof(kHello)),
PlatformChannelRecvmsg(client_handle.get(), buf, sizeof(buf),
PlatformChannelRecvmsg(client_handle, buf, sizeof(buf),
&received_handles));
EXPECT_STREQ(kHello, buf);
ASSERT_EQ(2u, received_handles.size());
EXPECT_FALSE(received_handles[0].is_valid());
EXPECT_TRUE(received_handles[1].is_valid());

{
base::ScopedFILE fp(test::FILEFromPlatformHandle(
ScopedPlatformHandle(received_handles[1]), "rb"));
received_handles[1] = PlatformHandle();
base::ScopedFILE fp(
test::FILEFromPlatformHandle(std::move(received_handles[1]), "rb"));
ASSERT_TRUE(fp);
rewind(fp.get());
char read_buf[100];
Expand Down
Loading

0 comments on commit 6d83a0b

Please sign in to comment.