Remove RawChannel::Init() and Channel::Init() failure cases. I don't know how they'd ever fail, unless the FD/handle passed in was bad (in which case our code is buggy, and we should CHECK-fail). (As a consequence I don't know how to reasonably test these failure code paths either.) R=yzshen@chromium.org Review URL: https://codereview.chromium.org/852113002
diff --git a/mojo/edk/embedder/embedder.cc b/mojo/edk/embedder/embedder.cc index 622b3b7..1d12c1e 100644 --- a/mojo/edk/embedder/embedder.cc +++ b/mojo/edk/embedder/embedder.cc
@@ -36,14 +36,7 @@ DCHECK(internal::g_core); scoped_refptr<system::Channel> channel = new system::Channel(internal::g_core->platform_support()); - if (!channel->Init(system::RawChannel::Create(platform_handle.Pass()))) { - // This is very unusual (e.g., maybe |platform_handle| was invalid or we - // reached some system resource limit). - LOG(ERROR) << "Channel::Init() failed"; - // Return null, since |Shutdown()| shouldn't be called in this case. - return 0; - } - + channel->Init(system::RawChannel::Create(platform_handle.Pass())); channel->SetBootstrapEndpoint(channel_endpoint); DCHECK(internal::g_channel_manager); @@ -150,11 +143,7 @@ // TODO(vtl): Write tests for this. void DestroyChannel(ChannelInfo* channel_info) { DCHECK(channel_info); - if (!channel_info->channel_id) { - // Presumably, |Init()| on the channel failed. - return; - } - + DCHECK(channel_info->channel_id); DCHECK(internal::g_channel_manager); // This will destroy the channel synchronously if called from the channel // thread.
diff --git a/mojo/edk/system/channel.cc b/mojo/edk/system/channel.cc index 8889919..71d7208 100644 --- a/mojo/edk/system/channel.cc +++ b/mojo/edk/system/channel.cc
@@ -36,7 +36,7 @@ channel_manager_(nullptr) { } -bool Channel::Init(scoped_ptr<RawChannel> raw_channel) { +void Channel::Init(scoped_ptr<RawChannel> raw_channel) { DCHECK(creation_thread_checker_.CalledOnValidThread()); DCHECK(raw_channel); @@ -44,14 +44,8 @@ // becomes thread-safe. DCHECK(!is_running_); raw_channel_ = raw_channel.Pass(); - - if (!raw_channel_->Init(this)) { - raw_channel_.reset(); - return false; - } - + raw_channel_->Init(this); is_running_ = true; - return true; } void Channel::SetChannelManager(ChannelManager* channel_manager) {
diff --git a/mojo/edk/system/channel.h b/mojo/edk/system/channel.h index 64acb7d..3961859 100644 --- a/mojo/edk/system/channel.h +++ b/mojo/edk/system/channel.h
@@ -60,9 +60,8 @@ // This must be called on the creation thread before any other methods are // called, and before references to this object are given to any other - // threads. |raw_channel| should be uninitialized. Returns true on success. On - // failure, no other methods should be called (including |Shutdown()|). - bool Init(scoped_ptr<RawChannel> raw_channel); + // threads. |raw_channel| should be uninitialized. + void Init(scoped_ptr<RawChannel> raw_channel); // Sets the channel manager associated with this channel. This should be set // at most once and only called before |WillShutdownSoon()| (and
diff --git a/mojo/edk/system/channel_unittest.cc b/mojo/edk/system/channel_unittest.cc index 741c99f..e82b321 100644 --- a/mojo/edk/system/channel_unittest.cc +++ b/mojo/edk/system/channel_unittest.cc
@@ -22,17 +22,9 @@ namespace system { namespace { -enum Tristate { TRISTATE_UNKNOWN = -1, TRISTATE_FALSE = 0, TRISTATE_TRUE = 1 }; - -Tristate BoolToTristate(bool b) { - return b ? TRISTATE_TRUE : TRISTATE_FALSE; -} - class ChannelTest : public testing::Test { public: - ChannelTest() - : io_thread_(base::TestIOThread::kAutoStart), - init_result_(TRISTATE_UNKNOWN) {} + ChannelTest() : io_thread_(base::TestIOThread::kAutoStart) {} ~ChannelTest() override {} void SetUp() override { @@ -51,9 +43,7 @@ CHECK(raw_channel_); CHECK(channel_); - CHECK_EQ(init_result_, TRISTATE_UNKNOWN); - - init_result_ = BoolToTristate(channel_->Init(raw_channel_.Pass())); + channel_->Init(raw_channel_.Pass()); } void ShutdownChannelOnIOThread() { @@ -68,7 +58,6 @@ scoped_ptr<RawChannel>* mutable_raw_channel() { return &raw_channel_; } Channel* channel() { return channel_.get(); } scoped_refptr<Channel>* mutable_channel() { return &channel_; } - Tristate init_result() const { return init_result_; } private: void SetUpOnIOThread() { @@ -85,8 +74,6 @@ embedder::ScopedPlatformHandle other_platform_handle_; scoped_refptr<Channel> channel_; - Tristate init_result_; - DISALLOW_COPY_AND_ASSIGN(ChannelTest); }; @@ -101,7 +88,6 @@ io_thread()->PostTaskAndWait( FROM_HERE, base::Bind(&ChannelTest::InitChannelOnIOThread, base::Unretained(this))); - EXPECT_EQ(TRISTATE_TRUE, init_result()); io_thread()->PostTaskAndWait( FROM_HERE, base::Bind(&ChannelTest::ShutdownChannelOnIOThread, @@ -112,74 +98,6 @@ *mutable_channel() = nullptr; } -// ChannelTest.InitFails ------------------------------------------------------- - -class MockRawChannelOnInitFails : public RawChannel { - public: - MockRawChannelOnInitFails() : on_init_called_(false) {} - ~MockRawChannelOnInitFails() override {} - - // |RawChannel| public methods: - size_t GetSerializedPlatformHandleSize() const override { return 0; } - - private: - // |RawChannel| protected methods: - IOResult Read(size_t*) override { - CHECK(false); - return IO_FAILED_UNKNOWN; - } - IOResult ScheduleRead() override { - CHECK(false); - return IO_FAILED_UNKNOWN; - } - embedder::ScopedPlatformHandleVectorPtr GetReadPlatformHandles( - size_t, - const void*) override { - CHECK(false); - return embedder::ScopedPlatformHandleVectorPtr(); - } - IOResult WriteNoLock(size_t*, size_t*) override { - CHECK(false); - return IO_FAILED_UNKNOWN; - } - IOResult ScheduleWriteNoLock() override { - CHECK(false); - return IO_FAILED_UNKNOWN; - } - bool OnInit() override { - EXPECT_FALSE(on_init_called_); - on_init_called_ = true; - return false; - } - void OnShutdownNoLock(scoped_ptr<ReadBuffer>, - scoped_ptr<WriteBuffer>) override { - CHECK(false); - } - - bool on_init_called_; - - DISALLOW_COPY_AND_ASSIGN(MockRawChannelOnInitFails); -}; - -TEST_F(ChannelTest, InitFails) { - io_thread()->PostTaskAndWait(FROM_HERE, - base::Bind(&ChannelTest::CreateChannelOnIOThread, - base::Unretained(this))); - ASSERT_TRUE(channel()); - - ASSERT_TRUE(raw_channel()); - mutable_raw_channel()->reset(new MockRawChannelOnInitFails()); - - io_thread()->PostTaskAndWait( - FROM_HERE, - base::Bind(&ChannelTest::InitChannelOnIOThread, base::Unretained(this))); - EXPECT_EQ(TRISTATE_FALSE, init_result()); - - // Should destroy |Channel| with no |Shutdown()| (on not-the-I/O-thread). - EXPECT_TRUE(channel()->HasOneRef()); - *mutable_channel() = nullptr; -} - // ChannelTest.CloseBeforeAttachAndRun ----------------------------------------- TEST_F(ChannelTest, CloseBeforeRun) { @@ -191,7 +109,6 @@ io_thread()->PostTaskAndWait( FROM_HERE, base::Bind(&ChannelTest::InitChannelOnIOThread, base::Unretained(this))); - EXPECT_EQ(TRISTATE_TRUE, init_result()); scoped_refptr<ChannelEndpoint> channel_endpoint; scoped_refptr<MessagePipe> mp( @@ -219,7 +136,6 @@ io_thread()->PostTaskAndWait( FROM_HERE, base::Bind(&ChannelTest::InitChannelOnIOThread, base::Unretained(this))); - EXPECT_EQ(TRISTATE_TRUE, init_result()); scoped_refptr<ChannelEndpoint> channel_endpoint; scoped_refptr<MessagePipe> mp( @@ -262,7 +178,6 @@ io_thread()->PostTaskAndWait( FROM_HERE, base::Bind(&ChannelTest::InitChannelOnIOThread, base::Unretained(this))); - EXPECT_EQ(TRISTATE_TRUE, init_result()); scoped_refptr<ChannelEndpoint> channel_endpoint; scoped_refptr<MessagePipe> mp(
diff --git a/mojo/edk/system/message_pipe_test_utils.cc b/mojo/edk/system/message_pipe_test_utils.cc index ba6a0e0..f227543 100644 --- a/mojo/edk/system/message_pipe_test_utils.cc +++ b/mojo/edk/system/message_pipe_test_utils.cc
@@ -74,7 +74,7 @@ // Create and initialize |Channel|. channel_ = new Channel(platform_support_); - CHECK(channel_->Init(RawChannel::Create(platform_handle.Pass()))); + channel_->Init(RawChannel::Create(platform_handle.Pass())); // Start the bootstrap endpoint. // Note: On the "server" (parent process) side, we need not attach/run the
diff --git a/mojo/edk/system/raw_channel.cc b/mojo/edk/system/raw_channel.cc index aa9c95f..aff1110 100644 --- a/mojo/edk/system/raw_channel.cc +++ b/mojo/edk/system/raw_channel.cc
@@ -171,7 +171,7 @@ DCHECK(!weak_ptr_factory_.HasWeakPtrs()); } -bool RawChannel::Init(Delegate* delegate) { +void RawChannel::Init(Delegate* delegate) { DCHECK(delegate); DCHECK(!delegate_); @@ -188,13 +188,7 @@ DCHECK(!write_buffer_); write_buffer_.reset(new WriteBuffer(GetSerializedPlatformHandleSize())); - if (!OnInit()) { - delegate_ = nullptr; - message_loop_for_io_ = nullptr; - read_buffer_.reset(); - write_buffer_.reset(); - return false; - } + OnInit(); IOResult io_result = ScheduleRead(); if (io_result != IO_PENDING) { @@ -204,10 +198,8 @@ FROM_HERE, base::Bind(&RawChannel::OnReadCompleted, weak_ptr_factory_.GetWeakPtr(), io_result, 0)); } - - // ScheduleRead() failure is treated as a read failure (by notifying the - // delegate), not as an init failure. - return true; + // Note: |ScheduleRead()| failure is treated as a read failure (by notifying + // the delegate), not an initialization failure. } void RawChannel::Shutdown() {
diff --git a/mojo/edk/system/raw_channel.h b/mojo/edk/system/raw_channel.h index 62e13ff..f567767 100644 --- a/mojo/edk/system/raw_channel.h +++ b/mojo/edk/system/raw_channel.h
@@ -87,9 +87,8 @@ // This must be called (on an I/O thread) before this object is used. Does // *not* take ownership of |delegate|. Both the I/O thread and |delegate| must // remain alive until |Shutdown()| is called (unless this fails); |delegate| - // will no longer be used after |Shutdown()|. Returns true on success. On - // failure, |Shutdown()| should *not* be called. - bool Init(Delegate* delegate); + // will no longer be used after |Shutdown()|. + void Init(Delegate* delegate); // This must be called (on the I/O thread) before this object is destroyed. void Shutdown(); @@ -279,7 +278,7 @@ virtual IOResult ScheduleWriteNoLock() = 0; // Must be called on the I/O thread WITHOUT |write_lock_| held. - virtual bool OnInit() = 0; + virtual void OnInit() = 0; // On shutdown, passes the ownership of the buffers to subclasses, which may // want to preserve them if there are pending read/write. Must be called on // the I/O thread under |write_lock_|.
diff --git a/mojo/edk/system/raw_channel_posix.cc b/mojo/edk/system/raw_channel_posix.cc index 71ec02f..078f8cb 100644 --- a/mojo/edk/system/raw_channel_posix.cc +++ b/mojo/edk/system/raw_channel_posix.cc
@@ -54,7 +54,7 @@ IOResult WriteNoLock(size_t* platform_handles_written, size_t* bytes_written) override; IOResult ScheduleWriteNoLock() override; - bool OnInit() override; + void OnInit() override; void OnShutdownNoLock(scoped_ptr<ReadBuffer> read_buffer, scoped_ptr<WriteBuffer> write_buffer) override; @@ -310,7 +310,7 @@ return IO_FAILED_UNKNOWN; } -bool RawChannelPosix::OnInit() { +void RawChannelPosix::OnInit() { DCHECK_EQ(base::MessageLoop::current(), message_loop_for_io()); DCHECK(!read_watcher_); @@ -318,18 +318,12 @@ DCHECK(!write_watcher_); write_watcher_.reset(new base::MessageLoopForIO::FileDescriptorWatcher()); - if (!message_loop_for_io()->WatchFileDescriptor( - fd_.get().fd, true, base::MessageLoopForIO::WATCH_READ, - read_watcher_.get(), this)) { - // TODO(vtl): I'm not sure |WatchFileDescriptor()| actually fails cleanly - // (in the sense of returning the message loop's state to what it was before - // it was called). - read_watcher_.reset(); - write_watcher_.reset(); - return false; - } - - return true; + // I don't know how this can fail (unless |fd_| is bad, in which case it's a + // bug in our code). I also don't know if |WatchFileDescriptor()| actually + // fails cleanly. + CHECK(message_loop_for_io()->WatchFileDescriptor( + fd_.get().fd, true, base::MessageLoopForIO::WATCH_READ, + read_watcher_.get(), this)); } void RawChannelPosix::OnShutdownNoLock(
diff --git a/mojo/edk/system/raw_channel_unittest.cc b/mojo/edk/system/raw_channel_unittest.cc index 5f86e7e..a8cbcba 100644 --- a/mojo/edk/system/raw_channel_unittest.cc +++ b/mojo/edk/system/raw_channel_unittest.cc
@@ -59,7 +59,7 @@ } void InitOnIOThread(RawChannel* raw_channel, RawChannel::Delegate* delegate) { - CHECK(raw_channel->Init(delegate)); + raw_channel->Init(delegate); } bool WriteTestMessageToHandle(const embedder::PlatformHandle& handle,
diff --git a/mojo/edk/system/raw_channel_win.cc b/mojo/edk/system/raw_channel_win.cc index 27b9ccf..7ec7ad7 100644 --- a/mojo/edk/system/raw_channel_win.cc +++ b/mojo/edk/system/raw_channel_win.cc
@@ -168,7 +168,7 @@ IOResult WriteNoLock(size_t* platform_handles_written, size_t* bytes_written) override; IOResult ScheduleWriteNoLock() override; - bool OnInit() override; + void OnInit() override; void OnShutdownNoLock(scoped_ptr<ReadBuffer> read_buffer, scoped_ptr<WriteBuffer> write_buffer) override; @@ -521,20 +521,19 @@ return io_result; } -bool RawChannelWin::OnInit() { +void RawChannelWin::OnInit() { DCHECK_EQ(base::MessageLoop::current(), message_loop_for_io()); DCHECK(handle_.is_valid()); - if (skip_completion_port_on_success_ && - !g_vista_or_higher_functions.Get().SetFileCompletionNotificationModes( - handle_.get().handle, FILE_SKIP_COMPLETION_PORT_ON_SUCCESS)) { - return false; + if (skip_completion_port_on_success_) { + // I don't know how this can fail (unless |handle_| is bad, in which case + // it's a bug in our code). + CHECK(g_vista_or_higher_functions.Get().SetFileCompletionNotificationModes( + handle_.get().handle, FILE_SKIP_COMPLETION_PORT_ON_SUCCESS)); } DCHECK(!io_handler_); io_handler_ = new RawChannelIOHandler(this, handle_.Pass()); - - return true; } void RawChannelWin::OnShutdownNoLock(scoped_ptr<ReadBuffer> read_buffer,
diff --git a/mojo/edk/system/remote_message_pipe_unittest.cc b/mojo/edk/system/remote_message_pipe_unittest.cc index bf608fb..1860c23 100644 --- a/mojo/edk/system/remote_message_pipe_unittest.cc +++ b/mojo/edk/system/remote_message_pipe_unittest.cc
@@ -127,8 +127,8 @@ CHECK(!channels_[channel_index]); channels_[channel_index] = new Channel(&platform_support_); - CHECK(channels_[channel_index]->Init( - RawChannel::Create(platform_handles_[channel_index].Pass()))); + channels_[channel_index]->Init( + RawChannel::Create(platform_handles_[channel_index].Pass())); } void BootstrapChannelEndpointsOnIOThread(scoped_refptr<ChannelEndpoint> ep0,