Fix HANDLE leak in ResourceDispatcherHost tests.

content/browser/loader/resource_dispatcher_host_unittest.cc

 // Copyright (c) 2012 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 <vector>
 
 #include "base/basictypes.h"
 #include "base/bind.h"
 #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"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "content/browser/browser_thread_impl.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/loader/cross_site_resource_handler.h"
 #include "content/browser/loader/detachable_resource_handler.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
@@ skipping 53 matching lines @@
 }
 
 void GenerateIPCMessage(
     scoped_refptr<ResourceMessageFilter> filter,
     scoped_ptr<IPC::Message> message) {
   bool msg_is_ok;
   ResourceDispatcherHostImpl::Get()->OnMessageReceived(
       *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) {
   int request_id = -1;
   switch (msg.type()) {
     case ResourceMsg_UploadProgress::ID:
     case ResourceMsg_ReceivedResponse::ID:
     case ResourceMsg_ReceivedRedirect::ID:
     case ResourceMsg_SetDataBuffer::ID:
     case ResourceMsg_DataReceived::ID:
@@ skipping 31 matching lines @@
 }
 
 // Spin up the message loop to kick off the request.
 static void KickOffRequest() {
   base::MessageLoop::current()->RunUntilIdle();
 }
 
 // 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);
   }
 
   // This groups the messages by their request ID. The groups will be in order
   // that the first message for each request ID was received, and the messages
   // within the groups will be in the order that they appeared.
   // Note that this clears messages_.
   typedef std::vector< std::vector<IPC::Message> > ClassifiedMessages;
   void GetClassifiedMessages(ClassifiedMessages* msgs);
@@ skipping 41 matching lines @@
         base::Bind(&ForwardingFilter::GetContexts,
                    base::Unretained(this))),
       dest_(dest),
       resource_context_(resource_context) {
     ChildProcessSecurityPolicyImpl::GetInstance()->Add(child_id());
     set_peer_pid_for_testing(base::GetCurrentProcId());
   }
 
   // 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);
   }
 
   ResourceContext* resource_context() { return resource_context_; }
 
  protected:
   virtual ~ForwardingFilter() {}
 
  private:
   void GetContexts(const ResourceHostMsg_Request& request,
@@ skipping 406 matching lines @@
     if (send_data_received_acks_ &&
         msg->type() == ResourceMsg_DataReceived::ID) {
       GenerateDataReceivedACK(*msg);
     }
 
     if (wait_for_request_complete_loop_ &&
         msg->type() == ResourceMsg_RequestComplete::ID) {
       wait_for_request_complete_loop_->Quit();
     }
 
+    // Do not release handles in it yet; the accumulator owns them now.
     delete msg;
     return true;
   }
 
  protected:
   // testing::Test
   virtual void SetUp() OVERRIDE {
     DCHECK(!test_fixture_);
     test_fixture_ = this;
     ChildProcessSecurityPolicyImpl::GetInstance()->Add(0);
@@ skipping 793 matching lines @@
       : ForwardingFilter(NULL, resource_context),
         has_canceled_(false),
         received_after_canceled_(0) {
   }
 
   // ForwardingFilter override
   virtual bool Send(IPC::Message* msg) OVERRIDE {
     // no messages should be received when the process has been canceled
     if (has_canceled_)
       received_after_canceled_++;
+    ReleaseHandlesInMessage(*msg);
     delete msg;
     return true;
   }
 
   bool has_canceled_;
   int received_after_canceled_;
 
  private:
   virtual ~TestFilter() {}
 };
@@ skipping 1445 matching lines @@
   // message loop for the delete itself. (This relies on the delete happening on
   // the FILE thread which is mapped to main thread in this test.)
   base::RunLoop().RunUntilIdle();
 
   EXPECT_FALSE(base::PathExists(response_head.download_file_path));
   EXPECT_FALSE(ChildProcessSecurityPolicyImpl::GetInstance()->CanReadFile(
       filter_->child_id(), response_head.download_file_path));
 }
 
 }  // namespace content