Tweaks to SharedMemory to avoid CHECK when printing

base/memory/shared_memory_win.cc

 // 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 "base/memory/shared_memory.h"
 
 #include <aclapi.h>
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
@@ skipping 56 matching lines @@
                     FALSE, 0);
 }
 
 SharedMemory::~SharedMemory() {
   Unmap();
   Close();
 }
 
 // static
 bool SharedMemory::IsHandleValid(const SharedMemoryHandle& handle) {
-  return handle != NULL;
+  return handle != NULL && handle != INVALID_HANDLE_VALUE;

[rvargas (doing something else), 2015/09/18 22:36:25]

<rant>
I know I'm grumpy, but this pattern doesn't make sense with or without this
change :(.

I know this is not what this method is promising, but there's no way to see if a
random handle is valid, and I'd rather remove this method completely and leave
that job to whoever is creating the handle (aka, remove raw handles completely).

Now, I understand that fix would be out of the scope of the current problem.

If we go with the current API, if a caller grabs a number from a hat and uses
it, it's basically depending on the internal implementation of this class, so
widening the range of values that we won't complain about is... kind of silly? I
hear that we should make this less prone to misuse, but really, we should fix
the API instead of adding knee pads to the cutting edges. We should be using
ScopedHandles here, and the caller should be using ScopedHandles, even if we end
up passing the raw handle through the interface.
</rant>

 }
 
 // static
 SharedMemoryHandle SharedMemory::NULLHandle() {
   return NULL;
 }
 
 // static
 void SharedMemory::CloseHandle(const SharedMemoryHandle& handle) {
-  DCHECK(handle != NULL);
+  DCHECK(IsHandleValid(handle));

[rvargas (doing something else), 2015/09/18 22:36:25]

ScopedHandle closer(handle);

<rant> receiving const SharedHandle& for something that we'll be essentially
destroying doesn't make sense to me. This method also has to go away </rant>

   ::CloseHandle(handle);
 }
 
 // static
 size_t SharedMemory::GetHandleLimit() {
   // Rounded down from value reported here:
   // http://blogs.technet.com/b/markrussinovich/archive/2009/09/29/3283844.aspx
   return static_cast<size_t>(1 << 23);
 }
 
@@ skipping 73 matching lines @@
 
   return true;
 }
 
 bool SharedMemory::Delete(const std::string& name) {
   // intentionally empty -- there is nothing for us to do on Windows.
   return true;
 }
 
 bool SharedMemory::Open(const std::string& name, bool read_only) {
-  DCHECK(!mapped_file_);
+  DCHECK(!IsHandleValid(mapped_file_));

[rvargas (doing something else), 2015/09/18 22:36:25]

DCHECK(mapped_file_.Isvalid());

(+ making the member a ScopedHandle). etc.

 
   name_ = ASCIIToUTF16(name);
   read_only_ = read_only;
   mapped_file_ = OpenFileMapping(
       read_only_ ? FILE_MAP_READ : FILE_MAP_READ | FILE_MAP_WRITE,
       false, name_.empty() ? NULL : name_.c_str());
-  if (mapped_file_ != NULL) {
+  if (IsHandleValid(mapped_file_)) {
     // Note: size_ is not set in this case.
     return true;
   }
   return false;
 }
 
 bool SharedMemory::MapAt(off_t offset, size_t bytes) {
-  if (mapped_file_ == NULL)
+  if (!IsHandleValid(mapped_file_))
     return false;
 
   if (bytes > static_cast<size_t>(std::numeric_limits<int>::max()))
     return false;
 
   if (memory_)
     return false;
 
   memory_ = MapViewOfFile(mapped_file_,
                           read_only_ ? FILE_MAP_READ : FILE_MAP_READ |
@@ skipping 45 matching lines @@
   if (!::DuplicateHandle(GetCurrentProcess(), mapped_file, process, &result,
                          access, FALSE, options)) {
     return false;
   }
   *new_handle = result;
   return true;
 }
 
 
 void SharedMemory::Close() {
-  if (mapped_file_ != NULL) {
+  if (IsHandleValid(mapped_file_)) {
     CloseHandle(mapped_file_);
     mapped_file_ = NULL;
   }
 }
 
 SharedMemoryHandle SharedMemory::handle() const {
   return mapped_file_;
 }
 
 }  // namespace base