Move initialization of ChromeURLRequestContexts to the IO thread.

Move initialization of ChromeURLRequestContexts to the IO thread.

Before, these URLRequestContexts were lazily created from the UI thread. Unfortunately that model made it easy for consumers on the UI thread to poke at stuff which was being used from the IO thread, and introduce races.

So instead of providing a URLRequestContext*, the Profile now vends a URLRequestContextGetter*. 

The consequence of this is:

* Consumers on the UI thread can no longer get access to a URLRequestContext.
* Consumers on the IO thread need to call URLRequestContextGetter::GetURLRequestContext() to get at the context. This uses the same style lazy-creation of URLRequestContexts, albeit from the IO thread.

OK, so now the smelly part:

There were a couple of consumers of URLRequestContext on the UI thread that can't easily be moved to the IO thread -- these are the consumers of the cookie store. Before they could happily mess with the cookie store from the UI thread, and this was fine since CookieStore is threadsafe. However under the new model, they have no way to get at the URLRequestContext from the UI thread, hence can't get a pointer to the cookie store.

To support that use-cases, I bastardized the API some by adding a URLRequestContextGetter::GetCookieStore() method that lets UI thread consumers get a pointer to the cookie store, since we know this particular cross-thread usage is safe.

BUG=http://crbug.com/22294


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

[eroman, 2009-10-02 09:56:21]

@timsteele: Please review the changes to sync.

@michaeln: Please review the changes to app cache.

@amit: Please review the changes to automation_profile_impl. Note that I didn't
know where the tests for chromeframe were, so I haven't tested these changes :(
.... I am pretty sure this is going to break, as it doesn't look like the
current code releases ChromeURLRequestContext from the IO thread (so I expect to
trip the asserts I added). Any help appreciated.

@darin: Please review the CL as a whole.

[darin (slow to review), 2009-10-05 22:18:26]

i started reviewing this, but i think it'd be best for us to chat in person
before i finish.  i have some high level questions.

http://codereview.chromium.org/258008/diff/9174/5241
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/258008/diff/9174/5241#newcode26
Line 26: // TODO(eroman): Cleanup the declaration order in this file -- it is
all
how about one file per class?

http://codereview.chromium.org/258008/diff/9174/5218
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/258008/diff/9174/5218#newcode16
Line 16: // Interface for retrieving a URLRequestContext.
nit: a -> an

[eroman, 2009-10-06 17:16:56]

Per our discussion, I've renamed ChromeRequestContext -->
ChromeURLRequestContextGetter.

http://codereview.chromium.org/258008/diff/9174/5218
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/258008/diff/9174/5218#newcode16
Line 16: // Interface for retrieving a URLRequestContext.
On 2009/10/05 22:18:27, darin wrote:
> nit: a -> an

Done.

[darin (slow to review), 2009-10-06 18:08:40]

I wasn't sure if the ProxyService changes were strictly needed in this CL, but
they look fine to me.

I still wonder if things wouldn't be simpler if consumers referred to
URLRequestContexts by ID instead of by URLRequestContextGetter.

http://codereview.chromium.org/258008/diff/9174/5218
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/258008/diff/9174/5218#newcode16
Line 16: // Interface for retrieving a URLRequestContext.
it'd be good to add a comment about how this interface is intended to be used. 
i think you want to call to attention the issue that it must not live longer
than the the IO thread.  also, be sure to mention that GetURLRequestContext may
only be called on the IO thread.  point out that any thread may hold a reference
to URLRequestContextGetter.

http://codereview.chromium.org/258008/diff/167/10014
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/258008/diff/167/10014#newcode131
Line 131: new AutomationCookieStore(profile_,
couldn't it be troublesome to access profile_ here on the IO thread?

http://codereview.chromium.org/258008/diff/167/10015
File chrome/browser/automation/automation_profile_impl.h (left):

http://codereview.chromium.org/258008/diff/167/10015#oldcode48
Line 48: virtual ChromeAppCacheService* GetAppCacheService() {
was this change intended?

http://codereview.chromium.org/258008/diff/167/10013
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/258008/diff/167/10013#newcode42
Line 42: #include "chrome/browser/net/url_request_context_getter.h"
nit: sort

http://codereview.chromium.org/258008/diff/167/10013#newcode969
Line 969: tab->profile()->GetRequestContext()->GetCookieStore();
maybe the profile should just have a GetCookieStore method.  the distinction
between GetRequestContext and GetURLRequestContext is pretty subtle.  maybe this
is just cleanup fodder for another CL.

http://codereview.chromium.org/258008/diff/167/10031
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/258008/diff/167/10031#newcode271
Line 271: request_context_getter_ = profile->GetRequestContext();
maybe GetRequestContext should be named GetURLRequestContextGetter :-P

http://codereview.chromium.org/258008/diff/167/10043
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/258008/diff/167/10043#newcode338
Line 338: static_cast<ChromeURLRequestContextGetter*>(
nit: 3 space indenting should be fixed

http://codereview.chromium.org/258008/diff/167/10047
File chrome/browser/net/url_request_context_getter.cc (right):

http://codereview.chromium.org/258008/diff/167/10047#newcode11
Line 11: 
nit: extraneous new line?

http://codereview.chromium.org/258008/diff/167/10005
File chrome/browser/profile.h (right):

http://codereview.chromium.org/258008/diff/167/10005#newcode28
Line 28: class ChromeURLRequestContextGetter;
nit: C after B

http://codereview.chromium.org/258008/diff/167/10040
File chrome/common/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/258008/diff/167/10040#newcode24
Line 24: ->appcache_service();
nit: "->" on the preceding line

http://codereview.chromium.org/258008/diff/167/10051
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/258008/diff/167/10051#newcode48
Line 48: new net::ProxyConfigServiceFixed(net::ProxyConfig()));
nit: indent by 4 for line continuations

[eroman, 2009-10-06 19:19:51]

> I wasn't sure if the ProxyService changes were strictly needed in
> this CL, but they look fine to me.

Unfortunately it is necessary.

Before the ProxyService was being created on the UI thread; now it is being
created on the IO thread.

The thing is, the Linux implementation of ProxyConfigService
(ProxyConfigServiceLinux) is expecting to be called on the UI thread, as it
messes around with gconf.

So what I did was to extract the creation of ProxyConfigService out of
ProxyService::Create(), and make it into a required parameter instead.
That way we can create the ProxyConfigService dependency early on from the UI
thread, and save it into the ChromeURLRequestContextFactory.

Now when we need to create the ProxyService from the IO thread, we just pass off
the ProxyConfigService which was created earlier.

> I still wonder if things wouldn't be simpler if consumers referred
> to URLRequestContexts by ID instead of by URLRequestContextGetter.

Hmm, I am not convinced that this is any better.

If I understand the proposal, it is to pass around an opaque integer ID rather
than a ChromeURLRequestContext*.

This ID number would have to be globally unique across profiles (so the
incognito extensions context for example would have a different ID than the main
extensions context).

AFAICT this still has all the complexity of before (needing to initialize the
context somewhere, and tear it down). Although I rather than doing it explicity
from a constructor/destructor, there would be some global registry mapping IDs
to contexts (and Profiles would populate the mappings).

I think IDs would work less effectively in the following areas:

 - Consumers don't hold a reference.

Presumably if we are using an ID registry, the Profile would be the context
owner.
However, it should be noted that the Profile is destroyed BEFORE the IO thread.
This means consumers on the IO thread might try using an ID vended by a Profile
which is already deleted. The current technique sidesteps that problem since
URLRequestContextGetter is refcounted -- so ChromeURLRequestContext may outlive
the Profile that it belongs to, until all the IO thread consumers have released
their references.

 - More difficult for unit-tests.

Tests would probably have to insert their mock contexts into a global registry
that maps IDs to contexts. This also makes it harder to know what the strictest
thread guarantees that we can enforce are.

http://codereview.chromium.org/258008/diff/9174/5218
File chrome/browser/net/url_request_context_getter.h (right):

http://codereview.chromium.org/258008/diff/9174/5218#newcode16
Line 16: // Interface for retrieving a URLRequestContext.
On 2009/10/06 18:08:40, darin wrote:
> it'd be good to add a comment about how this interface is intended to be used.

> i think you want to call to attention the issue that it must not live longer
> than the the IO thread.  also, be sure to mention that GetURLRequestContext
may
> only be called on the IO thread.  point out that any thread may hold a
reference
> to URLRequestContextGetter.

I believe this usage documentation belongs in chrome_url_request_context.h
instead, since technically this header just describes an interface and not an
implementation.

(In particular as this interface is used in unit-tests that don't have the same
threading constraints, it may not be accurate to talk about the IO/UI thread).

http://codereview.chromium.org/258008/diff/167/10014
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/258008/diff/167/10014#newcode131
Line 131: new AutomationCookieStore(profile_,
On 2009/10/06 18:08:40, darin wrote:
> couldn't it be troublesome to access profile_ here on the IO thread?

Yeah, seems fishy to me too.

Note however that I am just translating the code to the new API -- it is already
making use of the profile from the IO thread as things stand today (SetCookie()
calls profile_->tab_handle()).

Maybe Amit knows more about this...

http://codereview.chromium.org/258008/diff/167/10015
File chrome/browser/automation/automation_profile_impl.h (left):

http://codereview.chromium.org/258008/diff/167/10015#oldcode48
Line 48: virtual ChromeAppCacheService* GetAppCacheService() {
On 2009/10/06 18:08:40, darin wrote:
> was this change intended?

Yes, this is intentional.

Since AppCacheService is an object that lives on the IO thread, it seems
ill-advised to export it from the Profile. 

Now it lives exclusively in the ChromeURLRequestContext (is initialized and
destroyed from the IO thread).

http://codereview.chromium.org/258008/diff/167/10013
File chrome/browser/automation/automation_provider.cc (right):

http://codereview.chromium.org/258008/diff/167/10013#newcode42
Line 42: #include "chrome/browser/net/url_request_context_getter.h"
On 2009/10/06 18:08:40, darin wrote:
> nit: sort

Done.

http://codereview.chromium.org/258008/diff/167/10013#newcode969
Line 969: tab->profile()->GetRequestContext()->GetCookieStore();
On 2009/10/06 18:08:40, darin wrote:
> maybe the profile should just have a GetCookieStore method.  the distinction
> between GetRequestContext and GetURLRequestContext is pretty subtle.  maybe
this
> is just cleanup fodder for another CL.

Yeah, perhaps.

The problem is going to be there are callers that use the "default request
context":

  Profile::GetDefaultRequestContext()->GetCookieStore()

In order to directly translate we would need to have a static "default profile"
too.

http://codereview.chromium.org/258008/diff/167/10031
File chrome/browser/download/save_package.cc (right):

http://codereview.chromium.org/258008/diff/167/10031#newcode271
Line 271: request_context_getter_ = profile->GetRequestContext();
On 2009/10/06 18:08:40, darin wrote:
> maybe GetRequestContext should be named GetURLRequestContextGetter :-P

Agreed.

However I would prefer to leave that as a post-task, since it will bring in more
files.

http://codereview.chromium.org/258008/diff/167/10043
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/258008/diff/167/10043#newcode338
Line 338: static_cast<ChromeURLRequestContextGetter*>(
On 2009/10/06 18:08:40, darin wrote:
> nit: 3 space indenting should be fixed

Done.

http://codereview.chromium.org/258008/diff/167/10047
File chrome/browser/net/url_request_context_getter.cc (right):

http://codereview.chromium.org/258008/diff/167/10047#newcode11
Line 11: 
On 2009/10/06 18:08:40, darin wrote:
> nit: extraneous new line?

Done.

http://codereview.chromium.org/258008/diff/167/10005
File chrome/browser/profile.h (right):

http://codereview.chromium.org/258008/diff/167/10005#newcode28
Line 28: class ChromeURLRequestContextGetter;
On 2009/10/06 18:08:40, darin wrote:
> nit: C after B

Done.

http://codereview.chromium.org/258008/diff/167/10040
File chrome/common/appcache/appcache_dispatcher_host.cc (right):

http://codereview.chromium.org/258008/diff/167/10040#newcode24
Line 24: ->appcache_service();
On 2009/10/06 18:08:40, darin wrote:
> nit: "->" on the preceding line

Done.

http://codereview.chromium.org/258008/diff/167/10051
File webkit/tools/test_shell/test_shell_request_context.cc (right):

http://codereview.chromium.org/258008/diff/167/10051#newcode48
Line 48: new net::ProxyConfigServiceFixed(net::ProxyConfig()));
On 2009/10/06 18:08:40, darin wrote:
> nit: indent by 4 for line continuations

Done.

[darin (slow to review), 2009-10-06 20:06:16]

On Tue, Oct 6, 2009 at 12:19 PM, <eroman@chromium.org> wrote:

> I wasn't sure if the ProxyService changes were strictly needed in
>> this CL, but they look fine to me.
>>
>
> Unfortunately it is necessary.
>
>
>
OK



>  I still wonder if things wouldn't be simpler if consumers referred
>> to URLRequestContexts by ID instead of by URLRequestContextGetter.
>>
>
> Hmm, I am not convinced that this is any better.
>
> If I understand the proposal, it is to pass around an opaque integer ID
> rather
> than a ChromeURLRequestContext*.
>
> This ID number would have to be globally unique across profiles (so the
> incognito extensions context for example would have a different ID than the
> main
> extensions context).
>
> AFAICT this still has all the complexity of before (needing to initialize
> the
> context somewhere, and tear it down). Although I rather than doing it
> explicity
> from a constructor/destructor, there would be some global registry mapping
> IDs
> to contexts (and Profiles would populate the mappings).
>
> I think IDs would work less effectively in the following areas:
>
>  - Consumers don't hold a reference.
>
> Presumably if we are using an ID registry, the Profile would be the context
> owner.
>

I imagined that something living purely on the IO thread could own the
registry,
and during IO thread cleanup, we could destroy this registry, which would
destroy the URLRequestContext instances.

At startup, the Profile could send messages to the IO thread to create and
register the various URLRequestContext instances.

If a consumer can get to the IO thread, then the ID would be usable on that
thread
to get an URLRequestContext.




> However, it should be noted that the Profile is destroyed BEFORE the IO
> thread.
> This means consumers on the IO thread might try using an ID vended by a
> Profile
> which is already deleted. The current technique sidesteps that problem
> since
> URLRequestContextGetter is refcounted -- so ChromeURLRequestContext may
> outlive
> the Profile that it belongs to, until all the IO thread consumers have
> released
> their references.
>
>  - More difficult for unit-tests.
>
> Tests would probably have to insert their mock contexts into a global
> registry
> that maps IDs to contexts. This also makes it harder to know what the
> strictest
> thread guarantees that we can enforce are.
>
>
Good point, but we already do this for ChromeThread, and it is not that
problematic.



>
> http://codereview.chromium.org/258008/diff/9174/5218
> File chrome/browser/net/url_request_context_getter.h (right):
>
> http://codereview.chromium.org/258008/diff/9174/5218#newcode16
> Line 16: // Interface for retrieving a URLRequestContext.
> On 2009/10/06 18:08:40, darin wrote:
>
>> it'd be good to add a comment about how this interface is intended to
>>
> be used.
>
>> i think you want to call to attention the issue that it must not live
>>
> longer
>
>> than the the IO thread.  also, be sure to mention that
>>
> GetURLRequestContext may
>
>> only be called on the IO thread.  point out that any thread may hold a
>>
> reference
>
>> to URLRequestContextGetter.
>>
>
> I believe this usage documentation belongs in
> chrome_url_request_context.h instead, since technically this header just
> describes an interface and not an implementation.
>
> (In particular as this interface is used in unit-tests that don't have
> the same threading constraints, it may not be accurate to talk about the
> IO/UI thread).
>

Good points!



>
> http://codereview.chromium.org/258008/diff/167/10014
> File chrome/browser/automation/automation_profile_impl.cc (right):
>
> http://codereview.chromium.org/258008/diff/167/10014#newcode131
> Line 131: new AutomationCookieStore(profile_,
> On 2009/10/06 18:08:40, darin wrote:
>
>> couldn't it be troublesome to access profile_ here on the IO thread?
>>
>
> Yeah, seems fishy to me too.
>
> Note however that I am just translating the code to the new API -- it is
> already making use of the profile from the IO thread as things stand
> today (SetCookie() calls profile_->tab_handle()).
>
> Maybe Amit knows more about this...
>

OK, seems like a reasonable follow-up issue.



>
> http://codereview.chromium.org/258008/diff/167/10015
> File chrome/browser/automation/automation_profile_impl.h (left):
>
> http://codereview.chromium.org/258008/diff/167/10015#oldcode48
> Line 48: virtual ChromeAppCacheService* GetAppCacheService() {
> On 2009/10/06 18:08:40, darin wrote:
>
>> was this change intended?
>>
>
> Yes, this is intentional.
>
> Since AppCacheService is an object that lives on the IO thread, it seems
> ill-advised to export it from the Profile.
>
> Now it lives exclusively in the ChromeURLRequestContext (is initialized
> and destroyed from the IO thread).



Great.


>
> http://codereview.chromium.org/258008/diff/167/10013#newcode969
> Line 969: tab->profile()->GetRequestContext()->GetCookieStore();
> On 2009/10/06 18:08:40, darin wrote:
>
>> maybe the profile should just have a GetCookieStore method.  the
>>
> distinction
>
>> between GetRequestContext and GetURLRequestContext is pretty subtle.
>>
> maybe this
>
>> is just cleanup fodder for another CL.
>>
>
> Yeah, perhaps.
>
> The problem is going to be there are callers that use the "default
> request context":
>
>  Profile::GetDefaultRequestContext()->GetCookieStore()
>
> In order to directly translate we would need to have a static "default
> profile" too.


Oh, I see.  That would suck.



>
>
> http://codereview.chromium.org/258008/diff/167/10031
> File chrome/browser/download/save_package.cc (right):
>
> http://codereview.chromium.org/258008/diff/167/10031#newcode271
> Line 271: request_context_getter_ = profile->GetRequestContext();
> On 2009/10/06 18:08:40, darin wrote:
>
>> maybe GetRequestContext should be named GetURLRequestContextGetter :-P
>>
>
> Agreed.
>
> However I would prefer to leave that as a post-task, since it will bring
> in more files.
>

Yeah, makes sense.

-Darin


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

[amit, 2009-10-08 00:42:53]

The relevant test for Chrome frame is: CFACWithChrome_UseHostNetworkStack

The concern about ChromeUrlRequestContext getting deleted in the UI thread is a
real one. Now since the AutomationProfile is just a wrapper over the real
profile and the AutomationRequestContext is also a wrapper, the Release of the
Refptr will happen in the UI thread but the actual instance might get deleted
later in the IO thread.

Given the complexity, I guess it's better to talk in person about the background
of this change and how it may affect Chrome Frame.

[michaeln, 2009-10-09 05:05:46]

These changes don't simplify things too much. They do address some correctness
and allow us to use non-thread-safe refcounts (goodness), but its still way
complicated (maybe even more so).

http://codereview.chromium.org/258008/diff/11001/11044
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/258008/diff/11001/11044#newcode267
Line 267: // main URLRequestContext that we just created).
There are two relationships involved...

a. context->set_appcache_service(appcache_service)

That means when resources are loaded using that context, those resource are
subject to retrieval from the appcache_service rather than hitting the network.

b. appcache_service->set_request_context(url_request_context)

That means when the appcache_service tries to update/validate appcaches, it will
issue requests thru url_request_context (using its cookie_store and http_cache).
(Those requests explicitly bypass the appcache relationship described in a).

-----

A profile has exactly one AppCacheService instance.
* The 'original' request context for the profile does (a & b)
* The 'media' context does (a) only
* The 'extension' context does neither

Ditto for the OTRProfile, it has a seperate AppCacheService instance.
* The 'original' request context for the OTR profile does (a & b)
* The 'media' context does (a) only
* The 'extension' context does neither

-----

Feel free to consult my whiteboard, the diagram is still there :)

http://codereview.chromium.org/258008/diff/11001/11044#newcode400
Line 400: context->appcache_service()->set_request_context(original_context);
s/b set_request_context(context)

This AppCacheService for the OTR profile should issue requests using the OTR
original context when it runs its update jobs. See earlier comments.

<aside>
'main' may be a better term than 'original'
</aside>

http://codereview.chromium.org/258008/diff/11001/11044#newcode481
Line 481: original_context_getter_->GetIOContext();
I'm guessing that when dealing with an OTR profile, this in the OTR specific
'origin_context'... is that right?

http://codereview.chromium.org/258008/diff/11001/11044#newcode520
Line 520: context->set_http_transaction_factory(cache);
The "mediaContext" should have a reference to the same appcache_service as the
"originalContext" for its profile. See earlier comments.

context->set_appcache_service(original_context->appcache_service());

[eroman, 2009-10-12 23:49:51]

Thanks Michael, I believe I have addressed the sharing issue with
AppCacheService between incognito and media contexts.

Please review the changes here:

http://codereview.chromium.org/258008/diff2/11001:16001/17043

Also, here is my mental image of how those pieces are supposed to fit together:

http://www.corp.google.com/~ericroman/chrome/tmp/appcache_regular.png

http://www.corp.google.com/~ericroman/chrome/tmp/appcache_incognito.png

[michaeln, 2009-10-13 06:04:50]

Yup, those changes look right to me, thnx!

[tim (not reviewing), 2009-10-16 05:21:58]

On 2009/10/13 06:04:50, michaeln wrote:
> Yup, those changes look right to me, thnx!

So, has the dust mostly settled here? :)

[michaeln, 2009-10-16 16:48:59]

On 2009/10/16 05:21:58, timsteele wrote:
> On 2009/10/13 06:04:50, michaeln wrote:
> > Yup, those changes look right to me, thnx!
> 
> So, has the dust mostly settled here? :)

I haven't looked thru most of this CL too closely, i focused on just the changes
to appcache... and that part is in good shape.

[eroman, 2009-10-16 18:23:37]

On Thu, Oct 15, 2009 at 10:21 PM,  <tim@chromium.org> wrote:
> On 2009/10/13 06:04:50, michaeln wrote:
>>
>> Yup, those changes look right to me, thnx!
>
> So, has the dust mostly settled here? :)

Yes, please do review!

(I just have one change I will be making to
automation_profile_impl.cc, after speaking to Amit on how that stuff
works)

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

[eroman, 2009-10-16 21:17:29]

@amit:

I made the changes to AutomationProfileImpl that we discussed, please review
those deltas here:

http://codereview.chromium.org/258008/diff2/16001:21001/21016
http://codereview.chromium.org/258008/diff2/16001:21001/21015

[eroman, 2009-10-16 21:20:33]

http://codereview.chromium.org/258008/diff/167/10014
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/258008/diff/167/10014#newcode131
Line 131: new AutomationCookieStore(profile_,
On 2009/10/06 18:08:40, darin wrote:
> couldn't it be troublesome to access profile_ here on the IO thread?

@darin: Fixed this here:
 http://codereview.chromium.org/258008/diff2/16001:21001/21015

[amit, 2009-10-16 22:21:51]

http://codereview.chromium.org/258008/diff/21001/21015
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/258008/diff/21001/21015#newcode121
Line 121: tab_handle_(profile->tab_handle()),
There's an issue here. This happens in the context of:
AutomationProvider::CreateExternalTab
  ExternalTabContainer::Init
    AutomationProfileImpl::Initialize
The profile does not have a tab handle available at this point of time. It gets
set later in AutomationProvider::CreateExternalTab.

The correct thing to do is probably to set the tab handle in
ExternalTabContainer::set_tab_handle by scheduling it on the IO thread.

[eroman, 2009-10-17 00:59:31]

http://codereview.chromium.org/258008/diff/21001/21015
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/258008/diff/21001/21015#newcode121
Line 121: tab_handle_(profile->tab_handle()),
On 2009/10/16 22:21:51, amit wrote:
> There's an issue here. This happens in the context of:
> AutomationProvider::CreateExternalTab
>   ExternalTabContainer::Init
>     AutomationProfileImpl::Initialize
> The profile does not have a tab handle available at this point of time. It
gets
> set later in AutomationProvider::CreateExternalTab.
> 
> The correct thing to do is probably to set the tab handle in
> ExternalTabContainer::set_tab_handle by scheduling it on the IO thread.

@Amit: I have removed the tab_handle change, and restored the earlier behavior.
Instead I will add a TODO to fix this later (trying to limit how many changes
this CL makes).

[amit, 2009-10-17 14:21:11]

OK, lgtm

On Fri, Oct 16, 2009 at 5:59 PM, <eroman@chromium.org> wrote:

>
> http://codereview.chromium.org/258008/diff/21001/21015
> File chrome/browser/automation/automation_profile_impl.cc (right):
>
> http://codereview.chromium.org/258008/diff/21001/21015#newcode121
> Line 121: tab_handle_(profile->tab_handle()),
> On 2009/10/16 22:21:51, amit wrote:
>
>> There's an issue here. This happens in the context of:
>> AutomationProvider::CreateExternalTab
>>   ExternalTabContainer::Init
>>     AutomationProfileImpl::Initialize
>> The profile does not have a tab handle available at this point of
>>
> time. It gets
>
>> set later in AutomationProvider::CreateExternalTab.
>>
>
>  The correct thing to do is probably to set the tab handle in
>> ExternalTabContainer::set_tab_handle by scheduling it on the IO
>>
> thread.
>
> @Amit: I have removed the tab_handle change, and restored the earlier
> behavior.
> Instead I will add a TODO to fix this later (trying to limit how many
> changes this CL makes).
>
>
> http://codereview.chromium.org/258008
>

[tim (not reviewing), 2009-10-19 19:01:32]

http://codereview.chromium.org/258008/diff/21001/21008
File chrome/browser/sync/glue/http_bridge.cc (left):

http://codereview.chromium.org/258008/diff/21001/21008#oldcode156
Line 156: DCHECK(context_for_request_->is_user_agent_set()) << "User agent not
set";
I think we could leave this in by doing
context_getter_for_request_->is_user_agent_set() ?

http://codereview.chromium.org/258008/diff/21001/21008
File chrome/browser/sync/glue/http_bridge.cc (right):

http://codereview.chromium.org/258008/diff/21001/21008#newcode12
Line 12: #include "chrome/browser/net/url_request_context_getter.h"
you can remove this, already included in the .h

http://codereview.chromium.org/258008/diff/21001/21008#newcode31
Line 31: }
this could go in the header

http://codereview.chromium.org/258008/diff/21001/21008#newcode39
Line 39: baseline_context_getter_ = NULL;
I'm not sure how useful it is to do this lazily since there is no legitimate
case where we create an HttpBridge and won't use a context, and it would
simplify things to not have baseline_context_getter_ as a member variable. 
Unless I'm missing something? Could we just create context_ in the ctor?

http://codereview.chromium.org/258008/diff/21001/21009
File chrome/browser/sync/glue/http_bridge.h (right):

http://codereview.chromium.org/258008/diff/21001/21009#newcode126
Line 126: URLRequestContextGetter* GetRequestContextGetter() const;
where is this used now? Is it just the test? Could we #if defined (UNIT_TEST)?
There are just so many rq ctx getters around that I'm getting confused so it
would be nice to single some out if possible...

http://codereview.chromium.org/258008/diff/21001/21009#newcode140
Line 140: // Gets a customized URLRequestContextGetter for bridged requests. See
I think this actually _is_ "A customized URLRequestContextGetter for bridged
requests", yes?  But I think the spirit of the comment is about the actual
context so maybe "Gets a customized URLRequestContext for bridged requests."

[eroman, 2009-10-20 04:02:57]

http://codereview.chromium.org/258008/diff/21001/21008
File chrome/browser/sync/glue/http_bridge.cc (left):

http://codereview.chromium.org/258008/diff/21001/21008#oldcode156
Line 156: DCHECK(context_for_request_->is_user_agent_set()) << "User agent not
set";
On 2009/10/19 19:01:32, timsteele wrote:
> I think we could leave this in by doing
> context_getter_for_request_->is_user_agent_set() ?

So at first glance, one's expectation is that is_user_agent_set() should return
false if set_user_agent() hasn't been called. However that was not the case.

Instead, because RequestContext's constructor was initializing the user agent to
webkit_glue::GetUserAgent(), this check was meaningless, since that will never
be empty string.

Now, when I translated this to using the URLRequestContextGetter, I ended up
implementing is_user_agent_set() to actually correspond with whether
set_user_agent() had been called. But now that means I have to remove the
DCHECK, since it is no longer true (there are tests which never call
set_user_agent(), and are simply relying on the default user agent provided by
webkit_glue).

http://codereview.chromium.org/258008/diff/21001/21008
File chrome/browser/sync/glue/http_bridge.cc (right):

http://codereview.chromium.org/258008/diff/21001/21008#newcode12
Line 12: #include "chrome/browser/net/url_request_context_getter.h"
On 2009/10/19 19:01:32, timsteele wrote:
> you can remove this, already included in the .h

Done.

http://codereview.chromium.org/258008/diff/21001/21008#newcode31
Line 31: }
On 2009/10/19 19:01:32, timsteele wrote:
> this could go in the header

Done.
(Deleted it all together).

http://codereview.chromium.org/258008/diff/21001/21008#newcode39
Line 39: baseline_context_getter_ = NULL;
On 2009/10/19 19:01:32, timsteele wrote:
> I'm not sure how useful it is to do this lazily since there is no legitimate
> case where we create an HttpBridge and won't use a context, and it would
> simplify things to not have baseline_context_getter_ as a member variable. 
> Unless I'm missing something? Could we just create context_ in the ctor?

So this is basically the entire premise of my changelist -- I am trying to
offload the construction of URLRequestContexts to the IO thread.

In order to do that, we can only access and initialize contexts from the IO
thread.

While it certainly would simplify things to build the request context as part of
the constructor, it would now no longer fit that invariant.

(URLRequestContextGetter:GetURLRequestContext() is only supposed to be called
from the IO thread. I would like to put a DCHECK to that effect in here, but
that would mean having to pass around the IO loop used by unit-tests too).

http://codereview.chromium.org/258008/diff/21001/21009
File chrome/browser/sync/glue/http_bridge.h (right):

http://codereview.chromium.org/258008/diff/21001/21009#newcode126
Line 126: URLRequestContextGetter* GetRequestContextGetter() const;
On 2009/10/19 19:01:32, timsteele wrote:
> where is this used now? Is it just the test? Could we #if defined (UNIT_TEST)?
> There are just so many rq ctx getters around that I'm getting confused so it
> would be nice to single some out if possible...

Done.
(Indeed, this is a unit-test-only method).

http://codereview.chromium.org/258008/diff/21001/21009#newcode140
Line 140: // Gets a customized URLRequestContextGetter for bridged requests. See
On 2009/10/19 19:01:32, timsteele wrote:
> I think this actually _is_ "A customized URLRequestContextGetter for bridged
> requests", yes?  But I think the spirit of the comment is about the actual
> context so maybe "Gets a customized URLRequestContext for bridged requests."

Done.
(Indeed, you are right).

[tim (not reviewing), 2009-10-20 05:48:36]

Sync changes LGTM

http://codereview.chromium.org/258008/diff/21001/21008
File chrome/browser/sync/glue/http_bridge.cc (left):

http://codereview.chromium.org/258008/diff/21001/21008#oldcode156
Line 156: DCHECK(context_for_request_->is_user_agent_set()) << "User agent not
set";
On 2009/10/20 04:02:57, eroman wrote:
> On 2009/10/19 19:01:32, timsteele wrote:
> > I think we could leave this in by doing
> > context_getter_for_request_->is_user_agent_set() ?
> 
> So at first glance, one's expectation is that is_user_agent_set() should
return
> false if set_user_agent() hasn't been called. However that was not the case.
> 
> Instead, because RequestContext's constructor was initializing the user agent
to
> webkit_glue::GetUserAgent(), this check was meaningless, since that will never
> be empty string.
> 
> Now, when I translated this to using the URLRequestContextGetter, I ended up
> implementing is_user_agent_set() to actually correspond with whether
> set_user_agent() had been called. But now that means I have to remove the
> DCHECK, since it is no longer true (there are tests which never call
> set_user_agent(), and are simply relying on the default user agent provided by
> webkit_glue).
> 

Oh.  Thanks for getting to the bottom of things.  I feel like those tests should
probably set the agent, but fixing our tests is not something you need to worry
about. I'll file a bug to investigate.  (The point for the dcheck is to make
sure we send the sync-client-specific agent string to sync servers for our
logging, should someone ever forget when adding a new use of the bridge. 
Although now that we're not firewalled behind the syncapi.dll, we could probably
just pass it in on construction...) Anyway, sorry to bother you with that!

http://codereview.chromium.org/258008/diff/21001/21008
File chrome/browser/sync/glue/http_bridge.cc (right):

http://codereview.chromium.org/258008/diff/21001/21008#newcode39
Line 39: baseline_context_getter_ = NULL;
On 2009/10/20 04:02:57, eroman wrote:
> On 2009/10/19 19:01:32, timsteele wrote:
> > I'm not sure how useful it is to do this lazily since there is no legitimate
> > case where we create an HttpBridge and won't use a context, and it would
> > simplify things to not have baseline_context_getter_ as a member variable. 
> > Unless I'm missing something? Could we just create context_ in the ctor?
> 
> So this is basically the entire premise of my changelist -- I am trying to
> offload the construction of URLRequestContexts to the IO thread.
> 
> In order to do that, we can only access and initialize contexts from the IO
> thread.
> 
> While it certainly would simplify things to build the request context as part
of
> the constructor, it would now no longer fit that invariant.
> 
> (URLRequestContextGetter:GetURLRequestContext() is only supposed to be called
> from the IO thread. I would like to put a DCHECK to that effect in here, but
> that would mean having to pass around the IO loop used by unit-tests too).

Heh, indeed, I missed the point altogether :) Well, I think I was confused as to
on which thread the HttpBridge RequestContextGetter was constructed, but I see
it now.  Sorry for the confusion!

[darin (slow to review), 2009-10-20 17:35:52]

LGTM

good luck with the landing!