Change BrowserThreadDelegate to run Init() async.

Change BrowserThreadDelegate to run Init() async.

After this change Init() method will be run as the first task
on the thread asynchronously from the UI thread initialization.
This allows us to speed up the startup by making IOThread Init()
a non-blocking task for the UI.

Changes AwUrlRequestContextGetter to create a CookieMonster on
UI thread since it needs to happen synchonously.

BUG=258231
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212003

[Maria, 2013-07-11 20:13:47]

This is re-worked version of the change started in
https://codereview.chromium.org/18405007/ and based on the comments there. I
found it easier to split into a different code review.

[awong, 2013-07-11 20:43:48]

Thanks for continuing with the changes Maria, and for working through the
widened scope of the CL.

I have a few more comments.

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/aw_br...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/aw_br...
android_webview/browser/aw_browser_context.cc:90:
url_request_context_getter_->InitializeOnUiThread();
I think we have to move this into PreMainMessageLoopRun for it to work. 
InitializeOnUiThread() calls CreateCookieStore() which calls
BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), and
BrowserThread::GetBlockingPool()->GetSequencedTaskRunner().

I don't think these exist until after thread creation.

AFAICT there are no other delegates in AwBrowserContext which I think means that
all the message loops must be empty in PreMainMessageLoopRun.

If that logic sounds right, then I think it'd be cleaner to 
  (a) Make AwURLRequestContextGetter NOT a BrowserThreadDelegate
  (b) Move your AwURLRequestContextGetter::InitializeOnIiThread() to 
      CookieStore* AwBrowserContext::CreateCookieStore()
  (c) First thing in PreMainMesasgeLoopRun, do
      scoped_refptr<CookieStore> cookie_store = CreateCookieStore();
      url_request_context_getter_ = new AwURLRequestContextGetter(this);
      BrowserThread::PostTask(IO, FROM_HERE,
          Bind(&AwURLRequestContextGetter::InitOnIOThread, 
               url_request_context_getter_,
               cookie_store));

The upside of not being a BrowserThreadDelegate is that the threading
constraints are better understood.

Please push back on me if you think I'm missing something. This code is tricky
enough that I'm only 80% confident in my analysis.

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:66: // thread
trying to access the CookieManager API).
This comment is slightly out of date. We're already on the main thread so I
would just say that the goal is to publish to the CookieManager API before any
tasks start getting posted to threads.

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.h:36: // must be
called prior to Init() and contain the minimum amount of code
Please use correct capitalization and punctuation in comments. 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...

Also, I think the prevailing style is to say InitializeOnUIThread(). Yeah, it
breaks from style guide recommendations but it's also more consistent.

[jam, 2013-07-11 20:57:13]

https://codereview.chromium.org/18618004/diff/1/content/browser/browser_threa...
File content/browser/browser_thread_impl.cc (right):

https://codereview.chromium.org/18618004/diff/1/content/browser/browser_threa...
content/browser/browser_thread_impl.cc:109:
base::Bind(&BrowserThreadDelegate::Init, base::Unretained(delegate)));
what guarantee do we have now that BrowserThreadDelegate will be alive? i.e. it
could be deleted right after this method returns. perhaps we want to call this
out in the comment

[Maria, 2013-07-11 21:11:36]

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/aw_br...
File android_webview/browser/aw_browser_context.cc (right):

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/aw_br...
android_webview/browser/aw_browser_context.cc:90:
url_request_context_getter_->InitializeOnUiThread();
I think you are correct that I cannot be creating the cookie store until we have
a message loop for IO thread. However, I don't like the suggestion fix below
because it relies on the current knowledge of there not being a
BrowserThreadDelegate -- if someone was to add it in the future, they would
break our assumptions of an empty message loop.

Instead, what do you think about changing BrowserThreadDelegate interface to
have three methods:

Init()
InitAsync()
CleanUp

Then we will have a delegate->Init() call followed by delegate->InitAsync() call
scheduled as the first call on the loop.

So we would rename IOThread::Init into InitAsync and InitializeOnUiThread would
become Init() method.


On 2013/07/11 20:43:48, awong wrote:
> I think we have to move this into PreMainMessageLoopRun for it to work. 
> InitializeOnUiThread() calls CreateCookieStore() which calls
> BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), and
> BrowserThread::GetBlockingPool()->GetSequencedTaskRunner().
> 
> I don't think these exist until after thread creation.
> 
> AFAICT there are no other delegates in AwBrowserContext which I think means
that
> all the message loops must be empty in PreMainMessageLoopRun.
> 
> If that logic sounds right, then I think it'd be cleaner to 
>   (a) Make AwURLRequestContextGetter NOT a BrowserThreadDelegate
>   (b) Move your AwURLRequestContextGetter::InitializeOnIiThread() to 
>       CookieStore* AwBrowserContext::CreateCookieStore()
>   (c) First thing in PreMainMesasgeLoopRun, do
>       scoped_refptr<CookieStore> cookie_store = CreateCookieStore();
>       url_request_context_getter_ = new AwURLRequestContextGetter(this);
>       BrowserThread::PostTask(IO, FROM_HERE,
>           Bind(&AwURLRequestContextGetter::InitOnIOThread, 
>                url_request_context_getter_,
>                cookie_store));
> 
> The upside of not being a BrowserThreadDelegate is that the threading
> constraints are better understood.
> 
> Please push back on me if you think I'm missing something. This code is tricky
> enough that I'm only 80% confident in my analysis.

[awong, 2013-07-11 21:33:05]

On 2013/07/11 21:11:36, Maria wrote:
>
https://codereview.chromium.org/18618004/diff/1/android_webview/browser/aw_br...
> File android_webview/browser/aw_browser_context.cc (right):
> 
>
https://codereview.chromium.org/18618004/diff/1/android_webview/browser/aw_br...
> android_webview/browser/aw_browser_context.cc:90:
> url_request_context_getter_->InitializeOnUiThread();
> I think you are correct that I cannot be creating the cookie store until we
have
> a message loop for IO thread. However, I don't like the suggestion fix below
> because it relies on the current knowledge of there not being a
> BrowserThreadDelegate -- if someone was to add it in the future, they would
> break our assumptions of an empty message loop.
> 
> Instead, what do you think about changing BrowserThreadDelegate interface to
> have three methods:
> 
> Init()
> InitAsync()
> CleanUp
> 
> Then we will have a delegate->Init() call followed by delegate->InitAsync()
call
> scheduled as the first call on the loop.
> 
> So we would rename IOThread::Init into InitAsync and InitializeOnUiThread
would
> become Init() method.

I see your point, but I don't think we can actually do better. Assume we don't
change the code at all and someone adds a new delegate in the future (say to the
Cache thread) that uses the CookieApi. We'd still be broken.

The synchronization guarantees needed by Android WebView seem external to the
API design.  If we want to guard against it, it'd probably be better to sprinkle
DCHECKs all over chromium/src/android_webview/native/cookie_manager.cc to die if
SetCookieMonsterOnNetworkStackInit() hasn't been called.

As is I'm not sure I see what Android WebView is gaining from being a
BrowserThreadDelegate.


> On 2013/07/11 20:43:48, awong wrote:
> > I think we have to move this into PreMainMessageLoopRun for it to work. 
> > InitializeOnUiThread() calls CreateCookieStore() which calls
> > BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO), and
> > BrowserThread::GetBlockingPool()->GetSequencedTaskRunner().
> > 
> > I don't think these exist until after thread creation.
> > 
> > AFAICT there are no other delegates in AwBrowserContext which I think means
> that
> > all the message loops must be empty in PreMainMessageLoopRun.
> > 
> > If that logic sounds right, then I think it'd be cleaner to 
> >   (a) Make AwURLRequestContextGetter NOT a BrowserThreadDelegate
> >   (b) Move your AwURLRequestContextGetter::InitializeOnIiThread() to 
> >       CookieStore* AwBrowserContext::CreateCookieStore()
> >   (c) First thing in PreMainMesasgeLoopRun, do
> >       scoped_refptr<CookieStore> cookie_store = CreateCookieStore();
> >       url_request_context_getter_ = new AwURLRequestContextGetter(this);
> >       BrowserThread::PostTask(IO, FROM_HERE,
> >           Bind(&AwURLRequestContextGetter::InitOnIOThread, 
> >                url_request_context_getter_,
> >                cookie_store));
> > 
> > The upside of not being a BrowserThreadDelegate is that the threading
> > constraints are better understood.
> > 
> > Please push back on me if you think I'm missing something. This code is
tricky
> > enough that I'm only 80% confident in my analysis.

[mkosiba (inactive), 2013-07-12 12:36:00]

CC Kristian who knows a bit more about our use of the cookie store.

[Maria, 2013-07-12 17:53:11]

Please take another look at this CL.

Updated this change to add a new InitAsync() method to the delegate interface,
so the implementers have the option of initializing synchronously or
asynchronously and modified the two subclasses to make use of the new interface.

For the record, I tried the approach of getting rid of BrowserThreadDelegate
interface entirely and adding a hook to run right after all threads are created,
however this approach doesn't work for IOThread::CleanUp which needs to run as
the last thing after the thread stops processing messages and there's no good
place to hook it in from the main thread.

I think this is the most reasonable working solution I have.  There is a
suggestion that AwRequestContextGetter should not be a BrowserThreadDelegate at
all, but I would like to leave that as out of scope for this particular change
since changing that requires quite a bit of surgery.

[jam, 2013-07-16 17:52:34]

lgtm

[Kristian Monsen, 2013-07-16 20:04:09]

https://codereview.chromium.org/18618004/diff/29001/android_webview/browser/n...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/18618004/diff/29001/android_webview/browser/n...
android_webview/browser/net/aw_url_request_context_getter.h:62:
scoped_refptr<net::CookieStore> cookie_store_;
It would be nice to be able to avoid creating this, but I didn't see an obvious
way to do that.

If that is not possible, maybe a comment saying how this variable is used, and
that it is expected to be null after init?

[joth, 2013-07-16 20:19:52]

lgtm

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/aw_url_request_context_getter.cc (right):

https://codereview.chromium.org/18618004/diff/1/android_webview/browser/net/a...
android_webview/browser/net/aw_url_request_context_getter.cc:103:
url_request_context_->set_cookie_store(cookie_store_.get());
url_request_context_->set_cookie_store(cookie_store_.release())

https://codereview.chromium.org/18618004/diff/29001/android_webview/browser/n...
File android_webview/browser/net/aw_url_request_context_getter.h (right):

https://codereview.chromium.org/18618004/diff/29001/android_webview/browser/n...
android_webview/browser/net/aw_url_request_context_getter.h:62:
scoped_refptr<net::CookieStore> cookie_store_;
On 2013/07/16 20:04:09, Kristian Monsen wrote:
> It would be nice to be able to avoid creating this, but I didn't see an
obvious
> way to do that.
> 
> If that is not possible, maybe a comment saying how this variable is used, and
> that it is expected to be null after init?

agree to that -- comment would be good.

Or alternatively, just don't NULL it out in InitAsync? The cookie store needs to
live as long as the URL context getter, and it's refcounted, so we might as well
hold onto it here for the duration.

This will also make it easier to tidy up the init flow further in future --
rather than the random call out to DidCreateCookieMonster() we'd just add an
explicit call to
GetDefaultBrowserContext()->GetURLContextGetter()->GetCookieStore() when it is
needed.

[awong, 2013-07-16 21:21:38]

lgtm w/ nit

agreed with others about commenting on the cookie store ownership handoff.

https://codereview.chromium.org/18618004/diff/29001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/18618004/diff/29001/chrome/browser/io_thread....
chrome/browser/io_thread.h:212: virtual void Init() OVERRIDE {}
nit: don't inline virtual functions.

[Maria, 2013-07-16 21:53:03]

Updated based on comments. I chose to go with joth's suggestion and just keep
the cookie store around instead of nulling it out.

[joth, 2013-07-16 23:36:40]

aw/ lgtm

[commit-bot: I haz the power, 2013-07-17 00:54:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mariakhomenko@chromium.org/18618004/57001

[commit-bot: I haz the power, 2013-07-17 08:58:45]

Change committed as 212003