Allow native command line arguments to be initialized early.

The WebView has a path (early use of the CookieManager) which
needs access to the native command line arguments before we have
initialized Chromium.

This cl moves the setting up of the native command line to just after
the LibraryLoaded.load step rather than just before the
LibraryLoaded.initialise step.

We remove the ability to pass the command line to initialize
(which doesn't seem to be used) and remove the command line 
argument from the LibraryLoaded hook. You can access the
native command line arguments instead which will have been set up.

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

[hjd, 2014-05-20 15:55:19]

Could you take a look, I'm trying to disentangle command line
argument passing from initialization in LibraryLoader
slightly to get access to the arguments earlier. Thanks! :)

[bulach, 2014-05-21 11:33:49]

some comments below. I'm also adding aberent@, he knows this area quite a lot
and will hopefully give you a better advise :)

https://codereview.chromium.org/289303004/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/289303004/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:220:
public static void ensureCommandLineInit(String[] initCommandLine) {
hmm... this method seems dangerous, it "ensures" but still can "fail", i.e., if
the command line has already been initialized, callers of this method would
never know that the new switches haven't been applied at all..
there are several entry points for loading the library, and it feels this is
bound to be racy...

https://codereview.chromium.org/289303004/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:230:
nativeInitCommandLine(initCommandLine);
calling an unbound method throws some fairly cryptic exceptions :)
maybe check sInitialized and throw a nicer looking exception?
even better, do this at the method above, since the documentation mandates it
there..

[hjd, 2014-05-22 11:09:10]

Thanks :) Hi aberent@!

I changed the design so you either call initialize(String[] initCommandLine)
xor you call initializeCommandLine(String[] initCommandLine) then
completeInitialization() and attempting to call bad combinations throws and
exception.
This seems better and less likely to go wrong but has added a lot of complicated
checks.
(I think it would be cleaner just to force the callers to always call 
initializeCommandLine and completeInitialization but I don't think I have
access to the clank code and can't see how difficult this would be to change).

https://codereview.chromium.org/289303004/diff/20001/base/android/java/src/or...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/289303004/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:220:
public static void ensureCommandLineInit(String[] initCommandLine) {
On 2014/05/21 11:33:49, bulach wrote:
> hmm... this method seems dangerous, it "ensures" but still can "fail", i.e.,
if
> the command line has already been initialized, callers of this method would
> never know that the new switches haven't been applied at all..
> there are several entry points for loading the library, and it feels this is
> bound to be racy...

Yeah I felt bad writing that name... I've changed it so there are two paths
one where you initialize everything at once and another where you do it in 
two parts. Attempting to initialize the command line early twice is now 
throws and exception.

https://codereview.chromium.org/289303004/diff/20001/base/android/java/src/or...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:230:
nativeInitCommandLine(initCommandLine);
On 2014/05/21 11:33:49, bulach wrote:
> calling an unbound method throws some fairly cryptic exceptions :)
> maybe check sInitialized and throw a nicer looking exception?
> even better, do this at the method above, since the documentation mandates it
> there..

I added a lot of extra LoaderErrors and checked that sLoaded was true :)

[aberent, 2014-05-29 17:38:14]

lgtm

[hjd, 2014-06-13 11:17:34]

The CQ bit was checked by hjd@chromium.org

[commit-bot: I haz the power, 2014-06-13 11:18:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hjd@chromium.org/289303004/100001

[commit-bot: I haz the power, 2014-06-13 14:36:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-06-13 14:39:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-13 14:39:34]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[hjd, 2014-06-18 11:18:58]

Could I get a few OWNERS reviews please :)

Marcus for .*library_loader.* and
Torne for CookieManagerStartupTest.java

Thanks!

[mkosiba (inactive), 2014-06-18 16:54:45]

https://codereview.chromium.org/289303004/diff/100001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java
(right):

https://codereview.chromium.org/289303004/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java:34:
LibraryLoader.initializeCommandLine(new String[]{});
is this to prevent a crash or to test that it works?

[bulach, 2014-06-18 17:06:33]

+ross, some suggestions below..

https://codereview.chromium.org/289303004/diff/100001/base/android/java/src/o...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/289303004/diff/100001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:228:
if (!sLoaded) {
sorry, I'm still not truly convinced about this... :)
afaict, there are two entry points:
- synchronous library loading: by calling loadNow(), it'll block until the
library is loaded. it also block any other components trying to load the
library.
- asynchronous: the component spawns a background thread that calls loadNow(),
which will block until its done (either itself, or something else).

In other words: there may be different components that are trying to load
library in different ways concurrently.
then, they may end up calling this initializeCommandLine() here. but they have
*no* control over other components, so this exception may be thrown, depending
on the order things happen at runtime... :(

further, CommandLine.java has its own mechanism to try to keep the java side and
the native in sync.. that is, any component in the java land can set the flags,
and eventually, CommandLine.enableNativeProxy() here will make sure those are
passed to native.

so here's some crazy idea :)
how about _remove_ the public initialize(String[] ..) from here?

so then the flow would be roughly:
1. anybody can call into CommandLine.java to set their flags.
2. when this is initializing, it'll then always get the String[] from
CommandLine.java
3. nothing in here exposes the command line or its initialization

would that make things simpler? what would it break?! :)

[hjd, 2014-06-18 19:24:41]

On 2014/06/18 17:06:33, bulach wrote:
> +ross, some suggestions below..
> 
>
https://codereview.chromium.org/289303004/diff/100001/base/android/java/src/o...
> File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
> (right):
> 
>
https://codereview.chromium.org/289303004/diff/100001/base/android/java/src/o...
> base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:228:
> if (!sLoaded) {
> sorry, I'm still not truly convinced about this... :)
> afaict, there are two entry points:
> - synchronous library loading: by calling loadNow(), it'll block until the
> library is loaded. it also block any other components trying to load the
> library.
> - asynchronous: the component spawns a background thread that calls loadNow(),
> which will block until its done (either itself, or something else).
> 
> In other words: there may be different components that are trying to load
> library in different ways concurrently.
> then, they may end up calling this initializeCommandLine() here. but they have
> *no* control over other components, so this exception may be thrown, depending
> on the order things happen at runtime... :(
> 
> further, CommandLine.java has its own mechanism to try to keep the java side
and
> the native in sync.. that is, any component in the java land can set the
flags,
> and eventually, CommandLine.enableNativeProxy() here will make sure those are
> passed to native.
> 
> so here's some crazy idea :)
> how about _remove_ the public initialize(String[] ..) from here?
> 
> so then the flow would be roughly:
> 1. anybody can call into CommandLine.java to set their flags.
> 2. when this is initializing, it'll then always get the String[] from
> CommandLine.java
> 3. nothing in here exposes the command line or its initialization
> 
> would that make things simpler? what would it break?! :)

Ahh I see what you mean about multiple components.

I think that plan makes a lot of sense, it is a lot simpler :)

I suspect though that changing initialize() means changing something in clank
right?
I'll have a look at the clank noogler guide tomorrow but I might need a hand
from someone to complete the change on that side :)

[bulach, 2014-06-18 22:15:30]

great!! :)
I hope it'll be possible to do this simplification, thanks for investigating it!

I'm not in the office this week, but any of the folks around here (mkosiba,
rmcilroy, aberent) should be able to help you get the other repositories...

[hjd, 2014-07-02 13:11:53]

ptal

Marcus: base/android/library_loader/
Martin: CommandLineTest.java

OWNERS:
yfriedman@: ChildProcessService.java and library_loader_hooks.cc
jochen@: content/public/app/android_library_loader_hooks.h

https://codereview.chromium.org/289303004/diff/100001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java
(right):

https://codereview.chromium.org/289303004/diff/100001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/CookieManagerStartupTest.java:34:
LibraryLoader.initializeCommandLine(new String[]{});
On 2014/06/18 16:54:45, mkosiba wrote:
> is this to prevent a crash or to test that it works?

Prevent a crash but as we talked about I'll right a proper test.

[mkosiba (inactive), 2014-07-02 13:25:01]

test LGTM

[jochen (gone - plz use gerrit), 2014-07-02 13:28:49]

lgtm

[bulach, 2014-07-02 13:41:57]

lgtm, nice thanks!

[rmcilroy, 2014-07-02 15:18:09]

lgtm

[hjd, 2014-08-11 10:52:33]

The CQ bit was checked by hjd@chromium.org

[commit-bot: I haz the power, 2014-08-11 10:53:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hjd@chromium.org/289303004/200001

[commit-bot: I haz the power, 2014-08-11 13:36:04]

Change committed as 288697