Make ProfileImplIOData::Handle::GetResourceContext() imply LazyInitialize().

Make ProfileImplIOData::Handle::GetResourceContext() imply LazyInitialize().

This is the next step to moving the AppCache and other storage related contexts into IOData.

BUG=85121
TEST=existing.

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

[willchan no longer on Chromium, 2011-11-29 01:49:39]

Why is this necessary? LazyInitialize() initializes a bunch of objects which are
designed to be constructed and destroyed on the IO thread.
Handle::GetResourceContext() is designed to be called on the UI thread.

[awong, 2011-11-29 02:01:15]

I want to get rid of BrowserContext::GetFileSystemContext(),
BrowserContext::GetWebKitContext(), etc. and have everyone access them through
the ResourceContext structure.

However, this means that GetResourceContext() should LazyInitialize() the IOData
structure.

Unforutnately, AppCache, and PluginService both need to be able to take a
reference on the ResourceContext() w/o it initializing the IOData structure. 
Thus, I'm breaking it into 2 functions with the intention of killing off 
GetResourceContextNoInit() after I move the AppCache construction into a more
suitable spot.

Don't quite have a plan for PluginService yet unfortunately.

[willchan no longer on Chromium, 2011-11-29 02:05:39]

How do you intend to address the issue I pointed out earlier with the
ResourceContext members expecting to be initialized on the IO thread?

On Mon, Nov 28, 2011 at 6:01 PM, <ajwong@chromium.org> wrote:

> I want to get rid of BrowserContext::**GetFileSystemContext(),
> BrowserContext::**GetWebKitContext(), etc. and have everyone access them
> through
> the ResourceContext structure.
>

What's wrong with using BrowserContext? BrowserContext is intended to be
used on the UI thread. ResourceContext is for use on the IO thread.


>
> However, this means that GetResourceContext() should LazyInitialize() the
> IOData
> structure.
>
> Unforutnately, AppCache, and PluginService both need to be able to take a
> reference on the ResourceContext() w/o it initializing the IOData
> structure.
> Thus, I'm breaking it into 2 functions with the intention of killing off
> GetResourceContextNoInit() after I move the AppCache construction into a
> more
> suitable spot.
>
> Don't quite have a plan for PluginService yet unfortunately.
>
>
http://codereview.chromium.**org/8707002/<http://codereview.chromium.org/8707...
>

[awong, 2011-11-29 02:47:14]

On Mon, Nov 28, 2011 at 6:05 PM, William Chan (陈智昌)
<willchan@chromium.org>wrote:

> How do you intend to address the issue I pointed out earlier with the
> ResourceContext members expecting to be initialized on the IO thread?
>

I don't think this CL changes the effective initialization sequence at
all...

All that LazyInitialize() accomplishes is the profile_params_ structure is
snapshotted from Profile when GetResourceContext() is called.
 ResourceContext is still only initialized from the profile_params_ when
someone does accesses a function inside ResourceContext.  Since there is no
change in who is given a reference to the ResourceContext, or who is
calling functions on those objects, there should be no change in when/where
ResourceContext is initialized.

Without this change, I think it's possible for someone to say:

  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, Bind(&DoSomething,
&Handle::GetResourceContext()));

before anyone calls io_data->InitializeOnUIThread().  In this case, if
DoSomething() uses any accessor on the passed in ResourceContext, I think
it will get a NULL profile_params_ and segfault.  Making it so
GetResourceContext() ensures LazyInitialize() is called avoids this API
hole.



> On Mon, Nov 28, 2011 at 6:01 PM, <ajwong@chromium.org> wrote:
>
>> I want to get rid of BrowserContext::**GetFileSystemContext(),
>> BrowserContext::**GetWebKitContext(), etc. and have everyone access them
>> through
>> the ResourceContext structure.
>>
>
> What's wrong with using BrowserContext? BrowserContext is intended to be
> used on the UI thread. ResourceContext is for use on the IO thread.
>

Oops...this is something I completely missed.  I saw 2 parallel APIs and
was assuming that we were just in the middle of migrating from one
container to another.  I'll need to rethink things a bit.  My ideal world
would have one accessor object that, after initialization, could be used on
either the IO thread or the UI thread to avoid parallel APIs.  But this is
mainly me being OCD.

I still think making GetResourceContext() call LazyInitialize() by default
is a nicer thing to do though...

-Albert



>  However, this means that GetResourceContext() should LazyInitialize() the
>> IOData
>> structure.
>>
>> Unforutnately, AppCache, and PluginService both need to be able to take a
>> reference on the ResourceContext() w/o it initializing the IOData
>> structure.
>> Thus, I'm breaking it into 2 functions with the intention of killing off
>> GetResourceContextNoInit() after I move the AppCache construction into a
>> more
>> suitable spot.
>>
>> Don't quite have a plan for PluginService yet unfortunately.
>>
>>
http://codereview.chromium.**org/8707002/<http://codereview.chromium.org/8707...
>>
>
>

[willchan no longer on Chromium, 2011-11-29 03:08:08]

On Mon, Nov 28, 2011 at 6:46 PM, Albert J. Wong (王重傑)
<ajwong@chromium.org>wrote:

> On Mon, Nov 28, 2011 at 6:05 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> How do you intend to address the issue I pointed out earlier with the
>> ResourceContext members expecting to be initialized on the IO thread?
>>
>
>
OOPS, I thought you were invoking ResourceContext::LazyInitialize(). I see
you are just lazy initializing the Handle. Yes, this makes sense to me.


> I don't think this CL changes the effective initialization sequence at
> all...
>
> All that LazyInitialize() accomplishes is the profile_params_ structure is
> snapshotted from Profile when GetResourceContext() is called.
>  ResourceContext is still only initialized from the profile_params_ when
> someone does accesses a function inside ResourceContext.  Since there is no
> change in who is given a reference to the ResourceContext, or who is
> calling functions on those objects, there should be no change in when/where
> ResourceContext is initialized.
>
> Without this change, I think it's possible for someone to say:
>
>   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, Bind(&DoSomething,
> &Handle::GetResourceContext()));
>
> before anyone calls io_data->InitializeOnUIThread().  In this case, if
> DoSomething() uses any accessor on the passed in ResourceContext, I think
> it will get a NULL profile_params_ and segfault.  Making it so
> GetResourceContext() ensures LazyInitialize() is called avoids this API
> hole.
>
>
>
>> On Mon, Nov 28, 2011 at 6:01 PM, <ajwong@chromium.org> wrote:
>>
>>> I want to get rid of BrowserContext::**GetFileSystemContext(),
>>> BrowserContext::**GetWebKitContext(), etc. and have everyone access
>>> them through
>>> the ResourceContext structure.
>>>
>>
>> What's wrong with using BrowserContext? BrowserContext is intended to be
>> used on the UI thread. ResourceContext is for use on the IO thread.
>>
>
> Oops...this is something I completely missed.  I saw 2 parallel APIs and
> was assuming that we were just in the middle of migrating from one
> container to another.  I'll need to rethink things a bit.  My ideal world
> would have one accessor object that, after initialization, could be used on
> either the IO thread or the UI thread to avoid parallel APIs.  But this is
> mainly me being OCD.
>
> I still think making GetResourceContext() call LazyInitialize() by default
> is a nicer thing to do though...
>

Agreed.


>
> -Albert
>
>
>
>>  However, this means that GetResourceContext() should LazyInitialize()
>>> the IOData
>>> structure.
>>>
>>> Unforutnately, AppCache, and PluginService both need to be able to take a
>>> reference on the ResourceContext() w/o it initializing the IOData
>>> structure.
>>> Thus, I'm breaking it into 2 functions with the intention of killing off
>>> GetResourceContextNoInit() after I move the AppCache construction into a
>>> more
>>> suitable spot.
>>>
>>> Don't quite have a plan for PluginService yet unfortunately.
>>>
>>>
http://codereview.chromium.**org/8707002/<http://codereview.chromium.org/8707...
>>>
>>
>>
>

[willchan no longer on Chromium, 2011-11-29 03:09:23]

http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.h:47: // TODO(ajwong):
Remove GetResourceContextNoInit() once the ResourceContext
What does this mean?

[awong, 2011-11-29 03:20:21]

http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.h:47: // TODO(ajwong):
Remove GetResourceContextNoInit() once the ResourceContext
On 2011/11/29 03:09:23, willchan wrote:
> What does this mean?

This comment is now stale until I can rethink how to organize the construction
of the various bits of "storage" related data in ResourceContext.

Originally, I was going to try and break all dependencies on needing
ResourceContext before it IOData was LazyInitialize()d...but now I'm completely
not sure again.

I hate this code.

[willchan no longer on Chromium, 2011-11-29 18:36:08]

What does this mean for the status of this changelist?

http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.h (right):

http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.h:47: // TODO(ajwong):
Remove GetResourceContextNoInit() once the ResourceContext
On 2011/11/29 03:20:21, awong wrote:
> On 2011/11/29 03:09:23, willchan wrote:
> > What does this mean?
> 
> This comment is now stale until I can rethink how to organize the construction
> of the various bits of "storage" related data in ResourceContext.
> 
> Originally, I was going to try and break all dependencies on needing
> ResourceContext before it IOData was LazyInitialize()d...but now I'm
completely
> not sure again.
> 
> I hate this code.

This code is hugely important but also a complete shit show :(

[awong, 2011-11-29 19:04:24]

On 2011/11/29 18:36:08, willchan wrote:
> What does this mean for the status of this changelist?

Let me clean it up and make it non-dependent on the previous CL.

I think the NoLazyInitilaize is good to do regardless.  Will ping again when
it's ready.

> 
>
http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
> File chrome/browser/profiles/off_the_record_profile_io_data.h (right):
> 
>
http://codereview.chromium.org/8707002/diff/2001/chrome/browser/profiles/off_...
> chrome/browser/profiles/off_the_record_profile_io_data.h:47: // TODO(ajwong):
> Remove GetResourceContextNoInit() once the ResourceContext
> On 2011/11/29 03:20:21, awong wrote:
> > On 2011/11/29 03:09:23, willchan wrote:
> > > What does this mean?
> > 
> > This comment is now stale until I can rethink how to organize the
construction
> > of the various bits of "storage" related data in ResourceContext.
> > 
> > Originally, I was going to try and break all dependencies on needing
> > ResourceContext before it IOData was LazyInitialize()d...but now I'm
> completely
> > not sure again.
> > 
> > I hate this code.
> 
> This code is hugely important but also a complete shit show :(

[awong, 2011-11-30 21:53:00]

okay, ready for review.

[awong, 2011-12-21 19:53:20]

Hey Will, any chance you could get to this before going on leave?  Or should I
find another reviewer?

[willchan no longer on Chromium, 2011-12-21 19:54:04]

On 2011/12/21 19:53:20, awong wrote:
> Hey Will, any chance you could get to this before going on leave?  Or should I
> find another reviewer?

Sorry, I completely forgot about this. I'll try to take a look shortly.

[willchan no longer on Chromium, 2011-12-21 19:59:22]

LGTM

I wish there were some way to get the ordering dependencies done correctly so
it's not so mysterious why we need to skip LazyInitialize() in some cases but
not others. Hopefully future refactors will clean that up.

[awong, 2011-12-21 20:03:36]

On Wed, Dec 21, 2011 at 11:59 AM, <willchan@chromium.org> wrote:

> LGTM
>

Thanks!

I wish there were some way to get the ordering dependencies done correctly
> so
> it's not so mysterious why we need to skip LazyInitialize() in some cases
> but
> not others. Hopefully future refactors will clean that up.
>

Yes...we'll see how things go.  Charlie and I trying to scope how big of a
refactor is needed to support Isolated Apps.  If we end up touching
PluginService, breaking the early reference on ResourceContext will be very
high on my hit list.

-Albert

[commit-bot: I haz the power, 2012-01-11 02:18:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/8707002/19001

[commit-bot: I haz the power, 2012-01-11 03:47:57]

Try job failure for 8707002-19001 (retry) (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...