Splits ChromeURLDataManager into 2 chunks:...

Splits ChromeURLDataManager into 2 chunks:
. ChromeURLDataManager is no longer a singleton and is always used on
  the UI thread. ChromeURLDataManager is now profile specific (you get
  from the profile).
. ChromeURLDataManagerBackend handles the URLRequests and the
  DataSources. ChromeURLDataManagerBackend is created by
  ChromeURLRequestContext.

All DataSources are now profile specific. There were two that wanted
to be global, but have been converted.

BUG=52022
71868
TEST=none


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

[sky, 2011-02-07 18:53:15]

Will, could you review the changes to ChromeURLRequestContext?

Evan, if you aren't comfortable reviewing the rest of the changes, is there some
one you would recommend? Start with the changes to
ChromeURLDataManager/ChromeURLDataManagerBackend, everything else is fallout
from that.

Thanks,

  -Scott

[Evan Martin, 2011-02-07 19:06:21]

I started looking but mostly found myself pretty confused.

Here are the top contributors of lines as measured by the blame:

$ git blame chrome/browser/dom_ui/chrome_url_data_manager.cc | cut -d' ' -f2 |
sort | uniq -c | sort -rn | head -5
    217 (initial.commit
     35 (ahendrickson@chromium.org
     33 (tfarina@chromium.org
     17 (erg@google.com
     16 (mark@chromium.org

I suspect those initial.commit ones are me, so if you don't find a better
reviewer, I guess it falls on me to review.  But I really feel like I don't
understand how this all works anymore...

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:93:
scoped_refptr<RefCountedMemory> bytes) {
Can you DCHECK() that you're on the IO thread as a way of documenting that's
where it lives?  The name is a bit confusing (it sounds like it's the function
that is supposed to itself post to the IO thread).

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:25: // with AddDataSource.
Can you improve this comment?  (It's probably my fault, but now even I don't
remember how all this stuff works.)  In particular now that there's a backend
object, I guess mentioning that this is the UI-loop frontend would help.

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.cc:168: base::AutoLock
lock(*instances_lock_);
It seems you have a precondition that ::Register() is called (to create the
lock) before this ctor is called (?).  Is that because Register is static? 
Maybe it needs the "// static" line above it at L174

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:29: // DataSource, it'll
handle mapping from the UI thread to the IO thread.
Maybe to make this more clear, separate the "Use ChromeURLDataManager to ..."
into a paragraph that is directed towards people interested in adding new data
sources (it might be enough to just point them to the other header), and then
something separate about where this object lives/who owns it/who should poke at
it.

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:116: static base::Lock*
instances_lock_;
There is one lock shared across all backend instances?  That seems a bit weird
to me...

[sky, 2011-02-07 19:32:06]

I've added a few more comments. Let me know if things still ar vague. I'm also
adding ahendrickson to the reviewers list as I believe he did the recent changes
that deal with deleting on the appropriate thread.

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:25: // with AddDataSource.
On 2011/02/07 19:06:21, Evan Martin wrote:
> Can you improve this comment?  (It's probably my fault, but now even I don't
> remember how all this stuff works.)  In particular now that there's a backend
> object, I guess mentioning that this is the UI-loop frontend would help.

I added a bit more, but I'm not sure if I'm getting what you want. If you don't
feel what I've written is an improvement, let met know.

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/2028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:116: static base::Lock*
instances_lock_;
On 2011/02/07 19:06:21, Evan Martin wrote:
> There is one lock shared across all backend instances?  That seems a bit weird
> to me...

This lock is used for access to instances_ and shutdown_instances_. instances_
does not pin down the instances, where as shutdown_instance_ does. This is need
to handle proper shutdown. At the time shutdown is happening the UI thread is no
longer running. The DataSources have the trait BrowserThread::DeleteOnUIThread.
This means if we let normal deletion happen all the DataSources leak
(DeleteOnUIThread effectively drops the instance if the message loop is done
running). The fix is to tell the backend shutdown is starting. This creates and
adds all existing (and newly created) instances to shutdown_instances_.
shutdown_instances_ pins down the instances so that the DataSources can't leak.
After the IO thread has been shutdown, CompleteShutdown is invoked to release
all the instances. This is from the IO thread and triggers deletion as there
should be no more references.

This description is in ChromeURLDataManagerBackend::PrepareForShtudown. I've
added:
// See comment in PrepareForShutdown for details.
to instances_ and shutdown_instances_.

[sky, 2011-02-07 19:32:45]

Will, you'll also want to look at ChromeURLDataManagerBackend's GetBackend
(static in the .cc). That's used to map from the request to the backend.

Thanks,

  -Scott

[ahendrickson, 2011-02-07 22:57:06]

LGTM regarding deleting of DataSource's, but Valgrind will tell for sure $-)

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:27: //
ChromeURLDataManagerBackend, see if for details.
Nit: 'it', not 'if'

[willchan no longer on Chromium, 2011-02-08 04:57:50]

How does this stuff work with incognito DomUI? AFAICT, the
ChromeURLDataManagerBackend objects will be destroyed on the IO thread (since
they're owned by the ChromeURLRequestContext, which will be destroyed on the IO
thread), which will release the data sources on the IO thread, which I believe
hits DCHECKs since the data sources are created on the UI thread. Is there a
test somewhere that covers the incognito DomUI case (where the
ChromeURLRequestContexts are destroyed early, as opposed to during browser
shutdown)?

I think I'd like to see the ChromeURLDataManagerBackend objects not be
refcounted if possible.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
File chrome/browser/browser_signin.cc (left):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
chrome/browser/browser_signin.cc:218: BrowserSigninResourcesSource* source = new
BrowserSigninResourcesSource();
Is this one being replaced somewhere else?

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:23: // Invoked on the IO thread
to do the actual adding of the DataSource.
I'd add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) whenever you
have a OnIOThread() function.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:42: static void
Register();
You should probably document that this has to be registered before
ChromeURLDataManager is used. Otherwise it'll just crash. Also, it needs to be
used on startup, before other Chrome threads start to try to use it.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:101: typedef
std::map<std::string,
I know it's inconvenient to put them further away from use, but typedefs should
go earlier in the section (typedefs and enums go first in each access section).
Elliot's clang plugin will be enforcing this in the future.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:133:
DISALLOW_COPY_AND_ASSIGN(ChromeURLDataManagerBackend);
You need basictypes.h for this header file.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/sync_i...
File chrome/browser/dom_ui/sync_internals_ui_unittest.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/sync_i...
chrome/browser/dom_ui/sync_internals_ui_unittest.cc:113: private:
Can these just be added to the protected section? Or can the stuff in the
protected section be moved to the private section? Looks like |mock_js_backend_|
needs to be protected, but maybe everything else can go private?

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:737:
ChromeURLRequestContext::GetChromeURLDataManagerBackend() {
If we don't need lazy instantiation, I'd prefer to just construct it normally.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:623: virtual ChromeURLDataManager*
GetChromeURLDataManager() {
Is this expensive to instantiate? Can we just create it during Profile
construction rather than use lazy instantiation?

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile_impl.cc:1376: ChromeURLDataManager*
ProfileImpl::GetChromeURLDataManager() {
If we don't need to lazily instantiate this, then I think it'd be cleaner just
to construct it in ProfileImpl's constructor.

[sky, 2011-02-08 05:35:26]

> How does this stuff work with incognito DomUI? AFAICT, the
> ChromeURLDataManagerBackend objects will be destroyed on the IO thread
> (since
> they're owned by the ChromeURLRequestContext, which will be destroyed on the
> IO
> thread), which will release the data sources on the IO thread, 

The DataSources have the DeleteOnUIThread trait, so this case should work fine.
This scenario should also come up in the multi-profile world (assuming we delete
profiles on closing last browser with profile). The only problematic scenario is
when the UI thread is no longer running and then we destroy the IO thread
(shutdown). In this case the task to delete the datasources is added to the UI
message loop, but the message loop is never run again and they leak. This is why
there is the complex code at shutdown time that avoids deleting the backends on
the IO thread.

I've updated the patch.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
File chrome/browser/browser_signin.cc (left):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
chrome/browser/browser_signin.cc:218: BrowserSigninResourcesSource* source = new
BrowserSigninResourcesSource();
On 2011/02/08 04:57:52, willchan wrote:
> Is this one being replaced somewhere else?

As far as I can tell this code is only used in tests and doesn't need the
datasource registered. I've got an email out to verify that.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.cc:23: // Invoked on the IO thread
to do the actual adding of the DataSource.
On 2011/02/08 04:57:52, willchan wrote:
> I'd add a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO)) whenever you
> have a OnIOThread() function.

Done.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager.h (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager.h:27: //
ChromeURLDataManagerBackend, see if for details.
On 2011/02/07 22:57:06, ahendrickson wrote:
> Nit: 'it', not 'if'

Done.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:42: static void
Register();
On 2011/02/08 04:57:52, willchan wrote:
> You should probably document that this has to be registered before
> ChromeURLDataManager is used. Otherwise it'll just crash. Also, it needs to be
> used on startup, before other Chrome threads start to try to use it.

Done.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:101: typedef
std::map<std::string,
On 2011/02/08 04:57:52, willchan wrote:
> I know it's inconvenient to put them further away from use, but typedefs
should
> go earlier in the section (typedefs and enums go first in each access
section).
> Elliot's clang plugin will be enforcing this in the future.

I didn't realize that was part of the style guide.
Done.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/chrome...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:133:
DISALLOW_COPY_AND_ASSIGN(ChromeURLDataManagerBackend);
On 2011/02/08 04:57:52, willchan wrote:
> You need basictypes.h for this header file.

Done.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/sync_i...
File chrome/browser/dom_ui/sync_internals_ui_unittest.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/dom_ui/sync_i...
chrome/browser/dom_ui/sync_internals_ui_unittest.cc:113: private:
On 2011/02/08 04:57:52, willchan wrote:
> Can these just be added to the protected section? Or can the stuff in the
> protected section be moved to the private section? Looks like
|mock_js_backend_|
> needs to be protected, but maybe everything else can go private?

Moved to protected.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/net/chrome_ur...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/net/chrome_ur...
chrome/browser/net/chrome_url_request_context.cc:737:
ChromeURLRequestContext::GetChromeURLDataManagerBackend() {
On 2011/02/08 04:57:52, willchan wrote:
> If we don't need lazy instantiation, I'd prefer to just construct it normally.

I went with lazy instantiation as otherwise we unnecessarily create the backend
for the media/extensions contexts.

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.cc (right):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.cc:623: virtual ChromeURLDataManager*
GetChromeURLDataManager() {
On 2011/02/08 04:57:52, willchan wrote:
> Is this expensive to instantiate? Can we just create it during Profile
> construction rather than use lazy instantiation?

I don't think it's particularly expensive, but I don't think we should be
creating everything at construction time we might need. If we take that approach
with everything startup would be impacted. Additionally lazy instantiation is a
pretty common pattern in this class.

[sky, 2011-02-08 17:13:46]

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
File chrome/browser/browser_signin.cc (left):

http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
chrome/browser/browser_signin.cc:218: BrowserSigninResourcesSource* source = new
BrowserSigninResourcesSource();
On 2011/02/08 05:35:26, sky wrote:
> On 2011/02/08 04:57:52, willchan wrote:
> > Is this one being replaced somewhere else?
> 
> As far as I can tell this code is only used in tests and doesn't need the
> datasource registered. I've got an email out to verify that.

I think I'm wrong here. Let me fix this.

[sky, 2011-02-08 17:48:24]

On 2011/02/08 17:13:46, sky wrote:
>
http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
> File chrome/browser/browser_signin.cc (left):
> 
>
http://codereview.chromium.org/6286131/diff/5028/chrome/browser/browser_signi...
> chrome/browser/browser_signin.cc:218: BrowserSigninResourcesSource* source =
new
> BrowserSigninResourcesSource();
> On 2011/02/08 05:35:26, sky wrote:
> > On 2011/02/08 04:57:52, willchan wrote:
> > > Is this one being replaced somewhere else?
> > 
> > As far as I can tell this code is only used in tests and doesn't need the
> > datasource registered. I've got an email out to verify that.
> 
> I think I'm wrong here. Let me fix this.

It should be fixed now. New snapshot uploaded.

  -Scott

[willchan no longer on Chromium, 2011-02-08 18:24:45]

http://codereview.chromium.org/6286131/diff/14004/chrome/browser/dom_ui/chrom...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.cc (right):

http://codereview.chromium.org/6286131/diff/14004/chrome/browser/dom_ui/chrom...
chrome/browser/dom_ui/chrome_url_data_manager_backend.cc:151: base::Lock*
ChromeURLDataManagerBackend::instances_lock_ = NULL;
I think all this complexity with the instances_lock_ stuff isn't worth it. Let's
just leak.

* This is complicated and can probably lead to bugs in the future.
* It's not fully correct. When PrepareForShutdown() is called, the UI message
loop has *already* stopped. That means that if a backend has been destroyed
after the UI message loop has stopped but before PrepareForShutdown() is called,
we're leaking those DataSources.
* CompleteShutdown() will run after all other threads have shut down. I don't
think we've vetted all the DataSource destructors to make sure they are safe to
run after the other threads have stopped. This sounds dangerous. Probably their
tasks are just getting deleted, because they are most likely using
BrowserThread, which will delete the posted task if the target thread doesn't
exist anymore.

I think the only real answer is to redo the whole lifetime issues. Otherwise,
all this complexity is only for a partial solution that might cause issues of
its own. Let's just suppress whatever leaks come up.

[sky, 2011-02-08 22:01:32]

I've simplified the deletion since last patch. A custom trait is now used that
if we're in the process of shutting down adds the datasource to a vector
otherwise schedules deletion on the UI thread.

  -Scott

[willchan no longer on Chromium, 2011-02-08 23:07:36]

http://codereview.chromium.org/6286131/diff/66066/chrome/browser/dom_ui/chrom...
File chrome/browser/dom_ui/chrome_url_data_manager.cc (right):

http://codereview.chromium.org/6286131/diff/66066/chrome/browser/dom_ui/chrom...
chrome/browser/dom_ui/chrome_url_data_manager.cc:62: void
ChromeURLDataManager::PrepareForShutdown() {
There's still a race here due to PrepareForShutdown() being called after the UI
message loop has already stopped. I recommend the following:

* Have DeleteDataSource() always append to data_sources (can you rename to
g_data_sources to imply it's a global? this convention is used in some places in
chrome and i think it's a good one.). Then have it post a task to the UI thread
to flush g_data_sources.  If we have multiple data sources getting deleted, then
we may have multiple tasks, and only the first will do anything, with the rest
possibly being no-ops, but that should be fine.

*  In shutdown, this task won't get run. The solution is to have
CompleteShutdown() flush g_data_sources.

Btw, DeleteDataSource() does not seem to account for the last reference already
being released on the UI thread. I believe the DeleteOnUIThread trait does not
re-post to the UI thread if we're already on the UI thread. You probably can do
the same optimization.

http://codereview.chromium.org/6286131/diff/66066/chrome/browser/dom_ui/chrom...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/66066/chrome/browser/dom_ui/chrom...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:29: class
ChromeURLDataManagerBackend
Now that we're not delaying ChromeURLDataManagerBackend destruction to the UI
thread, can we stop refcounting this? Can you change ChromeURLRequestContext to
just use a scoped_ptr?

[sky, 2011-02-09 00:09:34]

Updated.

  -Scott

[willchan no longer on Chromium, 2011-02-09 00:35:53]

LGTM, thanks for the cleanup! Should wait for Evan's review too though. I'll
tell him what I covered.

http://codereview.chromium.org/6286131/diff/37018/chrome/browser/dom_ui/chrom...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/37018/chrome/browser/dom_ui/chrom...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:56: friend class
base::RefCountedThreadSafe<ChromeURLDataManagerBackend>;
Not needed anymore.

[Evan Martin, 2011-02-09 00:50:01]

LGTM

http://codereview.chromium.org/6286131/diff/37018/chrome/browser/dom_ui/chrom...
File chrome/browser/dom_ui/chrome_url_data_manager_backend.h (right):

http://codereview.chromium.org/6286131/diff/37018/chrome/browser/dom_ui/chrom...
chrome/browser/dom_ui/chrome_url_data_manager_backend.h:38: static void
Register();
Consider a longer name (RegisterProtocolFactories or RegisterURLScheme or
something)

http://codereview.chromium.org/6286131/diff/37018/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/6286131/diff/37018/chrome/browser/profiles/pro...
chrome/browser/profiles/profile.h:500: // Returns the ChromeURLDataManager.
Maybe:
"Returns the ChromeURLDataManager for this profile"