[NaCl SDK] nacl_io: Don't delete in PepperInterface in ki_uninit.

At least not unless it was created during ki_init.  This is a far more
conventional approach and make the test code simpler.

R=binji@chromium.org

Review URL: https://codereview.chromium.org/267833005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@268534 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 59 insertions, 38 deletions

diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_intercept.cc b/native_client_sdk/src/libraries/nacl_io/kernel_intercept.cc
index ce4f170..35bf3fc 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_intercept.cc
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_intercept.cc
@@ -26,6 +26,7 @@ using namespace nacl_io;
 
 struct KernelInterceptState {
   KernelProxy* kp;
+  PepperInterface* ppapi;
   bool kp_owned;
 };
 
@@ -41,6 +42,7 @@ int ki_push_state_for_testing() {
     return 1;
   s_saved_state = s_state;
   s_state.kp = NULL;
+  s_state.ppapi = NULL;
   s_state.kp_owned = false;
   return 0;
 }
@@ -56,9 +58,12 @@ int ki_init_ppapi(void* kp,
   if (s_state.kp != NULL)
     return 1;
   PepperInterface* ppapi = NULL;
-  if (instance && get_browser_interface)
+  if (instance && get_browser_interface) {
     ppapi = new RealPepperInterface(instance, get_browser_interface);
-  return ki_init_interface(kp, ppapi);
+    s_state.ppapi = ppapi;
+  }
+  int rtn = ki_init_interface(kp, ppapi);
+  return rtn;
 }
 
 int ki_init_interface(void* kp, void* pepper_interface) {
@@ -92,17 +97,20 @@ void ki_uninit() {
 
   // If we are going to delete the KernelProxy don't do it
   // until we've swapped it out.
-  KernelProxy* delete_kp = s_state.kp_owned ? s_state.kp : NULL;
+  KernelInterceptState state_to_delete = s_state;
 
   // Swap out the KernelProxy. This will normally reset the
   // proxy to NULL, aside from in test code that has called
   // ki_push_state_for_testing().
   s_state = s_saved_state;
   s_saved_state.kp = NULL;
+  s_saved_state.ppapi = NULL;
   s_saved_state.kp_owned = false;
 
-  if (delete_kp)
-    delete delete_kp;
+  if (state_to_delete.kp_owned)
+    delete state_to_delete.kp;
+
+  delete state_to_delete.ppapi;
 }
 
 nacl_io::KernelProxy* ki_get_proxy() {
diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_intercept.h b/native_client_sdk/src/libraries/nacl_io/kernel_intercept.h
index 9b72f58..65d269b 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_intercept.h
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_intercept.h
@@ -55,8 +55,7 @@ int ki_init_ppapi(void* kernel_proxy,
 
 /*
  * ki_init_interface() is a variant of ki_init() that can be called with
- * a PepperInterface object.  The ownership of this object then passes
- * to nacl_io and it will be deleted on ki_uninit().
+ * a PepperInterface object.
  */
 int ki_init_interface(void* kernel_proxy, void* pepper_interface);
 
diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc b/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc
index 0cd8c10..65265ba 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_proxy.cc
@@ -63,8 +63,6 @@ KernelProxy::~KernelProxy() {
        ++i) {
     delete i->second;
   }
-
-  delete ppapi_;
 }
 
 Error KernelProxy::Init(PepperInterface* ppapi) {
diff --git a/native_client_sdk/src/libraries/nacl_io/kernel_proxy.h b/native_client_sdk/src/libraries/nacl_io/kernel_proxy.h
index 1cb1cc2..89b8f19 100644
--- a/native_client_sdk/src/libraries/nacl_io/kernel_proxy.h
+++ b/native_client_sdk/src/libraries/nacl_io/kernel_proxy.h
@@ -46,7 +46,6 @@ class KernelProxy : protected KernelObject {
   KernelProxy();
   virtual ~KernelProxy();
 
-  // Takes ownership of |ppapi|.
   // |ppapi| may be NULL. If so, no filesystem that uses pepper calls can be
   // mounted.
   virtual Error Init(PepperInterface* ppapi);
diff --git a/native_client_sdk/src/tests/nacl_io_test/dev_fs_for_testing.h b/native_client_sdk/src/tests/nacl_io_test/dev_fs_for_testing.h
index 86fc52b..a898a12 100644
--- a/native_client_sdk/src/tests/nacl_io_test/dev_fs_for_testing.h
+++ b/native_client_sdk/src/tests/nacl_io_test/dev_fs_for_testing.h
@@ -14,15 +14,13 @@
 
 class DevFsForTesting : public nacl_io::DevFs {
  public:
-  DevFsForTesting() {
+  explicit DevFsForTesting(nacl_io::PepperInterface* ppapi) {
     nacl_io::FsInitArgs args(1);
-    args.ppapi = &pepper_;
+    args.ppapi = ppapi;
     Init(args);
   }
 
   int num_nodes() { return (int)inode_pool_.size(); }
- private:
-  FakePepperInterface pepper_;
 };
 
 #endif  // TESTS_NACL_IO_TEST_DEV_FS_FOR_TESTING_H_
diff --git a/native_client_sdk/src/tests/nacl_io_test/filesystem_test.cc b/native_client_sdk/src/tests/nacl_io_test/filesystem_test.cc
index bfbd172..6089b01 100644
--- a/native_client_sdk/src/tests/nacl_io_test/filesystem_test.cc
+++ b/native_client_sdk/src/tests/nacl_io_test/filesystem_test.cc
@@ -270,7 +270,8 @@ TEST(FilesystemTest, MemFsRenameDir) {
 
 TEST(FilesystemTest, DevAccess) {
   // Should not be able to open non-existent file.
-  DevFsForTesting fs;
+  FakePepperInterface pepper;
+  DevFsForTesting fs(&pepper);
   ScopedNode invalid_node, valid_node;
   ASSERT_EQ(ENOENT, fs.Access(Path("/foo"), F_OK));
   // Creating non-existent file should return EACCES
@@ -290,7 +291,8 @@ TEST(FilesystemTest, DevAccess) {
 }
 
 TEST(FilesystemTest, DevNull) {
-  DevFsForTesting fs;
+  FakePepperInterface pepper;
+  DevFsForTesting fs(&pepper);
   ScopedNode dev_null;
   int result_bytes = 0;
 
@@ -313,7 +315,8 @@ TEST(FilesystemTest, DevNull) {
 }
 
 TEST(FilesystemTest, DevZero) {
-  DevFsForTesting fs;
+  FakePepperInterface pepper;
+  DevFsForTesting fs(&pepper);
   ScopedNode dev_zero;
   int result_bytes = 0;
 
@@ -343,7 +346,8 @@ TEST(FilesystemTest, DevZero) {
 
 // Disabled due to intermittent failures on linux: http://crbug.com/257257
 TEST(FilesystemTest, DISABLED_DevUrandom) {
-  DevFsForTesting fs;
+  FakePepperInterface pepper;
+  DevFsForTesting fs(&pepper);
   ScopedNode dev_urandom;
   int result_bytes = 0;
 
diff --git a/native_client_sdk/src/tests/nacl_io_test/host_resolver_test.cc b/native_client_sdk/src/tests/nacl_io_test/host_resolver_test.cc
index 642ffd1..b5810c0 100644
--- a/native_client_sdk/src/tests/nacl_io_test/host_resolver_test.cc
+++ b/native_client_sdk/src/tests/nacl_io_test/host_resolver_test.cc
@@ -35,19 +35,18 @@ class HostResolverTest : public ::testing::Test {
 
 class FakeHostResolverTest : public ::testing::Test {
  public:
-  FakeHostResolverTest() : pepper_(NULL), fake_resolver_(NULL) {}
+  FakeHostResolverTest() : fake_resolver_(NULL) {}
 
   void SetUp() {
-    pepper_ = new FakePepperInterface();
     fake_resolver_ = static_cast<FakeHostResolverInterface*>(
-        pepper_->GetHostResolverInterface());
+        pepper_.GetHostResolverInterface());
 
     // Seed the fake resolver with some data
     fake_resolver_->fake_hostname = FAKE_HOSTNAME;
     AddFakeAddress(AF_INET);
 
     ASSERT_EQ(0, ki_push_state_for_testing());
-    ASSERT_EQ(0, ki_init_interface(NULL, pepper_));
+    ASSERT_EQ(0, ki_init_interface(NULL, &pepper_));
   }
 
   void AddFakeAddress(int family) {
@@ -72,11 +71,10 @@ class FakeHostResolverTest : public ::testing::Test {
 
   void TearDown() {
     ki_uninit();
-    pepper_ = NULL;
   }
 
  protected:
-  FakePepperInterface* pepper_;
+  FakePepperInterface pepper_;
   FakeHostResolverInterface* fake_resolver_;
 };
 
diff --git a/native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc b/native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc
index 63872be..b14bf18 100644
--- a/native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc
+++ b/native_client_sdk/src/tests/nacl_io_test/jspipe_test.cc
@@ -27,6 +27,8 @@ namespace {
 
 class JSPipeTest : public ::testing::Test {
  public:
+  JSPipeTest() : fs_(&pepper_) {}
+
   void SetUp() {
     ASSERT_EQ(0, ki_push_state_for_testing());
     ASSERT_EQ(0, ki_init(&kp_));
@@ -40,6 +42,7 @@ class JSPipeTest : public ::testing::Test {
 
  protected:
   KernelProxy kp_;
+  FakePepperInterface pepper_;
   DevFsForTesting fs_;
   ScopedNode pipe_dev_;
 };
diff --git a/native_client_sdk/src/tests/nacl_io_test/tty_test.cc b/native_client_sdk/src/tests/nacl_io_test/tty_test.cc
index 1cfafb1..d54fa96 100644
--- a/native_client_sdk/src/tests/nacl_io_test/tty_test.cc
+++ b/native_client_sdk/src/tests/nacl_io_test/tty_test.cc
@@ -24,31 +24,44 @@ using namespace nacl_io;
 
 namespace {
 
-class TtyTest : public ::testing::Test {
+class TtyNodeTest : public ::testing::Test {
  public:
+  TtyNodeTest() : fs_(&pepper_) {}
+
   void SetUp() {
-    ASSERT_EQ(0, ki_push_state_for_testing());
-    ASSERT_EQ(0, ki_init(&kp_));
     ASSERT_EQ(0, fs_.Access(Path("/tty"), R_OK | W_OK));
     ASSERT_EQ(EACCES, fs_.Access(Path("/tty"), X_OK));
     ASSERT_EQ(0, fs_.Open(Path("/tty"), O_RDWR, &dev_tty_));
     ASSERT_NE(NULL_NODE, dev_tty_.get());
   }
 
-  void TearDown() { ki_uninit(); }
-
  protected:
-  KernelProxy kp_;
+  FakePepperInterface pepper_;
   DevFsForTesting fs_;
   ScopedNode dev_tty_;
 };
 
-TEST_F(TtyTest, InvalidIoctl) {
+class TtyTest : public ::testing::Test {
+ public:
+  void SetUp() {
+    ASSERT_EQ(0, ki_push_state_for_testing());
+    ASSERT_EQ(0, ki_init(&kp_));
+  }
+
+  void TearDown() {
+    ki_uninit();
+  }
+
+ protected:
+  KernelProxy kp_;
+};
+
+TEST_F(TtyNodeTest, InvalidIoctl) {
   // 123 is not a valid ioctl request.
   EXPECT_EQ(EINVAL, dev_tty_->Ioctl(123));
 }
 
-TEST_F(TtyTest, TtyInput) {
+TEST_F(TtyNodeTest, TtyInput) {
   // Now let's try sending some data over.
   // First we create the message.
   std::string message("hello, how are you?\n");
@@ -99,7 +112,7 @@ static ssize_t output_handler(const char* buf, size_t count, void* data) {
   return count;
 }
 
-TEST_F(TtyTest, TtyOutput) {
+TEST_F(TtyNodeTest, TtyOutput) {
   // When no handler is registered then all writes should return EIO
   int bytes_written = 10;
   const char* message = "hello\n";
@@ -245,7 +258,8 @@ static void sighandler(int sig) { g_received_signal = sig; }
 TEST_F(TtyTest, WindowSize) {
   // Get current window size
   struct winsize old_winsize = {0};
-  ASSERT_EQ(0, dev_tty_->Ioctl(TIOCGWINSZ, &old_winsize));
+  int tty_fd = ki_open("/dev/tty", O_RDONLY);
+  ASSERT_EQ(0, ki_ioctl_wrapper(tty_fd, TIOCGWINSZ, &old_winsize));
 
   // Install signal handler
   sighandler_t new_handler = sighandler;
@@ -258,7 +272,7 @@ TEST_F(TtyTest, WindowSize) {
   struct winsize winsize;
   winsize.ws_col = 100;
   winsize.ws_row = 200;
-  EXPECT_EQ(0, dev_tty_->Ioctl(TIOCSWINSZ, &winsize));
+  EXPECT_EQ(0, ki_ioctl_wrapper(tty_fd, TIOCSWINSZ, &winsize));
   EXPECT_EQ(SIGWINCH, g_received_signal);
 
   // Restore old signal handler
@@ -267,12 +281,12 @@ TEST_F(TtyTest, WindowSize) {
   // Verify new window size can be queried correctly.
   winsize.ws_col = 0;
   winsize.ws_row = 0;
-  EXPECT_EQ(0, dev_tty_->Ioctl(TIOCGWINSZ, &winsize));
+  EXPECT_EQ(0, ki_ioctl_wrapper(tty_fd, TIOCGWINSZ, &winsize));
   EXPECT_EQ(100, winsize.ws_col);
   EXPECT_EQ(200, winsize.ws_row);
 
   // Restore original windows size.
-  EXPECT_EQ(0, dev_tty_->Ioctl(TIOCSWINSZ, &old_winsize));
+  EXPECT_EQ(0, ki_ioctl_wrapper(tty_fd, TIOCSWINSZ, &old_winsize));
 }
 
 /*
@@ -343,7 +357,7 @@ TEST_F(TtyTest, InputDuringSelect) {
   FD_SET(tty_fd, &errorfds);
 
   pthread_t resize_thread;
-  pthread_create(&resize_thread, NULL, input_thread_main, &dev_tty_);
+  pthread_create(&resize_thread, NULL, input_thread_main, NULL);
 
   struct timeval timeout;
   timeout.tv_sec = 20;