Fix HANDLE leak in ResourceDispatcherHost tests.

Fix HANDLE leak in ResourceDispatcherHost tests.

Ownership of the HANDLE in ResourceMsg_SetDataBuffer isn't managed
automatically.

Also invert ForwardingFilter/TestFilter's superclass/subclass
relationship. This arrangement avoids the useless dest_ = NULL
codepath.

BUG=366246
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266790

[davidben, 2014-04-25 16:02:14]

Pending a result from win_drmemory, but I think this should fix it, annoying as
it is. Making the IPC::Message own the HANDLE isn't going to be very easy with
how it's set up. I think the information would have to be in the message header.

[mmenke, 2014-04-25 16:07:23]

https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
content/browser/loader/resource_dispatcher_host_unittest.cc:231: return false;
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]

https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
content/browser/loader/resource_dispatcher_host_unittest.cc:231: return false;
On 2014/04/25 16:07:24, mmenke wrote:
> Shouldn't this be return true, because we ate the message and it shouldn't be
> sent anywhere else?

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

[davidben, 2014-04-25 16:24:36]

https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
content/browser/loader/resource_dispatcher_host_unittest.cc:231: return false;
On 2014/04/25 16:09:20, mmenke wrote:
> On 2014/04/25 16:07:24, mmenke wrote:
> > Shouldn't this be return true, because we ate the message and it shouldn't
be
> > sent anywhere else?
> 
> Oops...this is send, not receive...nevermind.

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.

[davidben, 2014-04-28 20:27:15]

On 2014/04/25 16:24:36, David Benjamin wrote:
>
https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
> File content/browser/loader/resource_dispatcher_host_unittest.cc (right):
> 
>
https://codereview.chromium.org/257763007/diff/1/content/browser/loader/resou...
> content/browser/loader/resource_dispatcher_host_unittest.cc:231: return false;
> On 2014/04/25 16:09:20, mmenke wrote:
> > On 2014/04/25 16:07:24, mmenke wrote:
> > > Shouldn't this be return true, because we ate the message and it shouldn't
> be
> > > sent anywhere else?
> > 
> > Oops...this is send, not receive...nevermind.
> 
> 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.

drmemory trybot doesn't seem to run content_unittests, but confirmed manually
that the leak's gone.

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

https://codereview.chromium.org/257763007/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/257763007/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_unittest.cc:203:
messages_.erase(messages_.begin());
Aren't we completely skipping the cleanup here?

https://codereview.chromium.org/257763007/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_unittest.cc:212: :
ResourceMessageFilter(
nit:  +2 indent (Know this was wrong before)

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

https://codereview.chromium.org/257763007/diff/20001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_unittest.cc (right):

https://codereview.chromium.org/257763007/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_unittest.cc:203:
messages_.erase(messages_.begin());
On 2014/04/28 20:52:08, mmenke wrote:
> Aren't we completely skipping the cleanup here?

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. :-)

https://codereview.chromium.org/257763007/diff/20001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_unittest.cc:212: :
ResourceMessageFilter(
On 2014/04/28 20:52:08, mmenke wrote:
> nit:  +2 indent (Know this was wrong before)

Bounced on tab in emacs a bit.

[mmenke, 2014-04-28 21:39:54]

LGTM...Well...Good enough, at least...

[davidben, 2014-04-28 22:34:39]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-04-28 22:35:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/257763007/100001

[commit-bot: I haz the power, 2014-04-28 23:27:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-28 23:27:16]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[davidben, 2014-04-28 23:35:56]

On 2014/04/28 23:27:16, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on win_chromium_rel

telemetry fell over again. :-/

[davidben, 2014-04-28 23:36:00]

The CQ bit was checked by davidben@chromium.org

[mmenke, 2014-04-28 23:36:49]

On 2014/04/28 23:35:56, David Benjamin wrote:
> On 2014/04/28 23:27:16, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   tryserver.chromium on win_chromium_rel
> 
> telemetry fell over again. :-/

Seen a lot of that lately...Really should just be disabled.

[commit-bot: I haz the power, 2014-04-28 23:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/257763007/100001

[commit-bot: I haz the power, 2014-04-29 08:54:49]

Change committed as 266790