Remove app context init from LibraryLoader.

Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext in the various custom application
objects where possible (BaseChromiumApplication and
ContentApplication should not do this due to webview). Most tests and
client code should not call ContextUtils#initApplicationContext.

This fixes WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
Committed: https://crrev.com/81f0e247dd5f8002adf01bb2c4b0c668d5200fca
Cr-Commit-Position: refs/heads/master@{#421906}

[Peter Wen, 2016-08-16 14:55:31]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 14:55:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-08-16 14:57:24]

Description was changed from

==========
Remove app context init from LibraryLoader.

BUG=637389
==========

to

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext. Test may still initialize the
application context multiple times, but client-side code should not.

This would fix the WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
==========

[Peter Wen, 2016-08-16 16:09:34]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 16:09:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-08-16 16:48:08]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 16:48:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-08-16 16:48:43]

wnwen@chromium.org changed reviewers:
+ torne@chromium.org

[Peter Wen, 2016-08-16 16:48:44]

This is following up on the discussion in http://crrev.com/2242173003.

[Peter Wen, 2016-08-16 16:49:55]

Description was changed from

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext. Test may still initialize the
application context multiple times, but client-side code should not.

This would fix the WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
==========

to

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext. Test may still initialize the
application context multiple times, but client-side code should not.

This fixes WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
==========

[commit-bot: I haz the power, 2016-08-16 17:19:51]

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

[commit-bot: I haz the power, 2016-08-16 17:19:52]

Dry run: This issue passed the CQ dry run.

[Torne, 2016-08-18 14:44:55]

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

https://codereview.chromium.org/2247143004/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:135:
public void ensureInitialized(Context context) throws ProcessInitException {
Why do we still need to pass the app context in here as a parameter if we're
requiring that all callers already set the global app context? Couldn't we just
use ContextUtils inside here to get it, preventing any possibility that it's
different?

[Peter Wen, 2016-08-18 15:53:23]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 15:53:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-08-18 15:58:03]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-18 15:58:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-08-18 16:30:01]

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

https://codereview.chromium.org/2247143004/diff/40001/base/android/java/src/o...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:135:
public void ensureInitialized(Context context) throws ProcessInitException {
On 2016/08/18 14:44:55, Torne wrote:
> Why do we still need to pass the app context in here as a parameter if we're
> requiring that all callers already set the global app context? Couldn't we
just
> use ContextUtils inside here to get it, preventing any possibility that it's
> different?

Done.

We can actually do this at all the places where application context is needed,
but I'll avoid bloating this CL even more. :)

[commit-bot: I haz the power, 2016-08-18 16:52:53]

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

[commit-bot: I haz the power, 2016-08-18 16:52:54]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-08-23 18:35:39]

Friendly ping.

[Peter Wen, 2016-09-12 17:08:26]

On 2016/08/23 18:35:39, Peter Wen wrote:
> Friendly ping.

Ping 2. :)

[Torne, 2016-09-20 13:31:17]

LGTM; thanks for doing this and sorry for taking so long to review it :)

[Peter Wen, 2016-09-20 14:11:11]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 14:11:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 15:33:23]

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

[commit-bot: I haz the power, 2016-09-20 15:33:25]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-09-20 16:49:15]

wnwen@chromium.org changed reviewers:
+ agl@chromium.org, clamy@chromium.org, nyquist@chromium.org,
rockot@chromium.org, slan@chromium.org

[Peter Wen, 2016-09-20 16:49:15]

+slan@ for chromecast/ OWNERS.
+rockot@ for mojo/ OWNERS.
+agl@ for net/ OWNERS.
+clamy@ for content/ OWNERS.
+nyquist@ for blimp/, chrome/android/, components/ OWNERS.

[slan, 2016-09-20 16:51:28]

cast lgtm

[Ken Rockot(use gerrit already), 2016-09-20 17:43:25]

mojo lgtm

[agl, 2016-09-20 17:55:57]

RS LGTM

[clamy, 2016-09-21 15:34:38]

clamy@chromium.org changed reviewers:
+ tedchoc@chromium.org

[clamy, 2016-09-21 15:34:39]

@tedchoc: PTAL at the code in content/. I don't feel familiar enough with the
Java code for the review. Thanks!

[Ted C, 2016-09-21 18:36:50]

tedchoc@chromium.org changed reviewers:
+ hanxi@chromium.org

[Ted C, 2016-09-21 18:36:51]

+hanxi for the changes/questions in ChildProcessServiceImpl

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:120:
ContextUtils.initApplicationContext(appContext);
I'll apologize in advance if this is a silly question, but why do we need this
line?

You've got the ChromeApplication instance above by calling
getTargetContext().getApplicationContext().  Why wasn't that calling
initApplicationContext() already?  Is onCreate() not guaranteed to be called at
that point?  What would happen if we overrode the application constructor
instead and had the ContextUtils.initApplicationContext there?

This just doesn't seem like something that should be required here, and would be
nice to not make tests have to call is all.

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java:49:
mAppContext = getInstrumentation().getTargetContext().getApplicationContext();
Same question, but I wonder if this differs because it is an
InstrumentationTestCase

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:184:
LibraryLoader.get(mLibraryProcessType).loadNow(hostBrowserContext);
this seems like it is changing the behavior.  previously, it was using the
context of the browser process, now it looks like it will be using the context
of the process.  I don't know this class well enough to say what is correct, but
I think this need a bit more digging.

Change made here:
https://codereview.chromium.org/2049843004/diff/420001/content/public/android...

And looking at the code, we always pass the app context for both when creating
the browser renderer process, so I think this name is actually quite misleading.
 Maybe for web_apk this is indeed something else, but we should fix the naming
here as it doesn't seem like this would be the browser context but instead the
application context for the renderer isolated process (maybe we should just drop
Browser from the name, and clarify that the host context can match context if
need be).

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291:
ContextUtils.initApplicationContext(mContext.getApplicationContext());
Why is this needed?  With this here, isn't chrome calling initApplicationContext
multiple times.

And now thinking about it, isn't the change in ChildProcessServiceImpl.java also
resulting in chrome calling it multiple times.  Is this for webview since they
don't have an Application to initialize it?  Everything but ContentProviders
should have this set for them by default in the Application creation time.

Should we just move up ContextUtils#initApplicationContext to
BaseChromiumApplication?

It would really be nice to just have two calls to initApplicationContext, one
for webview and one for things where we can control the application.  But in the
multi-process webview case, do we have a hook to initialize the renderer's app
context early enough?

[Xi Han, 2016-09-21 18:52:27]

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:184:
LibraryLoader.get(mLibraryProcessType).loadNow(hostBrowserContext);
On 2016/09/21 18:36:51, Ted C wrote:
> this seems like it is changing the behavior.  previously, it was using the
> context of the browser process, now it looks like it will be using the context
> of the process.  I don't know this class well enough to say what is correct,
but
> I think this need a bit more digging.
> 
> Change made here:
>
https://codereview.chromium.org/2049843004/diff/420001/content/public/android...
> 
> And looking at the code, we always pass the app context for both when creating
> the browser renderer process, so I think this name is actually quite
misleading.
>  Maybe for web_apk this is indeed something else, but we should fix the naming
> here as it doesn't seem like this would be the browser context but instead the
> application context for the renderer isolated process (maybe we should just
drop
> Browser from the name, and clarify that the host context can match context if
> need be).

Thank you Ted for sending this CL to me!

The |hostBrowserContext| is needed here. This is because WebAPK can also host
renderer process, and the way to create a child process is to use reflection to
create a ChildProcessServiceImpl which loads Chrome's native code. Depending in
which app this code is actually running, the |hostBrowserContext| is always
passed in when the ChildProcessServiceImpl was created, since it doesn't always
equal to the context of the current app.

To WebAPK, "host browser" means the browser that owns the WebAPK. Currently it
means Chroms only, but could be beta/dev/canary/stable. I don't have a better
name in my mind, any suggestion?

[Ted C, 2016-09-21 19:04:19]

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:184:
LibraryLoader.get(mLibraryProcessType).loadNow(hostBrowserContext);
On 2016/09/21 18:52:27, Xi Han wrote:
> On 2016/09/21 18:36:51, Ted C wrote:
> > this seems like it is changing the behavior.  previously, it was using the
> > context of the browser process, now it looks like it will be using the
context
> > of the process.  I don't know this class well enough to say what is correct,
> but
> > I think this need a bit more digging.
> > 
> > Change made here:
> >
>
https://codereview.chromium.org/2049843004/diff/420001/content/public/android...
> > 
> > And looking at the code, we always pass the app context for both when
creating
> > the browser renderer process, so I think this name is actually quite
> misleading.
> >  Maybe for web_apk this is indeed something else, but we should fix the
naming
> > here as it doesn't seem like this would be the browser context but instead
the
> > application context for the renderer isolated process (maybe we should just
> drop
> > Browser from the name, and clarify that the host context can match context
if
> > need be).
> 
> Thank you Ted for sending this CL to me!
> 
> The |hostBrowserContext| is needed here. This is because WebAPK can also host
> renderer process, and the way to create a child process is to use reflection
to
> create a ChildProcessServiceImpl which loads Chrome's native code. Depending
in
> which app this code is actually running, the |hostBrowserContext| is always
> passed in when the ChildProcessServiceImpl was created, since it doesn't
always
> equal to the context of the current app.
> 
> To WebAPK, "host browser" means the browser that owns the WebAPK. Currently it
> means Chroms only, but could be beta/dev/canary/stable. I don't have a better
> name in my mind, any suggestion?

What process do webappks generate their renderers in?  Assuming I'm reading
this code correct, this is how we initialize our renderers/gpu processes
etc, right?  And that doesn't happen in the browser process but in the
respective
processes (sandboxed for renderers, not for gpu).

That is why the naming is weird to me.  For Chrome (in regular mode), the
context
used is fully owned/lifetime bound to the renderer process.  The fact that
we are including the word "browser" to me implies that we are using the browser
context to load the native library and that is not the case for chrome.

I think just dropping the name "browser" from the param name is fine.  Or maybe
libraryLoadingContext to be more of a description of its usage instead of
describing
where it came from.

[Xi Han, 2016-09-21 19:37:24]

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:184:
LibraryLoader.get(mLibraryProcessType).loadNow(hostBrowserContext);
On 2016/09/21 19:04:18, Ted C wrote:
> On 2016/09/21 18:52:27, Xi Han wrote:
> > On 2016/09/21 18:36:51, Ted C wrote:
> > > this seems like it is changing the behavior.  previously, it was using the
> > > context of the browser process, now it looks like it will be using the
> context
> > > of the process.  I don't know this class well enough to say what is
correct,
> > but
> > > I think this need a bit more digging.
> > > 
> > > Change made here:
> > >
> >
>
https://codereview.chromium.org/2049843004/diff/420001/content/public/android...
> > > 
> > > And looking at the code, we always pass the app context for both when
> creating
> > > the browser renderer process, so I think this name is actually quite
> > misleading.
> > >  Maybe for web_apk this is indeed something else, but we should fix the
> naming
> > > here as it doesn't seem like this would be the browser context but instead
> the
> > > application context for the renderer isolated process (maybe we should
just
> > drop
> > > Browser from the name, and clarify that the host context can match context
> if
> > > need be).
> > 
> > Thank you Ted for sending this CL to me!
> > 
> > The |hostBrowserContext| is needed here. This is because WebAPK can also
host
> > renderer process, and the way to create a child process is to use reflection
> to
> > create a ChildProcessServiceImpl which loads Chrome's native code. Depending
> in
> > which app this code is actually running, the |hostBrowserContext| is always
> > passed in when the ChildProcessServiceImpl was created, since it doesn't
> always
> > equal to the context of the current app.
> > 
> > To WebAPK, "host browser" means the browser that owns the WebAPK. Currently
it
> > means Chroms only, but could be beta/dev/canary/stable. I don't have a
better
> > name in my mind, any suggestion?
> 
> What process do webappks generate their renderers in?  Assuming I'm reading
> this code correct, this is how we initialize our renderers/gpu processes
> etc, right?  And that doesn't happen in the browser process but in the
> respective
> processes (sandboxed for renderers, not for gpu).
> 
> That is why the naming is weird to me.  For Chrome (in regular mode), the
> context
> used is fully owned/lifetime bound to the renderer process.  The fact that
> we are including the word "browser" to me implies that we are using the
browser
> context to load the native library and that is not the case for chrome.
> 
> I think just dropping the name "browser" from the param name is fine.  Or
maybe
> libraryLoadingContext to be more of a description of its usage instead of
> describing
> where it came from.

In WebAPK's AndroidManifest.xml, we declares WebApkSandboxedProcessService, and
when this service is created by Android, it loads ChildProcessServiceImpl to
create renderers. So I think renderers are generated in the default process that
the WebAPK is created, please correct me if I am wrong.

I am fine with removing "browser" and using "hostContext" or something like
that. We definitely should avoid the confusing that "browser" could indicate the
"browser process".

[Torne, 2016-09-26 09:21:35]

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291:
ContextUtils.initApplicationContext(mContext.getApplicationContext());
On 2016/09/21 18:36:51, Ted C wrote:
> Why is this needed?  With this here, isn't chrome calling
initApplicationContext
> multiple times.
> 
> And now thinking about it, isn't the change in ChildProcessServiceImpl.java
also
> resulting in chrome calling it multiple times.  Is this for webview since they
> don't have an Application to initialize it?  Everything but ContentProviders
> should have this set for them by default in the Application creation time.
> 
> Should we just move up ContextUtils#initApplicationContext to
> BaseChromiumApplication?

We want to get rid of BaseChromiumApplication entirely. It's caused a lot of
code to get introduced in base and content that doesn't work for WebView. Only
things in the chrome layer should be assuming they have a custom Application
subclass.

> It would really be nice to just have two calls to initApplicationContext, one
> for webview and one for things where we can control the application.  But in
the
> multi-process webview case, do we have a hook to initialize the renderer's app
> context early enough?

[Peter Wen, 2016-09-26 19:02:01]

Addressed comment in ChildProcessServiceImpl. Testing changes to
ChromeApplication in https://codereview.chromium.org/2363323003, can't repro
some issues locally. Will address other comments after.

@hanxi - Is this change okay for WebAPK now?

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(left):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:184:
LibraryLoader.get(mLibraryProcessType).loadNow(hostBrowserContext);
Opps, didn't mean to make this change in behaviour. Renamed to hostContext and
discussing offline with Xi came to the conclusion that this is necessary because
WebAPK wants its own application context to be stored in ContextUtils but wants
to just load the library in LibraryLoader with Chrome's application context.

[Xi Han, 2016-09-26 19:05:40]

ChildProcessServiceImpl lgtm.

[Ted C, 2016-09-26 20:41:48]

On 2016/09/26 09:21:35, Torne wrote:
>
https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
> (right):
> 
>
https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291:
> ContextUtils.initApplicationContext(mContext.getApplicationContext());
> On 2016/09/21 18:36:51, Ted C wrote:
> > Why is this needed?  With this here, isn't chrome calling
> initApplicationContext
> > multiple times.
> > 
> > And now thinking about it, isn't the change in ChildProcessServiceImpl.java
> also
> > resulting in chrome calling it multiple times.  Is this for webview since
they
> > don't have an Application to initialize it?  Everything but ContentProviders
> > should have this set for them by default in the Application creation time.
> > 
> > Should we just move up ContextUtils#initApplicationContext to
> > BaseChromiumApplication?
> 
> We want to get rid of BaseChromiumApplication entirely. It's caused a lot of
> code to get introduced in base and content that doesn't work for WebView. Only
> things in the chrome layer should be assuming they have a custom Application
> subclass.

While that is true, it is orthogonal to this change, and I think worth
separate discussions to avoid conflating issues.

The main point I wanted to state is that initApplicationContext should
only happen in early initialization paths and not in feature code.

If we don't want to put it in BaseChromiumApplication, then it needs to
go in every child Application of BaseChromiumApplication and
ContentApplication.

At that point, we might consider adding other base Applications to make this
easier, which is the point where it is outside of the scope of this change.

> 
> > It would really be nice to just have two calls to initApplicationContext,
one
> > for webview and one for things where we can control the application.  But in
> the
> > multi-process webview case, do we have a hook to initialize the renderer's
app
> > context early enough?

[Ted C, 2016-09-26 20:49:15]

https://codereview.chromium.org/2247143004/diff/120001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/2247143004/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:170:
public void loadNow(Context context) throws ProcessInitException {
We should probably name this more explicit as well loadNowWithContextOverride to
make it seem the one you shouldn't call.

https://codereview.chromium.org/2247143004/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:171:
synchronized (sLock) {
If we call this with something other than the application context, I think we
need to be more aggressive about ensure it hasn't already been loaded.

Maybe something like:

if (mLoaded && context != ContextUtils.getApplicationContext) {
    throw new IllegalStateException(....);
}

If we don't do that, then we might allow things to unexpectedly be loaded with
different contexts and have to track that down.

[nyquist, 2016-09-27 01:13:13]

blimp, chrome, components/invalidation lgtm

[Peter Wen, 2016-09-27 20:33:23]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 20:33:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 21:08:28]

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

[commit-bot: I haz the power, 2016-09-27 21:08:30]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Peter Wen, 2016-09-28 14:59:04]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 14:59:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-09-28 15:05:21]

Description was changed from

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext. Test may still initialize the
application context multiple times, but client-side code should not.

This fixes WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
==========

to

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext in the various custom application
objects where possible (BaseChromiumApplication and
ContentApplication should not do this due to webview). Most tests and
client code should not call ContextUtils#initApplicationContext.

This fixes WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
==========

[commit-bot: I haz the power, 2016-09-28 15:39:24]

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

[commit-bot: I haz the power, 2016-09-28 15:39:26]

Dry run: This issue passed the CQ dry run.

[Peter Wen, 2016-09-28 15:49:45]

The CQ bit was checked by wnwen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 15:49:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Peter Wen, 2016-09-28 15:55:17]

@tedchoc - PTAL, removed unnecessary initialization. Still more transitive
clean-up possible, but will do so in follow-up CLs.

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabActivityTest.java:120:
ContextUtils.initApplicationContext(appContext);
On 2016/09/21 18:36:51, Ted C wrote:
> I'll apologize in advance if this is a silly question, but why do we need this
> line?
> 
> You've got the ChromeApplication instance above by calling
> getTargetContext().getApplicationContext().  Why wasn't that calling
> initApplicationContext() already?  Is onCreate() not guaranteed to be called
at
> that point?  What would happen if we overrode the application constructor
> instead and had the ContextUtils.initApplicationContext there?
> 
> This just doesn't seem like something that should be required here, and would
be
> nice to not make tests have to call is all.

Application constructor seems early enough to not make this extra call.

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/customtabs/CustomTabsConnectionTest.java:49:
mAppContext = getInstrumentation().getTargetContext().getApplicationContext();
On 2016/09/21 18:36:51, Ted C wrote:
> Same question, but I wonder if this differs because it is an
> InstrumentationTestCase

Both are fine removing.

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java
(right):

https://codereview.chromium.org/2247143004/diff/100001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/BrowserStartupController.java:291:
ContextUtils.initApplicationContext(mContext.getApplicationContext());
On 2016/09/21 18:36:51, Ted C wrote:
> Why is this needed?  With this here, isn't chrome calling
initApplicationContext
> multiple times.
> 
> And now thinking about it, isn't the change in ChildProcessServiceImpl.java
also
> resulting in chrome calling it multiple times.  Is this for webview since they
> don't have an Application to initialize it?  Everything but ContentProviders
> should have this set for them by default in the Application creation time.
> 
> Should we just move up ContextUtils#initApplicationContext to
> BaseChromiumApplication?
> 
> It would really be nice to just have two calls to initApplicationContext, one
> for webview and one for things where we can control the application.  But in
the
> multi-process webview case, do we have a hook to initialize the renderer's app
> context early enough?

Right, this should not be necessary here.

https://codereview.chromium.org/2247143004/diff/120001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/2247143004/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:170:
public void loadNow(Context context) throws ProcessInitException {
On 2016/09/26 20:49:15, Ted C wrote:
> We should probably name this more explicit as well loadNowWithContextOverride
to
> make it seem the one you shouldn't call.

Done.

https://codereview.chromium.org/2247143004/diff/120001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:171:
synchronized (sLock) {
On 2016/09/26 20:49:15, Ted C wrote:
> If we call this with something other than the application context, I think we
> need to be more aggressive about ensure it hasn't already been loaded.
> 
> Maybe something like:
> 
> if (mLoaded && context != ContextUtils.getApplicationContext) {
>     throw new IllegalStateException(....);
> }
> 
> If we don't do that, then we might allow things to unexpectedly be loaded with
> different contexts and have to track that down.

Agreed, done.

[commit-bot: I haz the power, 2016-09-28 16:39:35]

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

[commit-bot: I haz the power, 2016-09-28 16:39:36]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-09-28 17:27:46]

lgtm w/ a couple small tweaks

https://codereview.chromium.org/2247143004/diff/180001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/2247143004/diff/180001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:173:
if (mLoaded && appContext != ContextUtils.getApplicationContext()) {
should this be in the synchronized block since that is where mLoaded is
modified?

Seems like this could miss the case where a load is happening from a different
thread and already in the loadAlreadyLocked call but hasn't set mLoaded = true
yet.

https://codereview.chromium.org/2247143004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2247143004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:134:
* @param hostContext The context of the host browser (i.e. Chrome).
I would drop browser from the comment as well.

I think this would be fine to say:

The host context the library should be loaded with.

[Peter Wen, 2016-09-29 18:41:39]

Thank you for the quick and thorough review, Ted!

https://codereview.chromium.org/2247143004/diff/180001/base/android/java/src/...
File base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java
(right):

https://codereview.chromium.org/2247143004/diff/180001/base/android/java/src/...
base/android/java/src/org/chromium/base/library_loader/LibraryLoader.java:173:
if (mLoaded && appContext != ContextUtils.getApplicationContext()) {
On 2016/09/28 17:27:46, Ted C wrote:
> should this be in the synchronized block since that is where mLoaded is
> modified?
> 
> Seems like this could miss the case where a load is happening from a different
> thread and already in the loadAlreadyLocked call but hasn't set mLoaded = true
> yet.

Done. Thanks!

https://codereview.chromium.org/2247143004/diff/180001/content/public/android...
File
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
(right):

https://codereview.chromium.org/2247143004/diff/180001/content/public/android...
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:134:
* @param hostContext The context of the host browser (i.e. Chrome).
On 2016/09/28 17:27:46, Ted C wrote:
> I would drop browser from the comment as well.
> 
> I think this would be fine to say:
> 
> The host context the library should be loaded with.

Done.

[Peter Wen, 2016-09-29 18:47:55]

The CQ bit was checked by wnwen@chromium.org

[Peter Wen, 2016-09-29 18:47:56]

The patchset sent to the CQ was uploaded after l-g-t-m from torne@chromium.org,
rockot@chromium.org, slan@chromium.org, agl@chromium.org, nyquist@chromium.org,
hanxi@chromium.org, tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2247143004/#ps200001
(title: "Fix per review.")

[commit-bot: I haz the power, 2016-09-29 18:48:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 19:41:53]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-09-29 19:44:09]

Description was changed from

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext in the various custom application
objects where possible (BaseChromiumApplication and
ContentApplication should not do this due to webview). Most tests and
client code should not call ContextUtils#initApplicationContext.

This fixes WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389
==========

to

==========
Remove app context init from LibraryLoader.

Move towards single-initialization application context through
ContextUtils#initApplicationContext in the various custom application
objects where possible (BaseChromiumApplication and
ContentApplication should not do this due to webview). Most tests and
client code should not call ContextUtils#initApplicationContext.

This fixes WebView apps that create a new wrap for their own
application context every time Application#getApplicationContext is
called.

BUG=637389

Committed: https://crrev.com/81f0e247dd5f8002adf01bb2c4b0c668d5200fca
Cr-Commit-Position: refs/heads/master@{#421906}
==========

[commit-bot: I haz the power, 2016-09-29 19:44:11]

Patchset 11 (id:??) landed as
https://crrev.com/81f0e247dd5f8002adf01bb2c4b0c668d5200fca
Cr-Commit-Position: refs/heads/master@{#421906}