Fix appcache_service and request_context referencing.

Fix appcache_service and request_context referencing.

There is one appcache service per profile and several request context per profile. The profile holds a reference to the appcache service. Those request contexts which are subject to retrieval from appcaches hold a reference to the appcache service too. The appcache service is provided with a pointer back to the 'main' request context, this context is used when updating appcaches.

Initialization is a little tricky because profiles can't be used on the IO thread and request contexts can't be used on the UI thread.

BUG=22597, 22125
TEST=many existing tests exercise profile/context creation

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

[michaeln, 2009-09-21 01:28:21]

http://codereview.chromium.org/215024/diff/4012/7007
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/215024/diff/4012/7007#newcode52
Line 52: request_context_ = context;
oh... i need to null init this in the .cc file too... i'll do that prior to
committing

http://codereview.chromium.org/215024/diff/4012/7013
File webkit/tools/test_shell/test_shell.cc (right):

http://codereview.chromium.org/215024/diff/4012/7013#newcode425
Line 425: web_prefs_->application_cache_enabled = true;
i don't intend to commit this just yet... i'll put this in a separate CL for
submission in isolation... its here right now for me to run things thru the
trybots with it enabled

[michaeln, 2009-09-21 20:35:05]

new snapshot...

* null initted the data member mentioned before
* removed setting application_cache_enabled to true for test_shell as mentioned
before

[jorlow, 2009-09-21 22:31:01]

I don't understand everything here super well, so it might be best if Darin
still did the final review, but there are a couple things that really scare me.

Overall, I think it'd be a good thing if you could make the service's lifetime
deterministic.  Probably a subset of the profile's existence.  Maybe the users
of the service can ask to be notified when it's going away?

http://codereview.chromium.org/215024/diff/4012/7007
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/215024/diff/4012/7007#newcode52
Line 52: request_context_ = context;
On 2009/09/21 01:28:21, michaeln wrote:
> oh... i need to null init this in the .cc file too... i'll do that prior to
> committing

You should have the tests run with the flag enabled always.  This is done for a
lot of other experimental features that are off by default.

http://codereview.chromium.org/215024/diff/16/21
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/215024/diff/16/21#newcode116
Line 116: ChromeURLRequestContext* context = new ChromeURLRequestContext(
Why not do it in the constructor?

http://codereview.chromium.org/215024/diff/16/21#newcode120
Line 120: appcache_service->set_request_context(context);
What if there already is another set?

http://codereview.chromium.org/215024/diff/16/21#newcode557
Line 557: appcache_service_->set_request_context(NULL);
When does this happen?

http://codereview.chromium.org/215024/diff/16/20
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/215024/diff/16/20#newcode140
Line 140: if (io_thread)
!io_thread

But wait...is this really non-deterministic?  Can you come up with a way to make
it deterministic?

http://codereview.chromium.org/215024/diff/16/20#newcode208
Line 208: appcache_service_->AddRef();
Ewww...document where this thing's pair is at least.

http://codereview.chromium.org/215024/diff/16/20#newcode213
Line 213: 
extra new line

http://codereview.chromium.org/215024/diff/16/20#newcode588
Line 588: appcache_service_->AddRef();
Document where the matching Release is.

http://codereview.chromium.org/215024/diff/16/22
File chrome/common/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/215024/diff/16/22#newcode34
Line 34: bool is_initialized() { return is_initialized_; }
() const {

http://codereview.chromium.org/215024/diff/16/22#newcode38
Line 38: DCHECK(!is_initialized_);
Add a DCHECK that we're on the UI thread too?

http://codereview.chromium.org/215024/diff/16/22#newcode39
Line 39: 
extra newline seems unnecessary.

http://codereview.chromium.org/215024/diff/16/22#newcode63
Line 63: Initialize(is_incognito ? FilePath()
Why do this now?

The UI thread knows this because it has the profile...right?

http://codereview.chromium.org/215024/diff/16/17
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/215024/diff/16/17#newcode50
Line 50: URLRequestContext* request_context() { return request_context_; }
this function is const, no?

[michaeln, 2009-09-21 23:32:00]

new spanshot uploaded

http://codereview.chromium.org/215024/diff/16/21
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/215024/diff/16/21#newcode116
Line 116: ChromeURLRequestContext* context = new ChromeURLRequestContext(
> Why not do it in the constructor?

Do what in whose ctor?

Not all request contexts are equal. The 'original' context for a profile is used
for cache updates, the others (media,extensions) are not. These different kinds
of contexts are not represented as different context classes.

http://codereview.chromium.org/215024/diff/16/21#newcode120
Line 120: appcache_service->set_request_context(context);
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> What if there already is another set?

Added DCHECK(!appcache_service->request_context());

http://codereview.chromium.org/215024/diff/16/21#newcode557
Line 557: appcache_service_->set_request_context(NULL);
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> When does this happen?

When the profile and all it's original request context are no longer needed by
the browser. We're in ~ChromeURLRequestContext.

This doesn't happen until there are no
tabs/windows/ResourceMessageFilters/AppCacheDispatcherHosts/etc that can
possibly refer to anything in the profile around anymore.

http://codereview.chromium.org/215024/diff/16/20
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/215024/diff/16/20#newcode140
Line 140: if (io_thread)
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> !io_thread
> 
> But wait...is this really non-deterministic?  Can you come up with a way to
make
> it deterministic?

In response to '!io_thread'... I think the test is correct... io_thread is used
in the condition clause.

The profile unit test run 'out of context' of the browser, so this can be NULL
in test cases.

Suggestions welcome.

http://codereview.chromium.org/215024/diff/16/20#newcode208
Line 208: appcache_service_->AddRef();
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> Ewww...document where this thing's pair is at least.

added comment

See how request_context_'s are treated... following suit here... they have the
same 'created on UI thread, only used and finally released on the IO thread'
semantics... when in Rome.

http://codereview.chromium.org/215024/diff/16/20#newcode213
Line 213: 
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> extra new line

Done.

http://codereview.chromium.org/215024/diff/16/20#newcode588
Line 588: appcache_service_->AddRef();
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> Document where the matching Release is.

added comment

http://codereview.chromium.org/215024/diff/16/22
File chrome/common/appcache/chrome_appcache_service.h (right):

http://codereview.chromium.org/215024/diff/16/22#newcode34
Line 34: bool is_initialized() { return is_initialized_; }
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> () const {

Done.

http://codereview.chromium.org/215024/diff/16/22#newcode38
Line 38: DCHECK(!is_initialized_);
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> Add a DCHECK that we're on the UI thread too?

Can't... i had it that way, but it breaks the profile unit tests.

http://codereview.chromium.org/215024/diff/16/22#newcode39
Line 39: 
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> extra newline seems unnecessary.

Done.

http://codereview.chromium.org/215024/diff/16/22#newcode63
Line 63: Initialize(is_incognito ? FilePath()
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> Why do this now?
> 
> The UI thread knows this because it has the profile...right?

We want to Initialize() the base class on the IO thread.

http://codereview.chromium.org/215024/diff/16/17
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/215024/diff/16/17#newcode50
Line 50: URLRequestContext* request_context() { return request_context_; }
On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> this function is const, no?

Done.

[michaeln, 2009-09-21 23:35:05]

About the 'enable' flag... yes i want to turn that asap, but i want to do that
in a CL in isolation so if anything goes awry just that can be rolled back.
There are many resource loading use cases, i'm proceeding with caution around
turning the appcache stuff on in that pipeline.

[michaeln, 2009-09-21 23:49:25]

http://codereview.chromium.org/215024/diff/3020/2014
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/215024/diff/3020/2014#newcode213
Line 213: 
i've removed this blank line in my local copy

[michaeln, 2009-09-22 00:41:03]

Responding to this specifically...

> Overall, I think it'd be a good thing if you could make the service's lifetime
> deterministic.  Probably a subset of the profile's existence.

The lifetime is bound by the lifetime of the profile and its handful of request
contexts. If and when we need more apparatus around 'shutdown', i think the
place to add that would be in profile.cc CleanupAppCacheService. In addition to
scheduling a call to Release() on the io thread, that method could schedule a
call to Shutdown() on the io thread (then Release). But for now... there is no
need for any thing more heavy weight than Release/delete at this time.

Also, the FUD about !io_thread, should only occur in certain test case. I would
love to get rid of that, but don't see how?

[jorlow, 2009-09-22 01:03:58]

My responses to your comments.  Have not looked at the new code yet:

On 2009/09/22 00:41:03, michaeln wrote:
> Responding to this specifically...
> 
> > Overall, I think it'd be a good thing if you could make the service's
lifetime
> > deterministic.  Probably a subset of the profile's existence.
> 
> The lifetime is bound by the lifetime of the profile and its handful of
request
> contexts. If and when we need more apparatus around 'shutdown', i think the
> place to add that would be in profile.cc CleanupAppCacheService. In addition
to
> scheduling a call to Release() on the io thread, that method could schedule a
> call to Shutdown() on the io thread (then Release). But for now... there is no
> need for any thing more heavy weight than Release/delete at this time.

I think you're taking manual resource tracking more lightly than you should. 
It's a very subtle issue.  That said, I see why you're doing it and I guess it's
not much better to use other techniques.

http://codereview.chromium.org/215024/diff/16/21
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/215024/diff/16/21#newcode116
Line 116: ChromeURLRequestContext* context = new ChromeURLRequestContext(
On 2009/09/21 23:32:00, michaeln wrote:
> > Why not do it in the constructor?
> 
> Do what in whose ctor?
> 
> Not all request contexts are equal. The 'original' context for a profile is
used
> for cache updates, the others (media,extensions) are not. These different
kinds
> of contexts are not represented as different context classes.

I meant do it in the initialization section of the constructor.  (2 lines
above).

http://codereview.chromium.org/215024/diff/16/21#newcode557
Line 557: appcache_service_->set_request_context(NULL);
On 2009/09/21 23:32:00, michaeln wrote:
> On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> > When does this happen?
> 
> When the profile and all it's original request context are no longer needed by
> the browser. We're in ~ChromeURLRequestContext.
> 
> This doesn't happen until there are no
> tabs/windows/ResourceMessageFilters/AppCacheDispatcherHosts/etc that can
> possibly refer to anything in the profile around anymore.

So, in other words, the request_context should either be NULL or this?  Maybe we
can make this a null check and then inside DCHECK that it equals this?  Maybe
I'm just being overly paranoid?

http://codereview.chromium.org/215024/diff/16/20
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/215024/diff/16/20#newcode140
Line 140: if (io_thread)
On 2009/09/21 23:32:00, michaeln wrote:
> On 2009/09/21 22:31:01, Jeremy Orlow wrote:
> > !io_thread
> > 
> > But wait...is this really non-deterministic?  Can you come up with a way to
> make
> > it deterministic?
> 
> In response to '!io_thread'... I think the test is correct... io_thread is
used
> in the condition clause.
> 
> The profile unit test run 'out of context' of the browser, so this can be NULL
> in test cases.
> 
> Suggestions welcome.

Ohhh...I see.  I'd just add a comment like:   // Release on the IO thread if
it's still around.  Otherwise it's safe to release it here.

[michaeln, 2009-09-22 01:15:26]

I've poked at the io_thread() usage a little to more closely match other usages
in the profile class. Please look at the other usages when considering the new
usages.

Speifically, now calling g_browser_process.io_thread() instead of simply testing
for its presence already. In chrome proper, this will cause it to be started if
it hasn't already been started. In the test cases, NULL will be returned.

http://codereview.chromium.org/215024/diff/16/21
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/215024/diff/16/21#newcode116
Line 116: ChromeURLRequestContext* context = new ChromeURLRequestContext(
We're in a static method, not a ctor.

http://codereview.chromium.org/215024/diff/16/21#newcode557
Line 557: appcache_service_->set_request_context(NULL);
> So, in other words, the request_context should either be NULL or this?

Those aren't my words :) That assertion would not be valid.

appcache_service->reqeust_context() is set to the 'original' context for the
profile (in the appropiate static Create methods).

We could be in the dtor for one of the ancillary request context, in which case,
its not NULL and its not 'this'.

Remember a profile has several request contexts, one of which is the 'original'.

[michaeln, 2009-09-22 01:32:54]

http://codereview.chromium.org/215024/diff/16/21
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/215024/diff/16/21#newcode116
Line 116: ChromeURLRequestContext* context = new ChromeURLRequestContext(
> I meant do it in the initialization section of the constructor.  (2 lines
above).

I thought i had responded to that already. The same ctor is used to create
context instance that are not the 'original' context. This should only be done
for the 'original' context.

[michaeln, 2009-09-22 01:59:12]

http://codereview.chromium.org/215024/diff/8003/8004
File webkit/appcache/appcache_service.h (right):

http://codereview.chromium.org/215024/diff/8003/8004#newcode16
Line 16: #include "net/url_request/url_request_context.h"
i've removed the #include in my local copy... class URLRequestContext is all
that's needed here

[michaeln, 2009-09-22 16:12:36]

added bug number, this CL fixes that bug

[michaeln, 2009-09-22 16:38:08]

and another (bug 22125) this may resolve (not 100% sure about the second)

[Evan Martin, 2009-09-22 17:32:53]

typo in change description: "excersise"

[michaeln, 2009-09-22 17:38:27]

On 2009/09/22 17:32:53, Evan Martin wrote:
> typo in change description: "excersise"

fixed

[darin (slow to review), 2009-09-22 18:57:32]

LGTM

It makes me very sad that we have these cases where the io_thread may be null
just to support our unit tests.  The code would be a lot cleaner if we didn't
have to support such awkward modes.  My fear is that we might take those
branches in Chrome and not just in unit tests!

[jorlow1, 2009-09-22 19:18:17]

Agreed.  Is there any way to assert we're in a unit test?

On Tue, Sep 22, 2009 at 11:57 AM, <darin@chromium.org> wrote:

> LGTM
>
> It makes me very sad that we have these cases where the io_thread may be
> null
> just to support our unit tests.  The code would be a lot cleaner if we
> didn't
> have to support such awkward modes.  My fear is that we might take those
> branches in Chrome and not just in unit tests!
>
>
> http://codereview.chromium.org/215024
>

[jorlow1, 2009-09-22 19:28:03]

Besides prefering the test-only code paths have a dcheck or something (if
it's possible), LGTM

On Tue, Sep 22, 2009 at 12:17 PM, Jeremy Orlow <jorlow@google.com> wrote:

> Agreed.  Is there any way to assert we're in a unit test?
>
>
> On Tue, Sep 22, 2009 at 11:57 AM, <darin@chromium.org> wrote:
>
>> LGTM
>>
>> It makes me very sad that we have these cases where the io_thread may be
>> null
>> just to support our unit tests.  The code would be a lot cleaner if we
>> didn't
>> have to support such awkward modes.  My fear is that we might take those
>> branches in Chrome and not just in unit tests!
>>
>>
>> http://codereview.chromium.org/215024
>>
>
>

[michaeln, 2009-09-22 19:56:57]

Is there a bug open to track code cleanup of Profile vs profile related stuff
for use on the IO thread specifically?


Some thoughts on the topic... also mention in
http://code.google.com/p/chromium/issues/detail?id=22597


What's missing seems to be a class ProfileForIO type of thing, on which the
various structures NOT 
accessible on the UI thread hang (like request contexts).

class Profile {
  void Profile() {
    ScheduleInitForIO();  // this is racey thou, InitOnIO can execute prior to
return
                          // resulting in refcounting bugs, maybe defer
scheduling
                          // the task to be run on the IO thread till SOON
  }
  void InitForIO() {
    profile_for_io_  = new ProfileForIO();
  }
  ~Profile() {
    ScheduleReleaseForIO(profile_for_io_.release());
  }
  scoped_refptr<ProfileForIO> profile_for_io_;
};

// Created empty on the UI thread, finally released on the IO thread
class ProfileForIO {
  scoped_refptr<ChromeURLRequestContext> main_request_context_;
  scoped_refptr<ChromeURLRequestContext> blah_request_context_;
  scoped_refptr<ChromeAppCacheService> appcache_service_;
};

[jorlow1, 2009-09-22 20:50:58]

From my limited knowledge of the subject, that seems like a fine solution.
 Not so sure about the name though.  :-)
// Classes owned by the IO thread, even though they're initialized here in
the UI thread.
class IOThreadOwnedData

Do the members need to be refptr's?  in some/all cases, they may not need to
be.

On Tue, Sep 22, 2009 at 12:56 PM, <michaeln@chromium.org> wrote:

> Is there a bug open to track code cleanup of Profile vs profile related
> stuff
> for use on the IO thread specifically?
>
>
> Some thoughts on the topic... also mention in
> http://code.google.com/p/chromium/issues/detail?id=22597
>
>
> What's missing seems to be a class ProfileForIO type of thing, on which the
> various structures NOT
> accessible on the UI thread hang (like request contexts).
>
> class Profile {
>  void Profile() {
>    ScheduleInitForIO();  // this is racey thou, InitOnIO can execute prior
> to
> return
>                          // resulting in refcounting bugs, maybe defer
> scheduling
>                          // the task to be run on the IO thread till SOON
>  }
>  void InitForIO() {
>    profile_for_io_  = new ProfileForIO();
>  }
>  ~Profile() {
>    ScheduleReleaseForIO(profile_for_io_.release());
>  }
>  scoped_refptr<ProfileForIO> profile_for_io_;
> };
>
> // Created empty on the UI thread, finally released on the IO thread
> class ProfileForIO {
>  scoped_refptr<ChromeURLRequestContext> main_request_context_;
>  scoped_refptr<ChromeURLRequestContext> blah_request_context_;
>  scoped_refptr<ChromeAppCacheService> appcache_service_;
>
> };
>
> http://codereview.chromium.org/215024
>

[jorlow1, 2009-09-22 20:51:28]

On Tue, Sep 22, 2009 at 1:50 PM, Jeremy Orlow <jorlow@google.com> wrote:

> From my limited knowledge of the subject, that seems like a fine solution.
>  Not so sure about the name though.  :-)
> // Classes owned by the IO thread, even though they're initialized here in
> the UI thread.
> class IOThreadOwnedData
>

Er...to clarify...that was a suggested name and type of comment you might
have above it to make it super clear why it exists.


> Do the members need to be refptr's?  in some/all cases, they may not need
> to be.
>
> On Tue, Sep 22, 2009 at 12:56 PM, <michaeln@chromium.org> wrote:
>
>> Is there a bug open to track code cleanup of Profile vs profile related
>> stuff
>> for use on the IO thread specifically?
>>
>>
>> Some thoughts on the topic... also mention in
>> http://code.google.com/p/chromium/issues/detail?id=22597
>>
>>
>> What's missing seems to be a class ProfileForIO type of thing, on which
>> the
>> various structures NOT
>> accessible on the UI thread hang (like request contexts).
>>
>> class Profile {
>>  void Profile() {
>>    ScheduleInitForIO();  // this is racey thou, InitOnIO can execute prior
>> to
>> return
>>                          // resulting in refcounting bugs, maybe defer
>> scheduling
>>                          // the task to be run on the IO thread till SOON
>>  }
>>  void InitForIO() {
>>    profile_for_io_  = new ProfileForIO();
>>  }
>>  ~Profile() {
>>    ScheduleReleaseForIO(profile_for_io_.release());
>>  }
>>  scoped_refptr<ProfileForIO> profile_for_io_;
>> };
>>
>> // Created empty on the UI thread, finally released on the IO thread
>> class ProfileForIO {
>>  scoped_refptr<ChromeURLRequestContext> main_request_context_;
>>  scoped_refptr<ChromeURLRequestContext> blah_request_context_;
>>  scoped_refptr<ChromeAppCacheService> appcache_service_;
>>
>> };
>>
>> http://codereview.chromium.org/215024
>>
>
>