Add TestConnectFailure, TestGetHandleFailure and TestConnectAndPipe to PPAPI Broker UI test.

ppapi/tests/test_broker.cc

Index: ppapi/tests/test_broker.cc
diff --git a/ppapi/tests/test_broker.cc b/ppapi/tests/test_broker.cc
index b1d5b028c58cf2adedbc4f45237335461277dfb4..58f18149387e40ba79e8d45892d2d9959cf02f65 100644
--- a/ppapi/tests/test_broker.cc
+++ b/ppapi/tests/test_broker.cc
@@ -4,13 +4,231 @@
 
 #include "ppapi/tests/test_broker.h"
 
+#include <cstdio>
+#include <cstring>
+#include <fstream>
+#include <limits>
+
+#include "base/memory/scoped_ptr.h"
 #include "ppapi/c/pp_errors.h"
+#include "ppapi/c/trusted/ppp_broker.h"
 #include "ppapi/c/trusted/ppb_broker_trusted.h"
-#include "ppapi/cpp/module.h"
+#include "ppapi/tests/test_utils.h"
 #include "ppapi/tests/testing_instance.h"
 
+#if defined(OS_WIN)
+#include "Windows.h"
+#elif !defined(OS_POSIX)
+#error Not implemented. Don't remove as it's only checked here in this file.
+#endif
+
 REGISTER_TEST_CASE(Broker);
 
+namespace {
+
+const char kHelloMessage[] = "Hello Plugin! This is Broker!";
+const char kBrokerUnsandboxed[] = "Broker is Unsandboxed!";
+
+#if defined(OS_WIN)
+typedef HANDLE PlatformFile;
+const PlatformFile kInvalidPlatformFileValue = INVALID_HANDLE_VALUE;
+#elif defined(OS_POSIX)
+typedef int PlatformFile;
+const PlatformFile kInvalidPlatformFileValue = -1;
+#endif
+
+int32_t PlatformFileToInt(PlatformFile handle) {
+#if defined(OS_WIN)
+  return static_cast<int32_t>(reinterpret_cast<intptr_t>(handle));
+#elif defined(OS_POSIX)
+  return handle;
+#endif
+}
+
+PlatformFile IntToPlatformFile(int32_t handle) {
+#if defined(OS_WIN)
+  return reinterpret_cast<HANDLE>(static_cast<intptr_t>(handle));
+#elif defined(OS_POSIX)
+  return handle;
+#endif
+}
+
+const int32_t kInvalidHandle = PlatformFileToInt(kInvalidPlatformFileValue);

[ddorwin, 2011/11/14 18:19:39]

This would be a bad idea in non-test code. It's actually only used in one
function. Also, since PlatformFile is defined in this file, you could just
define int32_t values in the constant definitions above and eliminate
PlatformFileToInt().

[xhwang, 2011/11/14 22:24:30]

Done.

+
+bool ReadFromPlatformFile(PlatformFile file, char* buffer,

[ddorwin, 2011/11/14 18:19:39]

parameter order should be input then outputs. Change the write function too so
there is a consistent ordering.

[xhwang, 2011/11/14 22:24:30]

Done.

+                          size_t bytes_to_read, size_t& bytes_read) {

[ddorwin, 2011/11/14 18:19:39]

output parameters must be pointers per coding style. same below.

[xhwang, 2011/11/14 22:24:30]

Done.

+#if defined(OS_WIN)
+  assert(bytes_to_read < std::numeric_limits<DWORD>::max());
+  DWORD read = 0;
+  if (!::ReadFile(file, buffer, static_cast<DWORD>(bytes_to_read), &read, NULL))
+    return false;
+  bytes_read = static_cast<size_t>(read);
+#elif defined(OS_POSIX)
+  assert(bytes_to_read <
+         static_cast<size_t>(std::numeric_limits<ssize_t>::max()));
+  ssize_t ret = ::read(file, buffer, bytes_to_read);
+  if (ret == -1)
+    return false;
+  bytes_read = static_cast<size_t>(ret);
+#endif
+  return true;

[ddorwin, 2011/11/14 18:19:39]

maybe a newline for readability and to separate from the ifdef block. same
below.

[xhwang, 2011/11/14 22:24:30]

Done.

+}
+
+bool WriteToPlatformFile(PlatformFile file, const char* buffer,
+                         size_t bytes_to_write, size_t& bytes_written) {
+#if defined(OS_WIN)
+  assert(bytes_to_write < std::numeric_limits<DWORD>::max());
+  DWORD written = 0;
+  if (!::WriteFile(file, buffer, static_cast<DWORD>(bytes_to_write), &written,
+                   NULL))
+    return false;
+  bytes_written = static_cast<size_t>(written);
+#elif defined(OS_POSIX)
+  assert(bytes_to_write <
+         static_cast<size_t>(std::numeric_limits<ssize_t>::max()));
+  ssize_t ret = ::write(file, buffer, bytes_to_write);
+  if (ret == -1)
+      return false;

[ddorwin, 2011/11/14 18:19:39]

indent

[xhwang, 2011/11/14 22:24:30]

Done.

+  bytes_written = static_cast<size_t>(ret);
+#endif
+  return true;
+}
+
+bool ClosePlatformFile(PlatformFile file) {
+#if defined(OS_WIN)
+  return !!::CloseHandle(file);
+#elif defined(OS_POSIX)
+  return !::close(file);
+#endif
+}
+
+bool WriteMessage(PlatformFile file, const char* message, size_t message_len) {
+  size_t bytes_written = 0;
+  if (!WriteToPlatformFile(file, message, message_len, bytes_written) ||

[ddorwin, 2011/11/14 18:19:39]

This might be easier to read if written positively. And it can be simplified to
just "return success && equal;"

[xhwang, 2011/11/14 22:24:30]

I like this :-)

[xhwang, 2011/11/14 22:24:30]

Done.

+      bytes_written != message_len)
+    return false;
+
+  return true;
+}
+
+bool ReadMessage(PlatformFile file, char* message, size_t message_len) {

[ddorwin, 2011/11/14 18:19:39]

parameter ordering.

[xhwang, 2011/11/14 22:24:30]

Done.

+  size_t bytes_read = 0;
+  if (!ReadFromPlatformFile(file, message, message_len, bytes_read) ||
+      bytes_read != message_len)
+    return false;
+
+  return true;
+}
+
+bool VerifyMessage(PlatformFile file, const char* message_sent,
+                   size_t length_sent) {

[ddorwin, 2011/11/14 18:19:39]

length_sent doesn't make sense in the context of this function. Maybe in the
callers, but not here.

You might reorder these parameters to match the new ReadMessage order too.

[xhwang, 2011/11/14 22:24:30]

Done.

+  scoped_array<char> message_received(new char[length_sent]);
+
+  if (!ReadMessage(file, message_received.get(), length_sent))
+    return false;
+
+  return !::strcmp(message_received.get(), message_sent);
+}
+
+bool VerifyIsUnsandboxed() {
+  FILE* file = NULL;
+
+#if defined(OS_WIN)
+  wchar_t temp_path[MAX_PATH] = {'\0'};
+  wchar_t file_name[MAX_PATH] = {'\0'};
+  if (!::GetTempPath(MAX_PATH, temp_path) ||
+      !::GetTempFileName(temp_path, L"test_pepper_broker", 0, file_name) ||
+      ::_wfopen_s(&file, file_name, L"w"))
+    return false;
+#elif defined(OS_POSIX)
+  char file_name[] = "/tmp/test_pepper_broker_XXXXXX";
+  int fd = ::mkstemp(file_name);
+  if (-1 == fd)
+    return false;
+
+  file = ::fdopen(fd, "w");
+  if (!file) {
+    ::unlink(file_name);

[ddorwin, 2011/11/14 18:19:39]

why unlink instead of remove? Maybe comment.

[xhwang, 2011/11/14 22:24:30]

If path does not name a directory, remove(path) shall be equivalent to
unlink(path). Will change.

[xhwang, 2011/11/14 22:24:30]

Done.

+    ::close(fd);
+    return false;
+  }
+#endif
+
+  const char text[] = "Verify that the broker is unsandboxed.";
+  const size_t text_len = sizeof(text);
+  if (text_len != ::fwrite(text, 1, text_len, file)) {
+    ::fclose(file);

[ddorwin, 2011/11/14 18:19:39]

does it need to be deleted in case part of it was written? There are other cases
below where we leave the file too. Oh well, I guess.
This would all be easier if we could use EXPECT_*. *sigh*

[xhwang, 2011/11/14 22:24:30]

Done.

+    return false;
+  }
+
+  if (::fclose(file))
+    return false;
+

[ddorwin, 2011/11/14 18:19:39]

Clear file just to be safe.

[xhwang, 2011/11/14 22:24:30]

Add comment at the top of the function to warn about possible file leak and that
it'll be fixed once these tests are moved to Google Test.

[ddorwin, 2011/11/14 22:34:50]

Sorry, I meant:
file = NULL;
since we're going to re-assign it below for a read.

[xhwang, 2011/11/14 22:50:19]

Done.

+#if defined(OS_WIN)
+  if (::_wfopen_s(&file, file_name, L"r"))
+    return false;
+#elif defined(OS_POSIX)
+  file = ::fopen(file_name, "r");
+  if (!file)
+    return false;
+#endif
+
+  scoped_array<char> buffer(new char[text_len]);
+  if (text_len != ::fread(buffer.get(), 1, text_len, file)) {
+    ::fclose(file);
+    return false;
+  }
+
+  if (::fclose(file))
+    return false;
+
+#if defined(OS_WIN)
+  if (!::DeleteFile(file_name))
+    return false;
+#elif defined(OS_POSIX)
+  if (::remove(file_name))
+    return false;
+#endif
+
+  return true;
+}
+
+// Callback in the broker when a new broker connection occurs.
+int32_t OnInstanceConnected(PP_Instance instance, int32_t handle) {
+  PlatformFile file = IntToPlatformFile(handle);
+  if (file == kInvalidPlatformFileValue)
+    return PP_ERROR_FAILED;
+
+  // Send hello message.
+  if (!WriteMessage(file, kHelloMessage, sizeof(kHelloMessage))) {
+    ClosePlatformFile(file);
+    return PP_ERROR_FAILED;
+  }
+
+  // Verify broker is not sandboxed and send result to plugin over the pipe.
+  if (VerifyIsUnsandboxed()) {
+    if (!WriteMessage(file, kBrokerUnsandboxed, sizeof(kBrokerUnsandboxed))) {
+      ClosePlatformFile(file);
+      return PP_ERROR_FAILED;
+    }
+  }

[ddorwin, 2011/11/14 18:19:39]

comment that the plugin will handle the unsandboxed failure (and thus no else).

[xhwang, 2011/11/14 22:24:30]

Done.

+
+  if (!ClosePlatformFile(file))
+    return PP_ERROR_FAILED;
+
+  return PP_OK;
+}
+
+} // namespace
+
+PP_EXPORT int32_t PPP_InitializeBroker(
+    PP_ConnectInstance_Func* connect_instance_func) {
+  *connect_instance_func = &OnInstanceConnected;
+  return PP_OK;
+}
+
+PP_EXPORT void PPP_ShutdownBroker() {}
+
 TestBroker::TestBroker(TestingInstance* instance)
     : TestCase(instance),
       broker_interface_(NULL) {
@@ -24,6 +242,11 @@ bool TestBroker::Init() {
 
 void TestBroker::RunTest() {
   RUN_TEST(Create);
+  RUN_TEST(GetHandleFailure);
+  RUN_TEST(ConnectFailure);
+#if !defined(OS_WIN) // This is broken on Windows. See http://crbug.com/103975.
+  RUN_TEST(ConnectAndPipe);
+#endif
 }
 
 std::string TestBroker::TestCreate() {
@@ -35,12 +258,65 @@ std::string TestBroker::TestCreate() {
   ASSERT_FALSE(broker_interface_->IsBrokerTrusted(0));
   ASSERT_TRUE(broker_interface_->IsBrokerTrusted(broker));
 
+  PASS();
+}
+
+// Test connection on invalid resource.
+std::string TestBroker::TestConnectFailure() {
+  // Callback NOT force asynced. Connect should fail.  The callback will not be
+  // posted so there's no need to wait for the callback to complete.
+  TestCompletionCallback cb_1(instance_->pp_instance(), false);
+  ASSERT_EQ(PP_ERROR_BADRESOURCE,
+            broker_interface_->Connect(
+                0, pp::CompletionCallback(cb_1).pp_completion_callback()));
+
+  // Callback force aynced. Connect will return PP_OK_COMPLETIONPENDING and the
+  // callback will be posted.  However, the callback should fail.
+  TestCompletionCallback cb_2(instance_->pp_instance(), true);
+  ASSERT_EQ(PP_OK_COMPLETIONPENDING,
+            broker_interface_->Connect(
+                0, pp::CompletionCallback(cb_2).pp_completion_callback()));
+  ASSERT_EQ(PP_ERROR_BADRESOURCE, cb_2.WaitForResult());
+
+  PASS();
+}
+
+std::string TestBroker::TestGetHandleFailure() {
+  int32_t handle = kInvalidHandle;
+
   // Test getting the handle for an invalid resource.
-  int32_t handle;
-  ASSERT_TRUE(broker_interface_->GetHandle(0, &handle) == PP_ERROR_BADRESOURCE);
+  ASSERT_EQ(PP_ERROR_BADRESOURCE, broker_interface_->GetHandle(0, &handle));
 
   // Connect hasn't been called so this should fail.
-  ASSERT_TRUE(broker_interface_->GetHandle(broker, &handle) == PP_ERROR_FAILED);
+  PP_Resource broker = broker_interface_->CreateTrusted(
+      instance_->pp_instance());
+  ASSERT_TRUE(broker);
+  ASSERT_EQ(PP_ERROR_FAILED, broker_interface_->GetHandle(broker, &handle));
+
+  PASS();
+}
+
+std::string TestBroker::TestConnectAndPipe() {
+  PP_Resource broker = broker_interface_->CreateTrusted(
+      instance_->pp_instance());
+  ASSERT_TRUE(broker);
+
+  TestCompletionCallback cb_3(instance_->pp_instance());
+  ASSERT_EQ(PP_OK_COMPLETIONPENDING,
+            broker_interface_->Connect(
+                broker, pp::CompletionCallback(cb_3).pp_completion_callback()));
+  ASSERT_EQ(PP_OK, cb_3.WaitForResult());
+
+  int32_t handle = kInvalidHandle;
+  ASSERT_EQ(PP_OK, broker_interface_->GetHandle(broker, &handle));
+  ASSERT_NE(kInvalidHandle, handle);
+
+  PlatformFile file = IntToPlatformFile(handle);
+  ASSERT_TRUE(VerifyMessage(file, kHelloMessage, sizeof(kHelloMessage)));
+  ASSERT_TRUE(VerifyMessage(file, kBrokerUnsandboxed,

[ddorwin, 2011/11/14 18:19:39]

Would this hang if the broker was sandboxed and never sent the message? Is there
a reasonable way to avoid this?

[xhwang, 2011/11/14 22:24:30]

Both on POSIX and Win the socket pair/named pipes are created with
NON_BLOCKING/PIPE_NOWAIT set.  So if the message is not available for read for
any reason, VerifyMessage() will return false instead of hang.

+                            sizeof(kBrokerUnsandboxed)));
+
+  ASSERT_TRUE(ClosePlatformFile(file));
 
   PASS();
 }