Changed UrlFetcher to use a MessageLoopProxy instead of directly relying on C...

Changed UrlFetcher to use a MessageLoopProxy instead of directly relying on ChromeThread. This will allow us to make UrlFetcher independent of ChromeThread and we can then move it to chrome/common.
BUG=None
TEST=UrlFetcher unit-tests


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

[darin (slow to review), 2010-04-29 23:17:39]

http://codereview.chromium.org/1702016/diff/1/8
File chrome/browser/net/url_fetcher.cc (left):

http://codereview.chromium.org/1702016/diff/1/8#oldcode167
chrome/browser/net/url_fetcher.cc:167:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
it seems a tad unfortunate to lose these assertions.  maybe MessageLoopProxy
could grow an IsCurrent (or IsForCurrentThread or some better name?) method,
which would test if the MessageLoopProxy refers to MessageLoop::current().

http://codereview.chromium.org/1702016/diff/1/11
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/1702016/diff/1/11#newcode29
chrome/browser/net/url_request_context_getter.h:29: virtual
scoped_refptr<MessageLoopProxy> GetIOMessageLoopProxy() = 0;
please document this with something like "Returns a MessageLoopProxy
corresponding to the thread on which the URLRequestContext may be used" or
something like that.

[eroman, 2010-04-29 23:48:10]

This approach looks good to me.

I agree with Darin, that preserving the asserts for being on the right thread is
important, and recommend adding that to the proxy interface.

http://codereview.chromium.org/1702016/diff/1/11
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/1702016/diff/1/11#newcode21
chrome/browser/net/url_request_context_getter.h:21: virtual
~URLRequestContextGetter() {}
Please leave the destructor as protected.

To make it compile, you will probably need:
friend class base::RefCountedThreadSafe<URLRequestContextGetter>;

(John put in a lot of effort to make all of the refcounted stuff have private
destructors, since that makes certain misuses result in compile errors)

[sanjeevr, 2010-04-29 23:55:36]

Made all the changes. Please take a look.

http://codereview.chromium.org/1702016/diff/1/8
File chrome/browser/net/url_fetcher.cc (left):

http://codereview.chromium.org/1702016/diff/1/8#oldcode167
chrome/browser/net/url_fetcher.cc:167:
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
On 2010/04/29 23:17:40, darin wrote:
> it seems a tad unfortunate to lose these assertions.  maybe MessageLoopProxy
> could grow an IsCurrent (or IsForCurrentThread or some better name?) method,
> which would test if the MessageLoopProxy refers to MessageLoop::current().

Done.

I thought about this at one point but I let laziness get the better of me
(should teach me not to write code during the client leads meeting :) )

http://codereview.chromium.org/1702016/diff/1/11
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/1702016/diff/1/11#newcode21
chrome/browser/net/url_request_context_getter.h:21: virtual
~URLRequestContextGetter() {}
On 2010/04/29 23:48:10, eroman wrote:
> Please leave the destructor as protected.
> 
> To make it compile, you will probably need:
> friend class base::RefCountedThreadSafe<URLRequestContextGetter>;
> 
> (John put in a lot of effort to make all of the refcounted stuff have private
> destructors, since that makes certain misuses result in compile errors)

Done.

http://codereview.chromium.org/1702016/diff/1/11#newcode29
chrome/browser/net/url_request_context_getter.h:29: virtual
scoped_refptr<MessageLoopProxy> GetIOMessageLoopProxy() = 0;
On 2010/04/29 23:17:40, darin wrote:
> please document this with something like "Returns a MessageLoopProxy
> corresponding to the thread on which the URLRequestContext may be used" or
> something like that.

Done.

[eroman, 2010-04-30 00:09:54]

LGTM

http://codereview.chromium.org/1702016/diff/21002/33009
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/1702016/diff/21002/33009#newcode244
chrome/browser/net/chrome_url_request_context.h:244: // the destructor and
GetURLRequestContext().
Please update comment regarding the destructor.

http://codereview.chromium.org/1702016/diff/21002/33009#newcode293
chrome/browser/net/chrome_url_request_context.h:293: // thread before the
instance is deleted on the IO thread.
minor nit: can you update this comment (remove the "on the IO thread").

[eroman, 2010-04-30 00:11:45]

http://codereview.chromium.org/1702016/diff/21002/33001
File chrome/test/testing_profile.cc (left):

http://codereview.chromium.org/1702016/diff/21002/33001#oldcode100
chrome/test/testing_profile.cc:100: // a leak if your test does not have a
ChromeThread::IO in it because
Isn't this comment still applicable?

[eroman, 2010-04-30 00:26:24]

I take back my lgtm: there is a subtle problem in http_bridge.cc that needs to
be fixed first.

HttpBridge::RequestContextGetter::~RequestContextGetter is no longer being
called on the IO thread.

This in turn means that HttpBridge::RequestContext is not being deleted from the
IO thread.... which in turn means the HttpNetworkLayer is not being deleted from
the IO thread, which will lead to badness.

[sanjeevr, 2010-04-30 00:37:49]

I thought about this case and (perhaps mistakenly) assumed it was OK. Do all
subclasses of UrlRequestContext have to be deleted on the IO thread? Or is
it just ChromeUrlRequestContext? If the answer is "all subclasses have to be
deleted on the IO thread", then I need to do more work.

On Thu, Apr 29, 2010 at 5:26 PM, <eroman@chromium.org> wrote:

> I take back my lgtm: there is a subtle problem in http_bridge.cc that needs
> to
> be fixed first.
>
> HttpBridge::RequestContextGetter::~RequestContextGetter is no longer being
> called on the IO thread.
>
> This in turn means that HttpBridge::RequestContext is not being deleted
> from the
> IO thread.... which in turn means the HttpNetworkLayer is not being deleted
> from
> the IO thread, which will lead to badness.
>
>
> http://codereview.chromium.org/1702016/show
>

[sanjeevr, 2010-04-30 00:42:23]

http://codereview.chromium.org/1702016/diff/21002/33001
File chrome/test/testing_profile.cc (left):

http://codereview.chromium.org/1702016/diff/21002/33001#oldcode100
chrome/test/testing_profile.cc:100: // a leak if your test does not have a
ChromeThread::IO in it because
On 2010/04/30 00:11:46, eroman wrote:
> Isn't this comment still applicable?

I don't think this comment is applicable any longer. You can delete
TestURLRequestContextGetter on any thread. There should not be a memory leak
even if you do not have an IO thread.

[eroman, 2010-04-30 01:14:03]

> I thought about this case and (perhaps mistakenly) assumed it was OK. Do all
> subclasses of UrlRequestContext have to be deleted on the IO thread? Or is
> it just ChromeUrlRequestContext? If the answer is "all subclasses have to be
> deleted on the IO thread", then I need to do more work.

For the most part, yes.

URLRequestContextGetter wraps a URLRequestContext.
It is intended for passing a URLRequestContext between threads.

However URLRequestContext is not thread safe, and hence must be
created/used/destroyed on the network (IO) thread.

So any implementationof URLRequestContextGetter that holds a reference to a
URLRequestContext, should be deleting it on the right thread.

Failure to do this can lead to strange problems when tearing down fields of
URLRequestContext.

[sanjeevr, 2010-04-30 01:20:26]

Got it. Working on the fix. Thanks a lot for the explanation.

On Thu, Apr 29, 2010 at 6:14 PM, <eroman@chromium.org> wrote:

> I thought about this case and (perhaps mistakenly) assumed it was OK. Do
>> all
>> subclasses of UrlRequestContext have to be deleted on the IO thread? Or is
>> it just ChromeUrlRequestContext? If the answer is "all subclasses have to
>> be
>> deleted on the IO thread", then I need to do more work.
>>
>
> For the most part, yes.
>
> URLRequestContextGetter wraps a URLRequestContext.
> It is intended for passing a URLRequestContext between threads.
>
> However URLRequestContext is not thread safe, and hence must be
> created/used/destroyed on the network (IO) thread.
>
> So any implementationof URLRequestContextGetter that holds a reference to a
> URLRequestContext, should be deleting it on the right thread.
>
> Failure to do this can lead to strange problems when tearing down fields of
> URLRequestContext.
>
>
> http://codereview.chromium.org/1702016/show
>

[sanjeevr, 2010-04-30 06:09:15]

OK so I have changed this so that URLRequestContextGetter now owns the context
pointer and takes care of deleting it on the correct thread so that the
subclasses don't have to bother doing that. Please take another look.

[sanjeevr, 2010-04-30 19:39:18]

Ping?

On Thu, Apr 29, 2010 at 11:09 PM, <sanjeevr@chromium.org> wrote:

> OK so I have changed this so that URLRequestContextGetter now owns the
> context
> pointer and takes care of deleting it on the correct thread so that the
> subclasses don't have to bother doing that. Please take another look.
>
>
> http://codereview.chromium.org/1702016/show
>

[darin (slow to review), 2010-04-30 20:58:02]

http://codereview.chromium.org/1702016/diff/31002/61010
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/1702016/diff/31002/61010#newcode19
chrome/browser/net/url_request_context_getter.h:19: : public
base::RefCountedThreadSafe<URLRequestContextGetter> {
now that GetIOMessageLoopProxy is available, why not use that as a means to
implement a RefCountedThreadSafe Traits class that deletes the
URLRequestContextGetter on the IO thread?

[eroman, 2010-04-30 21:06:29]

LGTM

http://codereview.chromium.org/1702016/diff/31002/61013
File chrome/browser/net/url_request_context_getter.cc (right):

http://codereview.chromium.org/1702016/diff/31002/61013#newcode22
chrome/browser/net/url_request_context_getter.cc:22: return
url_request_context_;
Can you similarly assert that this is only called from io_message_loop_proxy_ ?

http://codereview.chromium.org/1702016/diff/31002/61003
File chrome/browser/sync/glue/http_bridge.cc (right):

http://codereview.chromium.org/1702016/diff/31002/61003#newcode42
chrome/browser/sync/glue/http_bridge.cc:42:
static_cast<RequestContext*>(url_request_context().get());
nit: This feels awkward to keep the pointer to a scoped_refptr after it has been
destroyed. I know that it should work since the base holds a reference, but I
thin k it would be cleaner to hold onto the refcount.

http://codereview.chromium.org/1702016/diff/31002/61004
File chrome/browser/sync/glue/http_bridge.h (right):

http://codereview.chromium.org/1702016/diff/31002/61004#newcode67
chrome/browser/sync/glue/http_bridge.h:67: // The destructor MUST be called on
the IO thread.
nit: remove one space of indentation.

[sanjeevr, 2010-05-01 01:30:08]

I now have a new implementation using a trait. I am sorry for the multiple
iterations on this. I started out by thinking about using a trait. But I was
thinking about using a generic trait that could use a MessageLoopProxy and could
not think of a way to construct one. I did not think of creating a specific
trait just for URLRequestContextGetter. I now have that and so the code is
functionally like before, the URLRequestContextGetter is always deleted on the
IO thread.

Please take another look (sorry again for the multiple rounds).

http://codereview.chromium.org/1702016/diff/31002/61013
File chrome/browser/net/url_request_context_getter.cc (right):

http://codereview.chromium.org/1702016/diff/31002/61013#newcode22
chrome/browser/net/url_request_context_getter.cc:22: return
url_request_context_;
On 2010/04/30 21:06:29, eroman wrote:
> Can you similarly assert that this is only called from io_message_loop_proxy_
?

This is not relevant any longer because this code is gone.

http://codereview.chromium.org/1702016/diff/31002/61003
File chrome/browser/sync/glue/http_bridge.cc (right):

http://codereview.chromium.org/1702016/diff/31002/61003#newcode42
chrome/browser/sync/glue/http_bridge.cc:42:
static_cast<RequestContext*>(url_request_context().get());
On 2010/04/30 21:06:29, eroman wrote:
> nit: This feels awkward to keep the pointer to a scoped_refptr after it has
been
> destroyed. I know that it should work since the base holds a reference, but I
> thin k it would be cleaner to hold onto the refcount.

This is not relevant any longer because this code is gone.

http://codereview.chromium.org/1702016/diff/31002/61004
File chrome/browser/sync/glue/http_bridge.h (right):

http://codereview.chromium.org/1702016/diff/31002/61004#newcode67
chrome/browser/sync/glue/http_bridge.h:67: // The destructor MUST be called on
the IO thread.
On 2010/04/30 21:06:29, eroman wrote:
> nit: remove one space of indentation.

Done.

[eroman, 2010-05-03 17:55:28]

LGTM

http://codereview.chromium.org/1702016/diff/39002/47013
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/1702016/diff/39002/47013#newcode38
chrome/browser/net/url_request_context_getter.h:38: virtual
~URLRequestContextGetter() { }
please remove the space inside brackets.

(style guide says this in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...
)

[sanjeevr, 2010-05-03 18:05:53]

Done, waiting for Darin's review.

http://codereview.chromium.org/1702016/diff/39002/47013
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/1702016/diff/39002/47013#newcode38
chrome/browser/net/url_request_context_getter.h:38: virtual
~URLRequestContextGetter() { }
On 2010/05/03 17:55:29, eroman wrote:
> please remove the space inside brackets.
> 
> (style guide says this in
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Horizo...
> )

Done.

Yeah I wasn't sure of this. I normally never use this style and I prefer to use
braces just like a method that actually had a body, but I have had review
comments requesting this style recently :)