Revert 63232 - On Windows, create a new TransportDIB::Handle struct which includes the file

mapping HANDLE and the source process ID. Duplicating the handle for the
remote process is done in TransportDIB::Map, instead of in various #ifdefs
scattered across the code. Also on windows, remove the struct for the
TransportDIB::Id which contained both the sequence number and the HANDLE and
replace it with just the sequence number.

Fix ThumbnailGenerator by mapping the TransportDIB on Windows and adding
a method to duplicate the file mapping handle before sending across the
channel.

Also, add a ScopedHandle and fix some handle leaks.
BUG=none
TEST=none

Review URL: http://codereview.chromium.org/3834003

TBR=kkania@chromium.org
Review URL: http://codereview.chromium.org/3943002

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

4 files changed, 83 insertions, 294 deletions

diff --git a/app/surface/transport_dib.h b/app/surface/transport_dib.h
index 37a3912..6606c2b 100644
--- a/app/surface/transport_dib.h
+++ b/app/surface/transport_dib.h
@@ -7,7 +7,6 @@
 #pragma once
 
 #include "base/basictypes.h"
-#include "base/process.h"
 
 #if defined(OS_WIN) || defined(OS_MACOSX)
 #include "base/shared_memory.h"
@@ -32,55 +31,13 @@ class TransportDIB {
  public:
   ~TransportDIB();
 
-  // Two typedefs are defined. A |Handle| can be sent over the wire so that the
-  // remote side can map the |TransportDIB|. These handles may be reused from
-  // previous DIBs. An |Id| is unique and never reused, but it is not sufficient
-  // to map the DIB.
+  // Two typedefs are defined. A Handle is the type which can be sent over
+  // the wire so that the remote side can map the transport DIB. The Id typedef
+  // is sufficient to identify the transport DIB when you know that the remote
+  // side already may have it mapped.
 #if defined(OS_WIN)
-  // On Windows, a |Handle| is a combination of the section (i.e., file mapping)
-  // handle and the ID of the corresponding process. When the DIB is mapped in
-  // a remote process, the section handle is duplicated for use in that process.
-  // However, if the remote process does not have permission to duplicate the
-  // handle, the first process must duplicate the handle before sending it.
-  // E.g., this is necessary if the DIB is created in the browser and will be
-  // mapped in the sandboxed renderer.
-  class TransferrableSectionHandle {
-   public:
-    TransferrableSectionHandle()
-        : section_(NULL), owner_id_(NULL), should_dup_on_map_(false) {
-    }
-
-    TransferrableSectionHandle(HANDLE section, base::ProcessId owner_id,
-                               bool should_dup_on_map)
-        : section_(section),
-          owner_id_(owner_id),
-          should_dup_on_map_(should_dup_on_map) {
-    }
-
-    // Duplicates the handle for use in the given process.
-    TransferrableSectionHandle DupForProcess(
-        base::ProcessHandle new_owner) const;
-
-    // Closes this handle. This should be called if this handle was duplicated
-    // and is not owned by a TransportDIB.
-    void Close() const;
-
-    // Returns true if this handle refers to an actual file mapping.
-    bool is_valid() const { return section_ != NULL && owner_id_ != NULL; }
-
-    HANDLE section() const { return section_; }
-    base::ProcessId owner_id() const { return owner_id_; }
-    bool should_dup_on_map() const { return should_dup_on_map_; }
-
-   private:
-    HANDLE section_;
-    base::ProcessId owner_id_;
-    // Whether the handle should be duplicated when the DIB is mapped.
-    bool should_dup_on_map_;
-  };
-  typedef TransferrableSectionHandle Handle;
-
-  // On Windows, the Id type is a sequence number (epoch) to solve an ABA
+  typedef HANDLE Handle;
+  // On Windows, the Id type includes a sequence number (epoch) to solve an ABA
   // issue:
   //   1) Process A creates a transport DIB with HANDLE=1 and sends to B.
   //   2) Process B maps the transport DIB and caches 1 -> DIB.
@@ -88,17 +45,38 @@ class TransportDIB {
   //      is also assigned HANDLE=1.
   //   4) Process A sends the Handle to B, but B incorrectly believes that it
   //      already has it cached.
-  typedef uint32 Id;
+  struct HandleAndSequenceNum {
+    HandleAndSequenceNum()
+        : handle(NULL),
+          sequence_num(0) {
+    }
+
+    HandleAndSequenceNum(HANDLE h, uint32 seq_num)
+        : handle(h),
+          sequence_num(seq_num) {
+    }
+
+    bool operator< (const HandleAndSequenceNum& other) const {
+      // Use the lexicographic order on the tuple <handle, sequence_num>.
+      if (other.handle != handle)
+        return other.handle < handle;
+      return other.sequence_num < sequence_num;
+    }
+
+    HANDLE handle;
+    uint32 sequence_num;
+  };
+  typedef HandleAndSequenceNum Id;
 
   // Returns a default, invalid handle, that is meant to indicate a missing
   // Transport DIB.
-  static Handle DefaultHandleValue() { return Handle(); }
+  static Handle DefaultHandleValue() { return NULL; }
 
   // Returns a value that is ONLY USEFUL FOR TESTS WHERE IT WON'T BE
   // ACTUALLY USED AS A REAL HANDLE.
   static Handle GetFakeHandleForTest() {
     static int fake_handle = 10;
-    return Handle(reinterpret_cast<HANDLE>(fake_handle++), 1, false);
+    return reinterpret_cast<Handle>(fake_handle++);
   }
 #elif defined(OS_MACOSX)
   typedef base::SharedMemoryHandle Handle;
@@ -131,41 +109,7 @@ class TransportDIB {
   }
 #endif
 
-  // When passing a TransportDIB::Handle across processes, you must always close
-  // the handle, even if you return early, or the handle will be leaked. Typical
-  // usage will be:
-  //
-  //   MyIPCHandler(TransportDIB::Handle dib_handle) {
-  //     TransportDIB::ScopedHandle handle_scoper(dib_handle);
-  //     ... do some stuff, possible returning early ...
-  //
-  //     TransportDIB* dib = TransportDIB::Map(handle_scoper.release());
-  //     // The handle lifetime is now managed by the TransportDIB.
-  class ScopedHandle {
-   public:
-    ScopedHandle() : handle_(DefaultHandleValue()) {}
-    explicit ScopedHandle(Handle handle) : handle_(handle) {}
-
-    ~ScopedHandle() {
-      Close();
-    }
-
-    Handle release() {
-      Handle temp = handle_;
-      handle_ = DefaultHandleValue();
-      return temp;
-    }
-
-    operator Handle() { return handle_; }
-
-   private:
-    void Close();
-
-    Handle handle_;
-    DISALLOW_COPY_AND_ASSIGN(ScopedHandle);
-  };
-
-  // Create a new |TransportDIB|, returning NULL on failure.
+  // Create a new TransportDIB, returning NULL on failure.
   //
   // The size is the minimum size in bytes of the memory backing the transport
   // DIB (we may actually allocate more than that to give us better reuse when
@@ -174,18 +118,12 @@ class TransportDIB {
   // The sequence number is used to uniquely identify the transport DIB. It
   // should be unique for all transport DIBs ever created in the same
   // renderer.
-  //
-  // On Linux, this will also map the DIB into the current process.
   static TransportDIB* Create(size_t size, uint32 sequence_num);
 
-  // Map the referenced transport DIB. The caller owns the returned object.
+  // Map the referenced transport DIB.  The caller owns the returned object.
   // Returns NULL on failure.
   static TransportDIB* Map(Handle transport_dib);
 
-  // Create a new |TransportDIB| with a handle to the shared memory. This
-  // always returns a valid pointer. The DIB is not mapped.
-  static TransportDIB* CreateWithHandle(Handle handle);
-
   // Returns true if the handle is valid.
   static bool is_valid(Handle dib);
 
@@ -193,31 +131,11 @@ class TransportDIB {
   // pointer will be owned by the caller. The bitmap will be of the given size,
   // which should fit inside this memory.
   //
-  // On POSIX, this |TransportDIB| will be mapped if not already. On Windows,
-  // this |TransportDIB| will NOT be mapped and should not be mapped prior,
-  // because PlatformCanvas will map the file internally.
-  //
   // Will return NULL on allocation failure. This could be because the image
   // is too large to map into the current process' address space.
   skia::PlatformCanvas* GetPlatformCanvas(int w, int h);
 
-  // Map the DIB into the current process if it is not already. This is used to
-  // map a DIB that has already been created. Returns true if the DIB is mapped.
-  bool Map();
-
-  // Return a handle for use in a specific process. On POSIX, this simply
-  // returns the handle as in the |handle| accessor below. On Windows, this
-  // returns a duplicate handle for use in the given process. This should be
-  // used instead of the |handle| accessor only if the process that will map
-  // this DIB does not have permission to duplicate the handle from the
-  // first process.
-  //
-  // Note: On Windows, if the duplicated handle is not closed by the other side
-  // (or this process fails to transmit the handle), the shared memory will be
-  // leaked.
-  Handle GetHandleForProcess(base::ProcessHandle process_handle) const;
-
-  // Return a pointer to the shared memory.
+  // Return a pointer to the shared memory
   void* memory() const;
 
   // Return the maximum size of the shared memory. This is not the amount of
diff --git a/app/surface/transport_dib_linux.cc b/app/surface/transport_dib_linux.cc
index ac8d71b..26cad3f 100644
--- a/app/surface/transport_dib_linux.cc
+++ b/app/surface/transport_dib_linux.cc
@@ -17,9 +17,6 @@
 // The shmat system call uses this as it's invalid return address
 static void *const kInvalidAddress = (void*) -1;
 
-void TransportDIB::ScopedHandle::Close() {
-}
-
 TransportDIB::TransportDIB()
     : key_(-1),
       address_(kInvalidAddress),
@@ -68,57 +65,34 @@ TransportDIB* TransportDIB::Create(size_t size, uint32 sequence_num) {
   return dib;
 }
 
-// static
-TransportDIB* TransportDIB::Map(Handle handle) {
-  scoped_ptr<TransportDIB> dib(CreateWithHandle(handle));
-  if (!dib->Map())
+TransportDIB* TransportDIB::Map(Handle shmkey) {
+  struct shmid_ds shmst;
+  if (shmctl(shmkey, IPC_STAT, &shmst) == -1)
+    return NULL;
+
+  void* address = shmat(shmkey, NULL /* desired address */, 0 /* flags */);
+  if (address == kInvalidAddress)
     return NULL;
-  return dib.release();
-}
 
-// static
-TransportDIB* TransportDIB::CreateWithHandle(Handle shmkey) {
   TransportDIB* dib = new TransportDIB;
+
+  dib->address_ = address;
+  dib->size_ = shmst.shm_segsz;
   dib->key_ = shmkey;
   return dib;
 }
 
-// static
 bool TransportDIB::is_valid(Handle dib) {
   return dib >= 0;
 }
 
 skia::PlatformCanvas* TransportDIB::GetPlatformCanvas(int w, int h) {
-  if (address_ == kInvalidAddress && !Map())
-    return NULL;
   scoped_ptr<skia::PlatformCanvas> canvas(new skia::PlatformCanvas);
   if (!canvas->initialize(w, h, true, reinterpret_cast<uint8_t*>(memory())))
     return NULL;
   return canvas.release();
 }
 
-bool TransportDIB::Map() {
-  if (address_ != kInvalidAddress)
-    return true;
-
-  struct shmid_ds shmst;
-  if (shmctl(key_, IPC_STAT, &shmst) == -1)
-    return false;
-
-  void* address = shmat(key_, NULL /* desired address */, 0 /* flags */);
-  if (address == kInvalidAddress)
-    return false;
-
-  address_ = address;
-  size_ = shmst.shm_segsz;
-  return true;
-}
-
-TransportDIB::Handle TransportDIB::GetHandleForProcess(
-    base::ProcessHandle process_handle) const {
-  return handle();
-}
-
 void* TransportDIB::memory() const {
   DCHECK_NE(address_, kInvalidAddress);
   return address_;
diff --git a/app/surface/transport_dib_mac.cc b/app/surface/transport_dib_mac.cc
index 68ccfca..a3eb0bb 100644
--- a/app/surface/transport_dib_mac.cc
+++ b/app/surface/transport_dib_mac.cc
@@ -13,11 +13,6 @@
 #include "base/shared_memory.h"
 #include "skia/ext/platform_canvas.h"
 
-void TransportDIB::ScopedHandle::Close() {
-  if (is_valid(handle_))
-    base::SharedMemory::CloseHandle(handle_);
-}
-
 TransportDIB::TransportDIB()
     : size_(0) {
 }
@@ -39,56 +34,46 @@ TransportDIB* TransportDIB::Create(size_t size, uint32 sequence_num) {
     return NULL;
   }
 
+  if (!dib->shared_memory_.Map(size)) {
+    delete dib;
+    return NULL;
+  }
+
   dib->size_ = size;
   return dib;
 }
 
 // static
-TransportDIB* TransportDIB::Map(Handle handle) {
-  scoped_ptr<TransportDIB> dib(CreateWithHandle(handle));
-  if (!dib->Map())
+TransportDIB* TransportDIB::Map(TransportDIB::Handle handle) {
+  if (!is_valid(handle))
     return NULL;
-  return dib.release();
-}
 
-// static
-TransportDIB* TransportDIB::CreateWithHandle(Handle handle) {
-  return new TransportDIB(handle);
+  TransportDIB* dib = new TransportDIB(handle);
+  struct stat st;
+  if ((fstat(handle.fd, &st) != 0) ||
+      (!dib->shared_memory_.Map(st.st_size))) {
+    delete dib;
+    if (HANDLE_EINTR(close(handle.fd)) < 0)
+      PLOG(ERROR) << "close";
+    return NULL;
+  }
+
+  dib->size_ = st.st_size;
+
+  return dib;
 }
 
-// static
 bool TransportDIB::is_valid(Handle dib) {
   return dib.fd >= 0;
 }
 
 skia::PlatformCanvas* TransportDIB::GetPlatformCanvas(int w, int h) {
-  if (!memory() && !Map())
-    return NULL;
   scoped_ptr<skia::PlatformCanvas> canvas(new skia::PlatformCanvas);
   if (!canvas->initialize(w, h, true, reinterpret_cast<uint8_t*>(memory())))
     return NULL;
   return canvas.release();
 }
 
-bool TransportDIB::Map() {
-  if (memory())
-    return true;
-
-  struct stat st;
-  if ((fstat(shared_memory_.handle().fd, &st) != 0) ||
-      (!shared_memory_.Map(st.st_size))) {
-    return false;
-  }
-
-  size_ = st.st_size;
-  return true;
-}
-
-TransportDIB::Handle TransportDIB::GetHandleForProcess(
-    base::ProcessHandle process_handle) const {
-  return handle();
-}
-
 void* TransportDIB::memory() const {
   return shared_memory_.memory();
 }
diff --git a/app/surface/transport_dib_win.cc b/app/surface/transport_dib_win.cc
index f170786..f7746e3 100644
--- a/app/surface/transport_dib_win.cc
+++ b/app/surface/transport_dib_win.cc
@@ -2,74 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include <windows.h>
-
 #include <limits>
+#include <windows.h>
 
 #include "app/surface/transport_dib.h"
 #include "base/logging.h"
-#include "base/scoped_handle_win.h"
 #include "base/scoped_ptr.h"
 #include "base/sys_info.h"
 #include "skia/ext/platform_canvas.h"
 
-TransportDIB::Handle TransportDIB::Handle::DupForProcess(
-    base::ProcessHandle new_owner_process) const {
-  base::ProcessId new_owner_id = ::GetProcessId(new_owner_process);
-  if (!new_owner_id) {
-    LOG(WARNING) << "Could not get process id from handle"
-                 << " handle:" << new_owner_process
-                 << " error:" << ::GetLastError();
-    return Handle();
-  }
-  HANDLE owner_process = ::OpenProcess(PROCESS_DUP_HANDLE, FALSE, owner_id_);
-  if (!owner_process) {
-    LOG(WARNING) << "Could not open process"
-                 << " id:" << owner_id_
-                 << " error:" << ::GetLastError();
-    return Handle();
-  }
-  ::ScopedHandle scoped_owner_process(owner_process);
-  HANDLE new_section = NULL;
-  ::DuplicateHandle(owner_process, section_,
-                    new_owner_process, &new_section,
-                    STANDARD_RIGHTS_REQUIRED | FILE_MAP_ALL_ACCESS,
-                    FALSE, 0);
-  if (!new_section) {
-    LOG(WARNING) << "Could not duplicate handle for process"
-                 << " error:" << ::GetLastError();
-    return Handle();
-  }
-  return Handle(new_section, new_owner_id, false);
-}
-
-void TransportDIB::Handle::Close() const {
-  // Do not close this handle if it is invalid, or it has not been duped yet.
-  if (!is_valid() || should_dup_on_map_)
-    return;
-
-  // The handle was already duped for a target process, so we should close it.
-  // The target process may not have been us, so close using
-  // |::DuplicateHandle|.
-  HANDLE owner_process = ::OpenProcess(PROCESS_DUP_HANDLE, FALSE, owner_id_);
-  if (!owner_process) {
-    LOG(WARNING) << "Could not open process"
-                 << " id:" << owner_id_
-                 << " error:" << ::GetLastError();
-    return;
-  }
-  if (!::DuplicateHandle(owner_process, section_,
-                         NULL, NULL,
-                         DUPLICATE_CLOSE_SOURCE,
-                         FALSE, 0)) {
-    NOTREACHED() << "Handle could not be closed: " << ::GetLastError();
-  }
-}
-
-void TransportDIB::ScopedHandle::Close() {
-  handle_.Close();
-}
-
 TransportDIB::TransportDIB() {
 }
 
@@ -101,73 +42,44 @@ TransportDIB* TransportDIB::Create(size_t size, uint32 sequence_num) {
 }
 
 // static
-TransportDIB* TransportDIB::Map(Handle handle) {
-  scoped_ptr<TransportDIB> dib(CreateWithHandle(handle));
-  if (!dib->Map())
+TransportDIB* TransportDIB::Map(TransportDIB::Handle handle) {
+  TransportDIB* dib = new TransportDIB(handle);
+  if (!dib->shared_memory_.Map(0 /* map whole shared memory segment */)) {
+    LOG(ERROR) << "Failed to map transport DIB"
+               << " handle:" << handle
+               << " error:" << GetLastError();
+    delete dib;
     return NULL;
-  return dib.release();
-}
+  }
 
-// static
-TransportDIB* TransportDIB::CreateWithHandle(Handle handle) {
-  // It is not sufficient to compare the current process ID and the ID in the
-  // handle here to see if a duplication is required because they will always
-  // be the same in single process mode.
-  if (handle.should_dup_on_map())
-    handle = handle.DupForProcess(::GetCurrentProcess());
-  return new TransportDIB(handle.section());
+  // There doesn't seem to be any way to find the size of the shared memory
+  // region! GetFileSize indicates that the handle is invalid. Thus, we
+  // conservatively set the size to the maximum and hope that the renderer
+  // isn't about to ask us to read off the end of the array.
+  dib->size_ = std::numeric_limits<size_t>::max();
+
+  return dib;
 }
 
-// static
 bool TransportDIB::is_valid(Handle dib) {
-  return dib.is_valid();
+  return dib != NULL;
 }
 
 skia::PlatformCanvas* TransportDIB::GetPlatformCanvas(int w, int h) {
-  // This DIB already mapped the file into this process, but PlatformCanvas
-  // will map it again.
-  DCHECK(!memory()) << "Mapped file twice in the same process.";
-
   scoped_ptr<skia::PlatformCanvas> canvas(new skia::PlatformCanvas);
-  if (!canvas->initialize(w, h, true, shared_memory_.handle()))
+  if (!canvas->initialize(w, h, true, handle()))
     return NULL;
   return canvas.release();
 }
 
-bool TransportDIB::Map() {
-  if (memory())
-    return true;
-
-  if (!shared_memory_.Map(0 /* map whole shared memory segment */)) {
-    LOG(ERROR) << "Failed to map transport DIB"
-               << " handle:" << shared_memory_.handle()
-               << " error:" << ::GetLastError();
-    return false;
-  }
-
-  // There doesn't seem to be any way to find the size of the shared memory
-  // region! GetFileSize indicates that the handle is invalid. Thus, we
-  // conservatively set the size to the maximum and hope that the renderer
-  // isn't about to ask us to read off the end of the array.
-  size_ = std::numeric_limits<size_t>::max();
-  return true;
-}
-
-TransportDIB::Handle TransportDIB::GetHandleForProcess(
-    base::ProcessHandle process_handle) const {
-  return handle().DupForProcess(process_handle);
-}
-
 void* TransportDIB::memory() const {
   return shared_memory_.memory();
 }
 
 TransportDIB::Handle TransportDIB::handle() const {
-  return TransferrableSectionHandle(shared_memory_.handle(),
-                                    ::GetCurrentProcessId(),
-                                    true);
+  return shared_memory_.handle();
 }
 
 TransportDIB::Id TransportDIB::id() const {
-  return sequence_num_;
+  return Id(shared_memory_.handle(), sequence_num_);
 }