Add check on invalid file descriptor at both broker and renderer sides.

content/renderer/pepper_plugin_delegate_impl_unittest.cc

Index: content/renderer/pepper_plugin_delegate_impl_unittest.cc
diff --git a/content/renderer/pepper_plugin_delegate_impl_unittest.cc b/content/renderer/pepper_plugin_delegate_impl_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2812712ca6179664defec66053004bf3438dc33a
--- /dev/null
+++ b/content/renderer/pepper_plugin_delegate_impl_unittest.cc
@@ -0,0 +1,34 @@
+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/renderer/pepper_plugin_delegate_impl.h"
+#include "content/test/mock_render_process.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class PepperPluginDelegateImplTest : public ::testing::Test {
+ protected:
+  MessageLoopForIO message_loop_;
+  // ppapi::proxy::ProxyChannel needs to be initialized on the render process.

[Paweł Hajdan Jr., 2011/11/04 16:58:39]

nit: The comment seems quite confusing now in this place.

Should it say something more like "We need a render process for
ppapi::proxy::ProxyChannel"?

[xhwang, 2011/11/04 22:34:04]

Done.

+  MockRenderProcess mock_process_;
+};
+
+// Try to initialize BrokerDispatcherWrapper with invalid ChannelHandle.

[ddorwin, 2011/11/04 23:10:03]

Are there some positive tests we should add while we're at it? Don't spend too
much time on it.

[xhwang, 2011/11/07 22:00:14]

Done.

+// Initialization should fail.
+TEST_F(PepperPluginDelegateImplTest, BrokerDispatcherWrapperInitFailure) {
+  BrokerDispatcherWrapper dispatcher_wrapper;
+  base::ProcessHandle broker_process_handle = base::kNullProcessHandle;
+  IPC::ChannelHandle channel_handle;  // Invalid by default.
+
+  // Try BrokerDispatcherWrapper.Init() TWICE to trigger the LOG(FATAL)

[ddorwin, 2011/11/04 23:10:03]

This block could probably be simplified. See if something like this makes sense:
An invalid handle should result in a failure (false) and no CHECK, such as the
one in CreatePipe().
Call it twice because without the invalid handle check, the posix code would hit
a one-time path due to a static variable.

[xhwang, 2011/11/07 22:00:14]

Done.

+  // of "Denying attempt to reuse initial IPC channel for ..." in
+  // IPC::Channel::ChannelImpl::CreatePipe() in case invalid ChannelHandle is
+  // not checked properly.  If invalid ChannelHandle is not checked and passed
+  // all the way down to IPC::Channel::ChannelImpl::CreatePipe(), CreatePipe
+  // will be called in client mode with channel_handle.socket.fd == -1.  There
+  // is a static variable "bool used_initial_channel = false" in this function.
+  // In the first test it will be set to true.  In the second test the
+  // LOG(FATAL) error will be triggered.
+  EXPECT_FALSE(dispatcher_wrapper.Init(broker_process_handle, channel_handle));
+  EXPECT_FALSE(dispatcher_wrapper.Init(broker_process_handle, channel_handle));
+}