When cancelling a pending request, release any outstanding SharedIOBuffer objects

When cancelling a pending request, release any outstanding SharedIOBuffer
objects and their associated shared memory segments. This avoids a shared
memory leak and file descriptor leak.

BUG=38383
TEST=Using test_chrome_fd_leak from bug 38383 and lsof, the renderer process
     should not show an fd leak on Mac or Linux.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=46007

[Mark Mentovai, 2010-04-28 22:49:15]

Darin, this is a high priority because it blocks M5. Let me know if you won’t be
able to get to it and I’ll look for another reviewer.

[darin (slow to review), 2010-04-29 21:28:13]

http://codereview.chromium.org/1703017/diff/1/2
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/1703017/diff/1/2#newcode510
chrome/common/resource_dispatcher.cc:510: MessageQueue& queue =
request_info.deferred_message_queue;
Should this be something that instead happens in ~PendingRequestInfo?

It seems like there should be a more uniform way (or a better catch-all) for
ensuring that these resources are freed.

At the very least, I see exactly the same code in RemovePendingRequest, so that
code should be factored out into a helper method so that it can be shared here. 
Is it the case that RemovePendingRequest is not being called?

[Mark Mentovai, 2010-04-29 22:00:10]

Right, RemovePendingRequest isn’t being called. Doing this in
~PendingRequestInfo was ineffective. In the version that I just uploaded, I
factored the common bits out into a helper function.

[Mark Mentovai, 2010-04-29 22:35:48]

I looked more closely and realized that because RemovePendingRequest wasn’t
being called, PendingRequestInfo objects were being leaked too. This version
fixes that problem by erasing the PendingRequestInfo object from within
CancelPendingRequest.

With PendingRequestInfo objects now being properly destroyed, I considered
moving the ReleaseResourcesInMessageQueue call into ~PendingRequestInfo. I
didn’t do that, however, because PendingRequestInfo objects live in
PendingRequestList hash_maps, meaning that their copy constructor and destructor
come into play. I didn’t want to risk ~PendingRequestInfo dumping a
deferred_message_queue of an object that’s going out of scope but whose
deferred_message_queue conceptually belongs to another PendingRequestInfo
created as a copy. That would surely cause problems. To avoid that, I’ve left
the ReleaseResourcesInMessageQueue calls in both RemovePendingRequest and
CancelPendingRequest.

[darin (slow to review), 2010-04-29 22:56:28]

LGTM, except...

http://codereview.chromium.org/1703017/diff/10001/11001
File chrome/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/1703017/diff/10001/11001#newcode497
chrome/common/resource_dispatcher.cc:497: DLOG(ERROR) << "unknown request";
Hmm... this DLOG should probably just be a WARNING.

http://codereview.chromium.org/1703017/diff/10001/11001#newcode504
chrome/common/resource_dispatcher.cc:504: // Avoid spamming the host with cancel
messages.
this spamming probably won't be possible now that you are removing this element
from pending_requests_.  nothing else seems to care about the is_cancelled flag.
 maybe it can be removed now.

[Mark Mentovai, 2010-04-29 23:27:09]

I checked this in with both of the suggested changes. That leaves us in a
situation where RemovePendingRequest and CancelPendingRequest are almost
identical, except that one DLOGs and returns a boolean. Perhaps it’s time to
fully unify them.

[darin (slow to review), 2010-04-30 03:46:31]

Agreed!

On Thu, Apr 29, 2010 at 4:27 PM, <mark@chromium.org> wrote:

> I checked this in with both of the suggested changes. That leaves us in a
> situation where RemovePendingRequest and CancelPendingRequest are almost
> identical, except that one DLOGs and returns a boolean. Perhaps it’s time
> to
> fully unify them.
>
>
> http://codereview.chromium.org/1703017/show
>