Fix HANDLE leak in ResourceDispatcherHost tests.

content/browser/loader/resource_dispatcher_host_unittest.cc

Index: content/browser/loader/resource_dispatcher_host_unittest.cc
diff --git a/content/browser/loader/resource_dispatcher_host_unittest.cc b/content/browser/loader/resource_dispatcher_host_unittest.cc
index 7717431928dee5c20fc1d62e747c5998e2ddbe4d..c976854fff00bab98f952a4fc1c0eb1fb45eb3ab 100644
--- a/content/browser/loader/resource_dispatcher_host_unittest.cc
+++ b/content/browser/loader/resource_dispatcher_host_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/file_util.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_vector.h"
+#include "base/memory/shared_memory.h"
 #include "base/message_loop/message_loop.h"
 #include "base/pickle.h"
 #include "base/run_loop.h"
@@ -82,6 +83,29 @@ void GenerateIPCMessage(
       *message, filter.get(), &msg_is_ok);
 }
 
+// On Windows, ResourceMsg_SetDataBuffer supplies a HANDLE which is not
+// automatically released.
+//
+// See ResourceDispatcher::ReleaseResourcesInDataMessage.
+//
+// TODO(davidben): It would be nice if the behavior for base::SharedMemoryHandle
+// were more like it is in POSIX where the received fds are tracked in a
+// ref-counted core that closes them if not extracted.
+void ReleaseHandlesInMessage(const IPC::Message& message) {
+  if (message.type() == ResourceMsg_SetDataBuffer::ID) {
+    PickleIterator iter(message);
+    int request_id;
+    CHECK(message.ReadInt(&iter, &request_id));
+    base::SharedMemoryHandle shm_handle;
+    if (IPC::ParamTraits<base::SharedMemoryHandle>::Read(&message,
+                                                         &iter,
+                                                         &shm_handle)) {
+      if (base::SharedMemory::IsHandleValid(shm_handle))
+        base::SharedMemory::CloseHandle(shm_handle);
+    }
+  }
+}
+
 }  // namespace
 
 static int RequestIDForMessage(const IPC::Message& msg) {
@@ -133,6 +157,13 @@ static void KickOffRequest() {
 // We may want to move this to a shared space if it is useful for something else
 class ResourceIPCAccumulator {
  public:
+  ~ResourceIPCAccumulator() {
+    for (size_t i = 0; i < messages_.size(); i++) {
+      ReleaseHandlesInMessage(messages_[i]);
+    }
+  }
+
+  // On Windows, takes ownership of SharedMemoryHandles in |msg|.
   void AddMessage(const IPC::Message& msg) {
     messages_.push_back(msg);
   }
@@ -194,8 +225,11 @@ class ForwardingFilter : public ResourceMessageFilter {
 
   // ResourceMessageFilter override
   virtual bool Send(IPC::Message* msg) OVERRIDE {
-    if (!dest_)
+    if (!dest_) {
+      ReleaseHandlesInMessage(*msg);
+      delete msg;
       return false;

[mmenke, 2014/04/25 16:07:24]

Shouldn't this be return true, because we ate the message and it shouldn't be
sent anywhere else?

[mmenke, 2014/04/25 16:09:20]

Oops...this is send, not receive...nevermind.

[davidben, 2014/04/25 16:24:37]

This arrangement is kind of silly anyway. This codepath never happens even
though ForwardingFilter does see a NULL dest_. I've inverted which of TestFilter
and ForwardingFilter subclasses which. This seems more sensible.

+    }
     return dest_->Send(msg);
   }
 
@@ -622,6 +656,7 @@ class ResourceDispatcherHostTest : public testing::Test,
       wait_for_request_complete_loop_->Quit();
     }
 
+    // Do not release handles in it yet; the accumulator owns them now.
     delete msg;
     return true;
   }
@@ -1435,6 +1470,7 @@ class TestFilter : public ForwardingFilter {
     // no messages should be received when the process has been canceled
     if (has_canceled_)
       received_after_canceled_++;
+    ReleaseHandlesInMessage(*msg);
     delete msg;
     return true;
   }