Fix two URLRequestContext leaks in HttpBridgeTest....

Fix two URLRequestContext leaks in HttpBridgeTest and reenable the test.  It was previously failing the purify step.



BUG=19002
TEST=HttpBridgeTest



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

[eroman, 2009-08-12 04:08:39]

The change description says this fixes TWO leaks.
However I believe only 1 leak has been fixed here.

The leak that I see is on lines 38-39:

38       request_context_ = new HttpBridge::RequestContext(
39           new TestURLRequestContext());

Here "new TestURLRequestContext()" is leaked, because
HttpBridge::RequestContext's ctor doesn't make any effort to hold a reference to
its |baseline_context| (which causes another memory problem I will talk about
shortly).

For this issue, it seems you can use a simpler solution over the ReleaseSoon
business -- just pop it into a local scoped_refptr:

  scoped_refptr<URLRequestContext fake_context(new TestURLRequestContext());
  request_context_ = new HttpBridge::RequestContext(fake_context);


Now as for the "second leak fixed", I am assuming you believe the "new
HttpBridge::RequestContext(...)" to be leaking?

That seems a bit dodgy to me, since in reading HttpBridge I understand the
contract to be that HttpBridge manages the lifetime of its URLRequestContext,
taking care to delete it on io_loop_ during its dtor.

If that is not the case, then it seems like a bug in http_bridge.cc and not the
unittest.


Lastly, in browsing through http_bridge.cc, I spotted another memory ownership
problem:

HttpBridge::RequestContext::RequestContext(
    const URLRequestContext* baseline_context) {
  ...
  // We don't use a cache for bridged loads, but we do want to share proxy info.
  host_resolver_ = baseline_context->host_resolver();
  proxy_service_ = baseline_context->proxy_service();
  ...
}

The issue here is that ProxyService is NOT refcounted.
So it is only valid to share a ProxyService* with |baseline_context| if we
additionally hold onto a reference of |baseline_context|. Since otherwise if
|baseline_context| gets deleted, then our ProxyService* ends up pointing to free
memory and bad stuff happens.

For what its worth, this same mistake is made in ChromeURLRequestContext:
<http://crbug.com/15289>.

[tim (not reviewing), 2009-08-12 05:37:50]

On Tue, Aug 11, 2009 at 9:08 PM, <eroman@chromium.org> wrote:

> The change description says this fixes TWO leaks.
> However I believe only 1 leak has been fixed here.
>
> The leak that I see is on lines 38-39:
>
> 38       request_context_ = new HttpBridge::RequestContext(
> 39           new TestURLRequestContext());
>
> Here "new TestURLRequestContext()" is leaked, because
> HttpBridge::RequestContext's ctor doesn't make any effort to hold a
> reference to its |baseline_context| (which causes another memory problem
> I will talk about shortly).
>
> For this issue, it seems you can use a simpler solution over the
> ReleaseSoon business -- just pop it into a local scoped_refptr:
>
>  scoped_refptr<URLRequestContext fake_context(new
> TestURLRequestContext());
>  request_context_ = new HttpBridge::RequestContext(fake_context);


I didn't do this because of the ProxyService connection, which you also
mention below.

>
>
>
> Now as for the "second leak fixed", I am assuming you believe the "new
> HttpBridge::RequestContext(...)" to be leaking?
>
> That seems a bit dodgy to me, since in reading HttpBridge I understand
> the contract to be that HttpBridge manages the lifetime of its
> URLRequestContext, taking care to delete it on io_loop_ during its dtor.
>
> If that is not the case, then it seems like a bug in http_bridge.cc and
> not the unittest.

Actually, you're right this was not leaking;  I should probably revert this
part of the change.  I was playing around while investigating the leak, and
left it this way as it treats both URLRequestContexts in the test class the
same way.

>
>
>
> Lastly, in browsing through http_bridge.cc, I spotted another memory
> ownership problem:
>
> HttpBridge::RequestContext::RequestContext(
>    const URLRequestContext* baseline_context) {
>  ...
>  // We don't use a cache for bridged loads, but we do want to share
> proxy info.
>  host_resolver_ = baseline_context->host_resolver();
>  proxy_service_ = baseline_context->proxy_service();
>  ...
> }
>
> The issue here is that ProxyService is NOT refcounted.
> So it is only valid to share a ProxyService* with |baseline_context| if
> we additionally hold onto a reference of |baseline_context|. Since
> otherwise if |baseline_context| gets deleted, then our ProxyService*
> ends up pointing to free memory and bad stuff happens.
>
> For what its worth, this same mistake is made in
> ChromeURLRequestContext: <http://crbug.com/15289>.


Right; I implemented this part by copying verbatim what was done in
ChromeURLRequestContext. I actually spoke with some of the authors of that
code before uploading this patch about the issue, but didn't know there was
a bug on file for this.  I was going to ask you _why_  the ProxyService
wasn't refcounted, but when I re-examined the code in
ChromeURLRequestContext and saw it was the same over and over for several
different URLRequestContexts that are pretty important pieces of code (like
the one for Off the record), I assumed it magically was OK :(

>
>
>
> http://codereview.chromium.org/165363
>

[tim (not reviewing), 2009-08-12 05:39:50]

Forgot to mention; the "second" leak was the one previously on line 86 --
another "new TestURLRequestContext()"

On Tue, Aug 11, 2009 at 10:37 PM, Tim Steele <tim@chromium.org> wrote:

>
>
> On Tue, Aug 11, 2009 at 9:08 PM, <eroman@chromium.org> wrote:
>
>> The change description says this fixes TWO leaks.
>> However I believe only 1 leak has been fixed here.
>>
>> The leak that I see is on lines 38-39:
>>
>> 38       request_context_ = new HttpBridge::RequestContext(
>> 39           new TestURLRequestContext());
>>
>> Here "new TestURLRequestContext()" is leaked, because
>> HttpBridge::RequestContext's ctor doesn't make any effort to hold a
>> reference to its |baseline_context| (which causes another memory problem
>> I will talk about shortly).
>>
>> For this issue, it seems you can use a simpler solution over the
>> ReleaseSoon business -- just pop it into a local scoped_refptr:
>>
>>  scoped_refptr<URLRequestContext fake_context(new
>> TestURLRequestContext());
>>  request_context_ = new HttpBridge::RequestContext(fake_context);
>
>
> I didn't do this because of the ProxyService connection, which you also
> mention below.
>
>>
>>
>>
>> Now as for the "second leak fixed", I am assuming you believe the "new
>> HttpBridge::RequestContext(...)" to be leaking?
>>
>> That seems a bit dodgy to me, since in reading HttpBridge I understand
>> the contract to be that HttpBridge manages the lifetime of its
>> URLRequestContext, taking care to delete it on io_loop_ during its dtor.
>>
>> If that is not the case, then it seems like a bug in http_bridge.cc and
>> not the unittest.
>
> Actually, you're right this was not leaking;  I should probably revert this
> part of the change.  I was playing around while investigating the leak, and
> left it this way as it treats both URLRequestContexts in the test class the
> same way.
>
>>
>>
>>
>> Lastly, in browsing through http_bridge.cc, I spotted another memory
>> ownership problem:
>>
>> HttpBridge::RequestContext::RequestContext(
>>    const URLRequestContext* baseline_context) {
>>  ...
>>  // We don't use a cache for bridged loads, but we do want to share
>> proxy info.
>>  host_resolver_ = baseline_context->host_resolver();
>>  proxy_service_ = baseline_context->proxy_service();
>>  ...
>> }
>>
>> The issue here is that ProxyService is NOT refcounted.
>> So it is only valid to share a ProxyService* with |baseline_context| if
>> we additionally hold onto a reference of |baseline_context|. Since
>> otherwise if |baseline_context| gets deleted, then our ProxyService*
>> ends up pointing to free memory and bad stuff happens.
>>
>> For what its worth, this same mistake is made in
>> ChromeURLRequestContext: <http://crbug.com/15289>.
>
>
> Right; I implemented this part by copying verbatim what was done in
> ChromeURLRequestContext. I actually spoke with some of the authors of that
> code before uploading this patch about the issue, but didn't know there was
> a bug on file for this.  I was going to ask you _why_  the ProxyService
> wasn't refcounted, but when I re-examined the code in
> ChromeURLRequestContext and saw it was the same over and over for several
> different URLRequestContexts that are pretty important pieces of code (like
> the one for Off the record), I assumed it magically was OK :(
>
>>
>>
>>
>> http://codereview.chromium.org/165363
>>
>
>

[tim (not reviewing), 2009-08-12 17:36:58]

new snap up! I removed the request_context_ member from the test class as it
doesn't need to hold on to it; the HttpBridge instance takes a ref and releases
it.

[eroman, 2009-08-12 23:43:11]

LGTM