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 16 matching lines @@
       for (int i = 1; i < static_cast<int>(messages_.size()); i++) {
         int id = RequestIDForMessage(messages_[i]);
         if (id == cur_id) {
           cur_requests.push_back(messages_[i]);
           messages_.erase(messages_.begin() + i);
           i--;
         }
       }
       msgs->push_back(cur_requests);
     }
     messages_.erase(messages_.begin());

[mmenke, 2014/04/28 20:52:08]

Aren't we completely skipping the cleanup here?

[davidben, 2014/04/28 21:20:47]

Oh hrm. Looks like it ends up being fine because those then get passed into
CheckSuccessfulRequest which puts it into a base::SharedMemory. That takes
ownership of the handle. Added a comment to make this clearer.

This really is kind of a mess. :-)

   }
 }
 
-// This class forwards the incoming messages to the ResourceDispatcherHostTest.
 // This is used to emulate different sub-processes, since this filter will
-// have a different ID than the original. For the test, we want all the incoming
-// messages to go to the same place, which is why this forwards.
-class ForwardingFilter : public ResourceMessageFilter {
+// have a different ID than the original.
+class TestFilter : public ResourceMessageFilter {
  public:
-  explicit ForwardingFilter(IPC::Sender* dest,
-                            ResourceContext* resource_context)
+  explicit TestFilter(ResourceContext* resource_context)
     : ResourceMessageFilter(

[mmenke, 2014/04/28 20:52:08]

nit:  +2 indent (Know this was wrong before)

[davidben, 2014/04/28 21:20:47]

Bounced on tab in emacs a bit.

         ChildProcessHostImpl::GenerateChildProcessUniqueId(),
         PROCESS_TYPE_RENDERER, NULL, NULL, NULL, NULL,
-        base::Bind(&ForwardingFilter::GetContexts,
-                   base::Unretained(this))),
-      dest_(dest),
-      resource_context_(resource_context) {
+        base::Bind(&TestFilter::GetContexts, base::Unretained(this))),
+      resource_context_(resource_context),
+      canceled_(false),
+      received_after_canceled_(0) {
     ChildProcessSecurityPolicyImpl::GetInstance()->Add(child_id());
     set_peer_pid_for_testing(base::GetCurrentProcId());
   }
 
+  void set_canceled(bool canceled) { canceled_ = canceled; }
+  int received_after_canceled() const { return received_after_canceled_; }
+
   // ResourceMessageFilter override
   virtual bool Send(IPC::Message* msg) OVERRIDE {
-    if (!dest_)
-      return false;
-    return dest_->Send(msg);
+    // No messages should be received when the process has been canceled.
+    if (canceled_)
+      received_after_canceled_++;
+    ReleaseHandlesInMessage(*msg);
+    delete msg;
+    return true;
   }
 
   ResourceContext* resource_context() { return resource_context_; }
 
  protected:
-  virtual ~ForwardingFilter() {}
+  virtual ~TestFilter() {}
 
  private:
   void GetContexts(const ResourceHostMsg_Request& request,
                    ResourceContext** resource_context,
                    net::URLRequestContext** request_context) {
     *resource_context = resource_context_;
     *request_context = resource_context_->GetRequestContext();
   }
 
+  ResourceContext* resource_context_;
+  bool canceled_;
+  int received_after_canceled_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestFilter);
+};
+
+
+// This class forwards the incoming messages to the ResourceDispatcherHostTest.
+// For the test, we want all the incoming messages to go to the same place,
+// which is why this forwards.
+class ForwardingFilter : public TestFilter {
+ public:
+  explicit ForwardingFilter(IPC::Sender* dest,
+                            ResourceContext* resource_context)
+      : TestFilter(resource_context),
+        dest_(dest) {
+  }
+
+  // TestFilter override
+  virtual bool Send(IPC::Message* msg) OVERRIDE {
+    return dest_->Send(msg);
+  }
+
+ private:
+  virtual ~ForwardingFilter() {}
+
   IPC::Sender* dest_;
-  ResourceContext* resource_context_;
 
   DISALLOW_COPY_AND_ASSIGN(ForwardingFilter);
 };
 
 // This class is a variation on URLRequestTestJob that will call
 // URLRequest::OnBeforeNetworkStart before starting.
 class URLRequestTestDelayedNetworkJob : public net::URLRequestTestJob {
  public:
   URLRequestTestDelayedNetworkJob(net::URLRequest* request,
                                   net::NetworkDelegate* network_delegate)
@@ skipping 388 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 778 matching lines @@
   ResourceIPCAccumulator::ClassifiedMessages msgs;
   accum_.GetClassifiedMessages(&msgs);
 
   // Check the cancellation
   ASSERT_EQ(1U, msgs.size());
   ASSERT_EQ(1U, msgs[0].size());
 
   CheckRequestCompleteErrorCode(msgs[0][0], net::ERR_ACCESS_DENIED);
 }
 
-// The host delegate acts as a second one so we can have some requests
-// pending and some canceled.
-class TestFilter : public ForwardingFilter {
- public:
-  explicit TestFilter(ResourceContext* resource_context)
-      : 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_++;
-    delete msg;
-    return true;
-  }
-
-  bool has_canceled_;
-  int received_after_canceled_;
-
- private:
-  virtual ~TestFilter() {}
-};
-
 // Tests CancelRequestsForProcess
 TEST_F(ResourceDispatcherHostTest, TestProcessCancel) {
   scoped_refptr<TestFilter> test_filter = new TestFilter(
       browser_context_->GetResourceContext());
   child_ids_.insert(test_filter->child_id());
 
   // request 1 goes to the test delegate
   ResourceHostMsg_Request request = CreateResourceRequest(
       "GET", ResourceType::SUB_RESOURCE, net::URLRequestTestJob::test_url_1());
 
@@ skipping 26 matching lines @@
   //EXPECT_EQ(3, host_.pending_requests());
 
   // Process test_url_2 and test_url_3 for one level so one callback is called.
   // We'll cancel test_url_4 (detachable) before processing it to verify that it
   // delays the cancel.
   for (int i = 0; i < 2; i++)
     EXPECT_TRUE(net::URLRequestTestJob::ProcessOnePendingMessage());
 
   // Cancel the requests to the test process.
   host_.CancelRequestsForProcess(filter_->child_id());
-  test_filter->has_canceled_ = true;
+  test_filter->set_canceled(true);
 
   // The requests should all be cancelled, except request 4, which is detached.
   EXPECT_EQ(1, host_.pending_requests());
   GlobalRequestID global_request_id(filter_->child_id(), 4);
   ResourceRequestInfoImpl* info = ResourceRequestInfoImpl::ForRequest(
       host_.GetURLRequest(global_request_id));
   ASSERT_TRUE(info->detachable_handler()->is_detached());
 
   // Flush all the pending requests.
   while (net::URLRequestTestJob::ProcessOnePendingMessage()) {}
 
   EXPECT_EQ(0, host_.pending_requests());
 
   // The test delegate should not have gotten any messages after being canceled.
-  ASSERT_EQ(0, test_filter->received_after_canceled_);
+  ASSERT_EQ(0, test_filter->received_after_canceled());
 
   // There should be two results.
   ResourceIPCAccumulator::ClassifiedMessages msgs;
   accum_.GetClassifiedMessages(&msgs);
   ASSERT_EQ(2U, msgs.size());
   CheckSuccessfulRequest(msgs[0], net::URLRequestTestJob::test_data_2());
   // The detachable request was cancelled by the renderer before it
   // finished. From the perspective of the renderer, it should have cancelled.
   ASSERT_EQ(2U, msgs[1].size());
   ASSERT_EQ(ResourceMsg_ReceivedResponse::ID, msgs[1][0].type());
@@ skipping 1372 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