Add support for multiple asynchronous browser startups.

Add support for multiple asynchronous browser startups.

This CL adds support for easily doing multiple asynchronous startups
where every startup request can get their own callback executed. This
makes the logic in at the caller very simple by for example extracting
away usage of AndroidBrowserProcess and catching ProcessInitException.

BrowserStartupConfig is renamed to BrowserStartupController, since it
now does more than just keep the configuration. The
BrowserStartupController is called from native when the native startup
is complete. It keeps an ordered list of caller's callbacks which will
all be called when initialization is finished. If initialization has
already been completed before the async startup request is called,
the callback is called immediately.

The first users of this are content shell and chromium testshell.

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

[nyquist, 2013-08-14 04:54:01]

bulach: PTAL

The first user of this is Chromium Testshell. We will need this multiple async
startup functionality very soon when the SyncAdapter is upstreamed.

[bulach, 2013-08-14 16:05:47]

+mariakhomenkom, aberent and haaawk FYI, I'll start reading this now.. thanks!

[bulach, 2013-08-14 17:02:00]

thanks tommy! anthony was looking into a somewhat similar issue:
https://codereview.chromium.org/22691002/

rather than having another level of controller, perhaps we should augment
BrowserStartupConfig.java itself? having two APIs for starting the browser
process will surely be confusing, wdyt?

https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:80:
callback.run(-1);
should we post this as a message in order to yield the callstack rather than
call it right here?
the caller may not be expected for this to be called synchronously..

https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:96:
// callbacks. We use |this| as the single callback for this.
if you remove this, this would be clearer :)
We use |this| as a listener for BrowserStartupConfig callback.

[nyquist, 2013-08-15 07:07:03]

bulach: PTAL. Addressed comments.

Basically merged BrowserStartupConfig and BrowserStartupController. Also had to
change the content shell activity to keep the old functionality, by using two
different callbacks based on whether the dump render tree flag was set or not.

Also rebased.

https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:80:
callback.run(-1);
On 2013/08/14 17:02:00, bulach wrote:
> should we post this as a message in order to yield the callstack rather than
> call it right here?
> the caller may not be expected for this to be called synchronously..

Done.

https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:96:
// callbacks. We use |this| as the single callback for this.
On 2013/08/14 17:02:00, bulach wrote:
> if you remove this, this would be clearer :)
> We use |this| as a listener for BrowserStartupConfig callback.

Done.

[haaawk, 2013-08-15 09:22:48]

On 2013/08/15 07:07:03, nyquist wrote:
> bulach: PTAL. Addressed comments.
> 
> Basically merged BrowserStartupConfig and BrowserStartupController. Also had
to
> change the content shell activity to keep the old functionality, by using two
> different callbacks based on whether the dump render tree flag was set or not.
> 
> Also rebased.
> 
>
https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
> File
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
> (right):
> 
>
https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:80:
> callback.run(-1);
> On 2013/08/14 17:02:00, bulach wrote:
> > should we post this as a message in order to yield the callstack rather than
> > call it right here?
> > the caller may not be expected for this to be called synchronously..
> 
> Done.
> 
>
https://codereview.chromium.org/22272006/diff/6001/content/public/android/jav...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:96:
> // callbacks. We use |this| as the single callback for this.
> On 2013/08/14 17:02:00, bulach wrote:
> > if you remove this, this would be clearer :)
> > We use |this| as a listener for BrowserStartupConfig callback.
> 
> Done.

Hi Tommy,

Could you add a method to allow StartupCallback registration without starting
the browser proccess? This is needed by Icing integration as it needs to know
when shared library is loaded and initialized but it doesn't want to trigger
library loading.

Thanks,
Piotr

[bulach, 2013-08-15 15:09:12]

+peter for the DRT bits..

thanks Tommy! the "Controller" makes sense.. some nits and suggestions, below. I
think having the method as "void" and only (and always) communicating via the
callback makes more sense, wdyt?

https://codereview.chromium.org/22272006/diff/19001/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/22272006/diff/19001/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:73:
if (!BrowserStartupController.get(this).startBrowserProcessesAsync(callback)) {
nit: I suppose "this /* context */" would be clearer..

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:22:
* This class controls how C++ browser main loop is run and ensures it happens
only once.
s/is run/is started/

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:25:
* times as needed, but the browser process will still only be initialized once.
All requests to
times as needed (for instance, multiple activities for the same application)

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:37:
void run(int startupResult);
nit: maybe "onStartupComplete" ?

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:70:
private final Context mContext;
nit: I heard rumors that this can cause leaks, specially from a long-lived
singleton like this object.. maybe make it a regular member and assign to null
at initializeAndroidBrowserProcess, since it won't be ever be needed again?

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:99:
* @return Whether the queue of tasks was created successfully.
hmm... would it make sense to have this method "void" and then always
callback.run(error_code)?

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:108:
callback.run(-1);
nit: maybe define -1 as public static final int of the StartupCallback?
I think there's two possible values, "already_initialized" and
"failed_initialization", if we decide to go with the "void" above.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:149:
sBrowserMayStartAsynchronously = true;
need to make find_bugs happy.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:155:
AndroidBrowserProcess.init(mContext, AndroidBrowserProcess.MAX_RENDERERS_LIMIT);
nit: 
Context context = mContext;
mContext = null;

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:66:
final AtomicBoolean startupResult = new AtomicBoolean();
startupResult is confusing with callback.mStartupResult...
as above, "startBrowserProcessesAsync" should be void, and all results be passed
via the callback it takes, wdyt?

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:100:
final AtomicBoolean startupResult1 = new AtomicBoolean();
ditto

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:108:
startupResult2.set(mController.startBrowserProcessesAsync(callback2));
would be more realistic as two separate "runOnUiThreadBlocking()" calls..

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:142:
final AtomicBoolean startupResult = new AtomicBoolean();
ditto

[nyquist, 2013-08-16 02:01:01]

bulach: PTAL

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:22:
* This class controls how C++ browser main loop is run and ensures it happens
only once.
On 2013/08/15 15:09:12, bulach wrote:
> s/is run/is started/

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:25:
* times as needed, but the browser process will still only be initialized once.
All requests to
On 2013/08/15 15:09:12, bulach wrote:
> times as needed (for instance, multiple activities for the same application)

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:37:
void run(int startupResult);
On 2013/08/15 15:09:12, bulach wrote:
> nit: maybe "onStartupComplete" ?

Done. Split into two methods: onSuccess and onFailure.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:70:
private final Context mContext;
On 2013/08/15 15:09:12, bulach wrote:
> nit: I heard rumors that this can cause leaks, specially from a long-lived
> singleton like this object.. maybe make it a regular member and assign to null
> at initializeAndroidBrowserProcess, since it won't be ever be needed again?

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:99:
* @return Whether the queue of tasks was created successfully.
On 2013/08/15 15:09:12, bulach wrote:
> hmm... would it make sense to have this method "void" and then always
> callback.run(error_code)?

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:108:
callback.run(-1);
On 2013/08/15 15:09:12, bulach wrote:
> nit: maybe define -1 as public static final int of the StartupCallback?
> I think there's two possible values, "already_initialized" and
> "failed_initialization", if we decide to go with the "void" above.

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:149:
sBrowserMayStartAsynchronously = true;
On 2013/08/15 15:09:12, bulach wrote:
> need to make find_bugs happy.

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:155:
AndroidBrowserProcess.init(mContext, AndroidBrowserProcess.MAX_RENDERERS_LIMIT);
On 2013/08/15 15:09:12, bulach wrote:
> nit: 
> Context context = mContext;
> mContext = null;

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:66:
final AtomicBoolean startupResult = new AtomicBoolean();
On 2013/08/15 15:09:12, bulach wrote:
> startupResult is confusing with callback.mStartupResult...
> as above, "startBrowserProcessesAsync" should be void, and all results be
passed
> via the callback it takes, wdyt?

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:100:
final AtomicBoolean startupResult1 = new AtomicBoolean();
On 2013/08/15 15:09:12, bulach wrote:
> ditto

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:108:
startupResult2.set(mController.startBrowserProcessesAsync(callback2));
On 2013/08/15 15:09:12, bulach wrote:
> would be more realistic as two separate "runOnUiThreadBlocking()" calls..

Done.

https://codereview.chromium.org/22272006/diff/19001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:142:
final AtomicBoolean startupResult = new AtomicBoolean();
On 2013/08/15 15:09:12, bulach wrote:
> ditto

Done.

[nyquist, 2013-08-16 07:26:22]

haaawk: I will add that in the next round of the review.

[bulach, 2013-08-16 16:52:09]

lgtm, thanks tommy! just some nits and suggestions below.

since this will require a few more OWNERS, maybe best to wait for aberent@ to
look into it by Monday?
I don't think this would make his pending patch particularly more complicated,
but anyways..

https://codereview.chromium.org/22272006/diff/29001/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/22272006/diff/29001/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:74:
// TODO: Show error message.
nit: TODO(someone) ? (understand you're just moving it around..)

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:24:
* It supports kicking of the startup sequence in an asynchronous way. Startup
can be called as many
s/kicking of/kicking off/

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:49:
static final boolean NOT_ALREADY_STARTED = false;
isn't it a bit over-the-top to have named constants for true/false? :)

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:146:
// We can now try to initialize the Android browser process.
nit: some people dislike pronouns, maybe just "Try to initialize.."

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:149:
// async startup. This means that we will never get a callback, so we need to
nit: "so manually call them now"

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:151:
Log.w(TAG, "Browser process was initialized without BrowserStartupController");
should we plan on removing all calls to AndroidBrowserProcess.init?
seems very dodgy to have two entry points.. does the ProcessInitException always
really map to "already initialized" ?

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:201:
return true;
shouldn't this be false?

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:35:
mInitializedCounter++;
should this post a message to executeEnqueuedCallbacks(), instead of the test
code itself? maybe take the param in ctor? then you can remove all posts to
executeEnqueuedCallbacks from the various tests, and it'd also simulate this
more closely to the real code..

[nyquist, 2013-08-16 20:28:59]

bulach: The changes to address the comments were big-ish, so I would be grateful
if you could take another look.
haaawk: FYI

https://codereview.chromium.org/22272006/diff/29001/chrome/android/testshell/...
File
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java
(right):

https://codereview.chromium.org/22272006/diff/29001/chrome/android/testshell/...
chrome/android/testshell/java/src/org/chromium/chrome/testshell/ChromiumTestShellActivity.java:74:
// TODO: Show error message.
On 2013/08/16 16:52:10, bulach wrote:
> nit: TODO(someone) ? (understand you're just moving it around..)

It became TODO(nyquist) and then magically disappeared.

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:24:
* It supports kicking of the startup sequence in an asynchronous way. Startup
can be called as many
On 2013/08/16 16:52:10, bulach wrote:
> s/kicking of/kicking off/

Done.

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:49:
static final boolean NOT_ALREADY_STARTED = false;
On 2013/08/16 16:52:10, bulach wrote:
> isn't it a bit over-the-top to have named constants for true/false? :)

It adds clarity, don't you think? Keep in mind I could go all Enum on you
instead, so I consider this cheap ;-)

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:146:
// We can now try to initialize the Android browser process.
On 2013/08/16 16:52:10, bulach wrote:
> nit: some people dislike pronouns, maybe just "Try to initialize.."

Done.

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:149:
// async startup. This means that we will never get a callback, so we need to
On 2013/08/16 16:52:10, bulach wrote:
> nit: "so manually call them now"

Done.

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:151:
Log.w(TAG, "Browser process was initialized without BrowserStartupController");
On 2013/08/16 16:52:10, bulach wrote:
> should we plan on removing all calls to AndroidBrowserProcess.init?
> seems very dodgy to have two entry points.. does the ProcessInitException
always
> really map to "already initialized" ?

Added TODO to remove AndroidBrowserProcess.
And you're right, it doesn't always map to that. Added new tests to ensure we
cover the different types of exception causes that can occur, and also whether
the init had already been started. Also ensure the prod code works correctly in
all those cases.

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:201:
return true;
On 2013/08/16 16:52:10, bulach wrote:
> shouldn't this be false?

The browser process was not really already initialized, so according to the
method description, it should return true. But this is changed now to support
different ways of throwing ProcessInitException (ie. library load might fail, or
startup itself might fail).

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22272006/diff/29001/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:35:
mInitializedCounter++;
On 2013/08/16 16:52:10, bulach wrote:
> should this post a message to executeEnqueuedCallbacks(), instead of the test
> code itself? maybe take the param in ctor? then you can remove all posts to
> executeEnqueuedCallbacks from the various tests, and it'd also simulate this
> more closely to the real code..

Great idea! This is now implemented.
It naturally requires for synchronously waiting for idle on the main looper to
ensure all callbacks have been handled.

[bulach, 2013-08-19 10:27:45]

lgtm, thanks tommy! minor comments below, anthony is back today so hopefully we
can get all the different aspects of this startup sorted this week.

https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:30:
* All communication with this class must happen on the main thread.
sorry, just realized this now: LOCK is not necessary then?

https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:75:
instance = sInstance;
as above, if this can only happen in the main thread, get rid of LOCK.

https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:109:
synchronized (LOCK) {
ditto

https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:119:
synchronized (LOCK) {
ditto

https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:169:
Log.w(TAG, "Browser process was initialized without BrowserStartupController");
yeah, we really need to get rid of this, please add a TODO to throw an exception
here..

[aberent, 2013-08-19 14:46:31]

On 2013/08/19 10:27:45, bulach wrote:
> lgtm, thanks tommy! minor comments below, anthony is back today so hopefully
we
> can get all the different aspects of this startup sorted this week.

I am back. Just before I went away I was adding code to my CL
(https://codereview.chromium.org/22691002/) to solve, I think, the same problem.
Unfortunately I didn't get that code uploaded to my CL before I went on
vacation. I am still not fully up to speed on your patch, so need a little time
to work out whether they do solve the same problem, and if so how they relate.
 
> 
>
https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
> File
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
> (right):
> 
>
https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:30:
> * All communication with this class must happen on the main thread.
> sorry, just realized this now: LOCK is not necessary then?
> 
>
https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:75:
> instance = sInstance;
> as above, if this can only happen in the main thread, get rid of LOCK.
> 
>
https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:109:
> synchronized (LOCK) {
> ditto
> 
>
https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:119:
> synchronized (LOCK) {
> ditto
> 
>
https://codereview.chromium.org/22272006/diff/38001/content/public/android/ja...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:169:
> Log.w(TAG, "Browser process was initialized without
BrowserStartupController");
> yeah, we really need to get rid of this, please add a TODO to throw an
exception
> here.

[nyquist, 2013-08-19 17:19:12]

jam: OWNERS for content/browser/browser_main_loop.cc and
content/content_browser.gypi
yfriedman: OWNERS for content/shell/android/shell_apk/

Naturally, feel free to take a look at the rest of the patch too.

[Peter Beverloo, 2013-08-19 17:35:40]

Asynchronously starting the browser doesn't work for layout tests in Content
Shell, which is why it was previously disabled. After patching in your CL, I get
the following crash stack (which is very similar to the crash stack we saw
following Anthony's patch).  This is tracked in http://crbug.com/267522.

Can we not enable this for content_shell when --dump-render-tree is set at all?


Fatal signal 11 (SIGSEGV) at 0x00000024 (code=1), thread 8017 (ntent_shell_apk)
*** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
Build fingerprint: 'google/yakju/maguro:4.2.1/JOP40D/533553:userdebug/dev-keys'
pid: 8017, tid: 8017, name: ntent_shell_apk  >>> org.chromium.content_shell_apk
<<<
backtrace:
    #00  pc 0000d598  /system/lib/libc.so
    #01  base/synchronization/lock_impl_posix.cc:45
(base::internal::LockImpl::Lock())
    #02  base/synchronization/lock.h:41 (base::Lock::Acquire())
    #03  base/threading/thread_checker_impl.cc:17
(base::ThreadCheckerImpl::CalledOnValidThread() const)
    #04  base/supports_user_data.cc:15 (base::SupportsUserData::GetUserData(void
const*) const)
    #05  content/browser/ssl/ssl_host_state.cc:16
(content::SSLHostState::GetFor(content::BrowserContext*))
    #06  content/browser/ssl/ssl_policy_backend.cc:14 (SSLPolicyBackend)
    #07  content/browser/ssl/ssl_manager.cc:99 (SSLManager)
    #08  content/browser/web_contents/navigation_controller_impl.cc:214
(NavigationControllerImpl)
    #09  content/browser/web_contents/web_contents_impl.cc:363 (WebContentsImpl)
    #10  content/browser/web_contents/web_contents_impl.cc:420
(content::WebContentsImpl::CreateWithOpener(content::WebContents::CreateParams
const&, content::WebContentsImpl*))
    #11  content/shell/shell.cc:163
(content::Shell::CreateNewWindow(content::BrowserContext*, GURL const&,
content::SiteInstance*, int, gfx::Size const&))
    #12  content/shell/webkit_test_controller.cc:245
(content::WebKitTestController::PrepareForLayoutTest(GURL const&, base::FilePath
const&, bool, std::string const&))
    #13  content/shell/shell_browser_main.cc:180
(ShellBrowserMain(content::MainFunctionParams const&,
scoped_ptr<content::BrowserMainRunner,
base::DefaultDeleter<content::BrowserMainRunner> > const&))
    #14  content/app/content_main_runner.cc:444
(content::RunNamedProcessTypeMain(std::string const&,
content::MainFunctionParams const&, content::ContentMainDelegate*))
    #15  content/app/content_main_runner.cc:776
(content::ContentMainRunnerImpl::Run())
    #16  content/app/android/content_main.cc:45 (Start)

[nyquist, 2013-08-19 17:43:46]

On 2013/08/19 17:35:40, Peter Beverloo wrote:
> Asynchronously starting the browser doesn't work for layout tests in Content
> Shell, which is why it was previously disabled. After patching in your CL, I
get
> the following crash stack (which is very similar to the crash stack we saw
> following Anthony's patch).  This is tracked in http://crbug.com/267522.
> 
> Can we not enable this for content_shell when --dump-render-tree is set at
all?
> 
> 
> Fatal signal 11 (SIGSEGV) at 0x00000024 (code=1), thread 8017
(ntent_shell_apk)
> *** *** *** *** *** *** *** *** *** *** *** *** *** *** *** ***
> Build fingerprint:
'google/yakju/maguro:4.2.1/JOP40D/533553:userdebug/dev-keys'
> pid: 8017, tid: 8017, name: ntent_shell_apk  >>>
org.chromium.content_shell_apk
> <<<
> backtrace:
>     #00  pc 0000d598  /system/lib/libc.so
>     #01  base/synchronization/lock_impl_posix.cc:45
> (base::internal::LockImpl::Lock())
>     #02  base/synchronization/lock.h:41 (base::Lock::Acquire())
>     #03  base/threading/thread_checker_impl.cc:17
> (base::ThreadCheckerImpl::CalledOnValidThread() const)
>     #04  base/supports_user_data.cc:15
(base::SupportsUserData::GetUserData(void
> const*) const)
>     #05  content/browser/ssl/ssl_host_state.cc:16
> (content::SSLHostState::GetFor(content::BrowserContext*))
>     #06  content/browser/ssl/ssl_policy_backend.cc:14 (SSLPolicyBackend)
>     #07  content/browser/ssl/ssl_manager.cc:99 (SSLManager)
>     #08  content/browser/web_contents/navigation_controller_impl.cc:214
> (NavigationControllerImpl)
>     #09  content/browser/web_contents/web_contents_impl.cc:363
(WebContentsImpl)
>     #10  content/browser/web_contents/web_contents_impl.cc:420
> (content::WebContentsImpl::CreateWithOpener(content::WebContents::CreateParams
> const&, content::WebContentsImpl*))
>     #11  content/shell/shell.cc:163
> (content::Shell::CreateNewWindow(content::BrowserContext*, GURL const&,
> content::SiteInstance*, int, gfx::Size const&))
>     #12  content/shell/webkit_test_controller.cc:245
> (content::WebKitTestController::PrepareForLayoutTest(GURL const&,
base::FilePath
> const&, bool, std::string const&))
>     #13  content/shell/shell_browser_main.cc:180
> (ShellBrowserMain(content::MainFunctionParams const&,
> scoped_ptr<content::BrowserMainRunner,
> base::DefaultDeleter<content::BrowserMainRunner> > const&))
>     #14  content/app/content_main_runner.cc:444
> (content::RunNamedProcessTypeMain(std::string const&,
> content::MainFunctionParams const&, content::ContentMainDelegate*))
>     #15  content/app/content_main_runner.cc:776
> (content::ContentMainRunnerImpl::Run())
>     #16  content/app/android/content_main.cc:45 (Start)

Sounds good. I'll disable it for when --dump-render-tree is set.

[nyquist, 2013-08-20 04:56:29]

beverloo: PTAL

[Yaron, 2013-08-20 05:58:58]

lgtm

https://codereview.chromium.org/22272006/diff/60002/content/public/android/ja...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/22272006/diff/60002/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:58:
private static void setAsynchronousStartupConfig() {
*sigh* It's a shame this isn't inlined but I think I see why. Setting static
field so it's a static fn and you want the caller to be overridable by the test
class?

https://codereview.chromium.org/22272006/diff/60002/content/public/android/ja...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:102:
ThreadUtils.assertOnUiThread();
Nit: redundant. I think keep this instead of L101

https://codereview.chromium.org/22272006/diff/60002/content/public/android/ja...
File
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java
(right):

https://codereview.chromium.org/22272006/diff/60002/content/public/android/ja...
content/public/android/javatests/src/org/chromium/content/browser/BrowserStartupControllerTest.java:105:
// The callback should have been setup.
Nit: these comments could be part of assert failure string so it's clearer from
logs what went wrong.

[Peter Beverloo, 2013-08-20 14:42:00]

This works fine for layout tests now. Thank you, Tommy!!

[nyquist, 2013-08-20 15:24:19]

piman: PTAL for OWNERS for
    content/browser/browser_main_loop.cc
    content/content_browser.gypi

[piman, 2013-08-20 17:04:44]

lgtm

[commit-bot: I haz the power, 2013-08-20 18:00:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nyquist@chromium.org/22272006/74001

[commit-bot: I haz the power, 2013-08-20 22:55:56]

Change committed as 218562