Skip to content

Commit

Permalink
Use Mojo pipes to signal sync IPC events
Browse files Browse the repository at this point in the history
This transitions legacy sync IPC to use Mojo message pipe handles
for all of its sync event waiting. This is a necessary precursor to
mixing sync legacy IPC with sync Mojo IPC, and is also required to
support correct FIFO between ChannelProxy and Mojo Channel
associated interfaces.

Specifically:

 - Introduces a new IPC::MojoEvent type which is a WaitableEvent-like
   interface around a local message pipe.
 - Moves mojo::SyncHandleRegistry out of internal bindings API and
   exposes it publicly.
 - Replaces most uses of WaitableEvent with MojoEvent for sync IPC
 - Replaces all use of WaitableEvent::WaitMany for sync IPC
   with mojo::SyncHandleRegistry::WatchAllHandles.
 - Cleans up some unnecessary complexity in SyncMessage since
   pump_messages_event() was only being used with a single
   global event that's always signaled.

The system's behavior should be effectively unchanged by this CL,
but legacy sync IPC and mojo sync IPC can now be mixed freely.

BUG=612500
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel;tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2033243003
Cr-Commit-Position: refs/heads/master@{#399848}
  • Loading branch information
krockot authored and Commit bot committed Jun 15, 2016
1 parent 1684132 commit 9791271
Show file tree
Hide file tree
Showing 47 changed files with 1,213 additions and 549 deletions.
2 changes: 2 additions & 0 deletions chrome/browser/printing/cloud_print/test/DEPS
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
include_rules = [
# Service-side mockup tests need service classes to mock.
"+chrome/service",

"+mojo/edk/embedder",
]
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
#include "ipc/ipc_descriptors.h"
#include "ipc/ipc_multiprocess_test.h"
#include "ipc/ipc_switches.h"
#include "mojo/edk/embedder/embedder.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "testing/multiprocess_func_list.h"
Expand Down Expand Up @@ -249,6 +250,9 @@ int CloudPrintMockService_Main(SetExpectationsCallback set_expectations) {
// lifetime.
EXPECT_TRUE(service_process.Initialize(&main_message_loop, state));

// Needed for IPC.
mojo::edk::Init();

MockServiceIPCServer server(&service_process,
service_process.io_task_runner(),
state->GetServiceProcessChannel(),
Expand Down
2 changes: 2 additions & 0 deletions components/nacl.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
'../base/base.gyp:base_static',
'../crypto/crypto.gyp:crypto',
'../ipc/ipc.gyp:ipc',
'../mojo/mojo_edk.gyp:mojo_system_impl',
'../native_client/src/trusted/service_runtime/service_runtime.gyp:sel_main_chrome',
'../ppapi/ppapi_internal.gyp:ppapi_ipc',
'../ppapi/ppapi_internal.gyp:ppapi_shared',
Expand Down Expand Up @@ -284,6 +285,7 @@
},
'dependencies': [
'nacl_common_win64',
'../mojo/mojo_edk.gyp:mojo_system_impl_win64',
'../native_client/src/trusted/service_runtime/service_runtime.gyp:sel_main_chrome64',
'../ppapi/ppapi_internal.gyp:ppapi_shared_win64',
'../ppapi/ppapi_internal.gyp:ppapi_ipc_win64',
Expand Down
3 changes: 3 additions & 0 deletions components/nacl/loader/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ source_set("minimal") {
"//components/nacl/common:minimal",
"//crypto",
"//ipc",
"//mojo/edk/system",
"//native_client/src/trusted/service_runtime:sel_main_chrome",
"//ppapi/c",
"//ppapi/proxy:ipc",
Expand Down Expand Up @@ -116,6 +117,7 @@ if (is_linux) {
"//content/public/common",
"//crypto",
"//ipc",
"//mojo/edk/system",
"//sandbox/linux:sandbox_services",
"//url/ipc:url_ipc",
]
Expand Down Expand Up @@ -206,6 +208,7 @@ if (is_nacl_nonsfi) {
"//components/tracing",
"//content",
"//ipc",
"//mojo/edk/system",
"//native_client/src/nonsfi/irt:nacl_sys_private",
"//native_client/src/nonsfi/loader:elf_loader",

Expand Down
2 changes: 2 additions & 0 deletions components/nacl/loader/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ include_rules = [
"+sandbox/win/src",
"+ppapi/c", # header files only

"+mojo/edk/embedder",

"+native_client/src/include",
"+native_client/src/public",
"+native_client/src/trusted/desc/nacl_desc_quota.h",
Expand Down
4 changes: 4 additions & 0 deletions components/nacl/loader/nacl_helper_linux.cc
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "content/public/common/zygote_fork_delegate_linux.h"
#include "ipc/ipc_descriptors.h"
#include "ipc/ipc_switches.h"
#include "mojo/edk/embedder/embedder.h"
#include "sandbox/linux/services/credentials.h"
#include "sandbox/linux/services/namespace_sandbox.h"

Expand Down Expand Up @@ -118,6 +119,9 @@ void BecomeNaClLoader(base::ScopedFD browser_fd,
base::GlobalDescriptors::GetInstance()->Set(kPrimaryIPCChannel,
browser_fd.release());

// The Mojo EDK must be initialized before using IPC.
mojo::edk::Init();

base::MessageLoopForIO main_message_loop;
#if defined(OS_NACL_NONSFI)
CHECK(uses_nonsfi_mode);
Expand Down
4 changes: 4 additions & 0 deletions components/nacl/loader/nacl_main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,15 @@
#include "components/nacl/loader/nacl_main_platform_delegate.h"
#include "content/public/common/content_switches.h"
#include "content/public/common/main_function_params.h"
#include "mojo/edk/embedder/embedder.h"

// main() routine for the NaCl loader process.
int NaClMain(const content::MainFunctionParams& parameters) {
const base::CommandLine& parsed_command_line = parameters.command_line;

// The Mojo EDK must be initialized before using IPC.
mojo::edk::Init();

// The main thread of the plugin services IO.
base::MessageLoopForIO main_message_loop;
base::PlatformThread::SetName("CrNaClMain");
Expand Down
16 changes: 16 additions & 0 deletions components/nacl_nonsfi.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,10 @@
'-lgles2_utils_nacl',
'-lgpu_ipc_nacl',
'-lipc_nacl_nonsfi',
'-lmojo_cpp_bindings_nacl',
'-lmojo_cpp_system_nacl',
'-lmojo_public_system_nacl',
'-lmojo_system_impl_nacl',
'-lnacl_helper_nonsfi_sandbox',
'-lplatform',
'-lppapi_ipc_nacl',
Expand All @@ -96,6 +100,10 @@
'>(tc_lib_dir_nonsfi_helper32)/libgles2_utils_nacl.a',
'>(tc_lib_dir_nonsfi_helper32)/libgpu_ipc_nacl.a',
'>(tc_lib_dir_nonsfi_helper32)/libipc_nacl_nonsfi.a',
'>(tc_lib_dir_nonsfi_helper32)/libmojo_cpp_bindings_nacl.a',
'>(tc_lib_dir_nonsfi_helper32)/libmojo_cpp_system_nacl.a',
'>(tc_lib_dir_nonsfi_helper32)/libmojo_public_system_nacl.a',
'>(tc_lib_dir_nonsfi_helper32)/libmojo_system_impl_nacl.a',
'>(tc_lib_dir_nonsfi_helper32)/libnacl_helper_nonsfi_sandbox.a',
'>(tc_lib_dir_nonsfi_helper32)/libplatform.a',
'>(tc_lib_dir_nonsfi_helper32)/libppapi_ipc_nacl.a',
Expand All @@ -120,6 +128,10 @@
'>(tc_lib_dir_nonsfi_helper_arm)/libgles2_utils_nacl.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libgpu_ipc_nacl.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libipc_nacl_nonsfi.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libmojo_cpp_bindings_nacl.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libmojo_cpp_system_nacl.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libmojo_public_system_nacl.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libmojo_system_impl_nacl.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libnacl_helper_nonsfi_sandbox.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libplatform.a',
'>(tc_lib_dir_nonsfi_helper_arm)/libppapi_ipc_nacl.a',
Expand All @@ -136,6 +148,10 @@
'../base/base_nacl.gyp:base_nacl_nonsfi',
'../content/content_nacl_nonsfi.gyp:content_common_nacl_nonsfi',
'../ipc/ipc_nacl.gyp:ipc_nacl_nonsfi',
'../mojo/mojo_edk_nacl.gyp:mojo_system_impl_nacl',
'../mojo/mojo_public_nacl.gyp:mojo_cpp_bindings_nacl',
'../mojo/mojo_public_nacl.gyp:mojo_cpp_system_nacl',
'../mojo/mojo_public_nacl.gyp:mojo_public_system_nacl',
'../native_client/src/nonsfi/irt/irt.gyp:nacl_sys_private',
'../native_client/src/nonsfi/loader/loader.gyp:elf_loader',
'../native_client/src/untrusted/nacl/nacl.gyp:nacl_lib_newlib',
Expand Down
2 changes: 1 addition & 1 deletion gpu/gpu.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -315,8 +315,8 @@
'type': '<(gtest_target_type)',
'dependencies': [
'../base/base.gyp:base',
'../base/base.gyp:run_all_unittests',
'../base/base.gyp:test_support_base',
'../ipc/ipc.gyp:ipc_run_all_unittests',
'../ipc/ipc.gyp:test_support_ipc',
'../skia/skia.gyp:skia',
'../testing/gtest.gyp:gtest',
Expand Down
2 changes: 1 addition & 1 deletion gpu/ipc/service/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -136,13 +136,13 @@ test("gpu_ipc_service_unittests") {
":service",
":test_support",
"//base",
"//base/test:run_all_unittests",
"//base/test:test_support",
"//gpu/command_buffer/common",
"//gpu/command_buffer/common:gles2_utils",
"//gpu/command_buffer/service",
"//gpu/config",
"//gpu/ipc/common",
"//ipc:run_all_unittests",
"//ipc:test_support",
"//skia",
"//testing/gmock",
Expand Down
20 changes: 19 additions & 1 deletion ipc/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ component("ipc") {
"message_filter_router.h",
"message_router.cc",
"message_router.h",
"mojo_event.cc",
"mojo_event.h",
"param_traits_log_macros.h",
"param_traits_macros.h",
"param_traits_read_macros.h",
Expand Down Expand Up @@ -127,12 +129,14 @@ component("ipc") {

public_deps = [
":param_traits",
"//mojo/public/cpp/system",
]
deps = [
"//base",

# TODO(viettrungluu): Needed for base/lazy_instance.h, which is suspect.
"//base/third_party/dynamic_annotations",
"//mojo/public/cpp/bindings",
]

if (is_win || is_mac) {
Expand All @@ -155,6 +159,20 @@ source_set("param_traits") {
}

if (!is_ios) {
source_set("run_all_unittests") {
testonly = true

sources = [
"run_all_unittests.cc",
]

deps = [
"//base",
"//base/test:test_support",
"//mojo/edk/system",
]
}

test("ipc_tests") {
sources = [
"attachment_broker_mac_unittest.cc",
Expand All @@ -173,7 +191,6 @@ if (!is_ios) {
"ipc_test_message_generator.cc",
"ipc_test_message_generator.h",
"ipc_test_messages.h",
"run_all_unittests.cc",
"sync_socket_unittest.cc",
]

Expand All @@ -196,6 +213,7 @@ if (!is_ios) {

deps = [
":ipc",
":run_all_unittests",
":test_support",
"//base",
"//base:i18n",
Expand Down
8 changes: 8 additions & 0 deletions ipc/DEPS
Original file line number Diff line number Diff line change
@@ -1,6 +1,14 @@
include_rules = [
"+crypto",
"+mojo/edk/embedder",
"+mojo/public",
# For ipc_channel_nacl.cc:
"+native_client/src/public",
"+sandbox/mac/seatbelt.h",
]

specific_include_rules = {
"run_all_unittests\.cc": [
"+mojo/edk/embedder",
]
}
22 changes: 21 additions & 1 deletion ipc/ipc.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@
'../base/base.gyp:base',
# TODO(viettrungluu): Needed for base/lazy_instance.h, which is suspect.
'../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations',
'../mojo/mojo_public.gyp:mojo_cpp_bindings',
'../mojo/mojo_public.gyp:mojo_cpp_system',
],
# TODO(gregoryd): direct_dependent_settings should be shared with the
# 64-bit target, but it doesn't work due to a bug in gyp
Expand All @@ -36,11 +38,28 @@
}],
],
},
{
'target_name': 'ipc_run_all_unittests',
'type': 'static_library',
'dependencies': [
'../base/base.gyp:base',
'../base/base.gyp:test_support_base',
'../mojo/mojo_edk.gyp:mojo_system_impl',
'../testing/gtest.gyp:gtest',
],
'include_dirs': [
'..',
],
'sources': [
'run_all_unittests.cc',
],
},
{
'target_name': 'ipc_tests',
'type': '<(gtest_target_type)',
'dependencies': [
'ipc',
'ipc_run_all_unittests',
'test_support_ipc',
'../base/base.gyp:base',
'../base/base.gyp:base_i18n',
Expand Down Expand Up @@ -70,7 +89,6 @@
'ipc_test_messages.h',
'ipc_test_message_generator.cc',
'ipc_test_message_generator.h',
'run_all_unittests.cc',
'sync_socket_unittest.cc',
'unix_domain_socket_util_unittest.cc',
],
Expand Down Expand Up @@ -166,6 +184,8 @@
# suspect.
'../base/third_party/dynamic_annotations/dynamic_annotations.gyp:dynamic_annotations_win64',
'../crypto/crypto.gyp:crypto_nacl_win64',
'../mojo/mojo_public.gyp:mojo_cpp_bindings_win64',
'../mojo/mojo_public.gyp:mojo_cpp_system_win64',
],
# TODO(gregoryd): direct_dependent_settings should be shared with the
# 32-bit target, but it doesn't work due to a bug in gyp
Expand Down
2 changes: 2 additions & 0 deletions ipc/ipc.gypi
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@
'message_filter_router.h',
'message_router.cc',
'message_router.h',
'mojo_event.cc',
'mojo_event.h',
'param_traits_log_macros.h',
'param_traits_macros.h',
'param_traits_read_macros.h',
Expand Down
4 changes: 4 additions & 0 deletions ipc/ipc_nacl.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
},
'dependencies': [
'../base/base_nacl.gyp:base_nacl',
'../mojo/mojo_public_nacl.gyp:mojo_cpp_bindings_nacl',
'../mojo/mojo_public_nacl.gyp:mojo_cpp_system_nacl',
],
},
{
Expand Down Expand Up @@ -55,6 +57,8 @@
],
'dependencies': [
'../base/base_nacl.gyp:base_nacl_nonsfi',
'../mojo/mojo_public_nacl.gyp:mojo_cpp_bindings_nacl',
'../mojo/mojo_public_nacl.gyp:mojo_cpp_system_nacl',
],
},
],
Expand Down
Loading

0 comments on commit 9791271

Please sign in to comment.