[Cronet] Make sure to only pass the application Context to InitApplicationContext to avoid DCHECK fa

[Cronet] Make sure to only pass the application Context to InitApplicationContext to avoid DCHECK failures.

An application may have multiple Contexts but only one application Context.  InitApplicationContext DCHECKs if passed more than one Context, so make sure to always pass it only the application Context.
Also, clean up other uses of Contexts in Cronet to only use the application Context when necessary.  Unnecessarily using the application Context was hiding the bug this CL fixes.  I'm renaming variables to make it clear where an application context is needed versus where any Context is okay.  It's good practice to use the Context explicitly passed in as the wrapping it provides may add additional functionality on top of wrapped Contexts and the application Context.
Also, add a test that Cronet can be simultaneously instantiated upon a Context, a wrapped version of the Context and the application Context to verify Cronet doesn't DCHECK.

BUG=453845
Committed: https://crrev.com/6c10657ea6fc4bfe370e61fde5d0405aa22b0e81
Cr-Commit-Position: refs/heads/master@{#319622}

[pauljensen, 2015-03-06 12:37:33]

Patchset #1 (id:1) has been deleted

[pauljensen, 2015-03-06 13:11:05]

pauljensen@chromium.org changed reviewers:
+ mef@chromium.org, mmenke@chromium.org, xunjieli@chromium.org

[pauljensen, 2015-03-06 13:11:05]

PTAL, thanks!

[xunjieli, 2015-03-06 14:45:41]

On 2015/03/06 13:11:05, pauljensen wrote:
> PTAL, thanks!

LGTM. thanks for the fix!

[xunjieli, 2015-03-06 14:50:31]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:600:
public void testInitDifferentContexts() throws Exception {
Btw, should we test creating multiple ChromiumUrlRequestContext too?

[mmenke, 2015-03-06 15:58:28]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:44:
CronetLibraryLoader.ensureInitialized(context, config);
Should this also use getApplicationContext?  Also, I think we should be clearer
on what is the app context and what is the...erm...non-app context.  Looking
through the files, I have no idea when we expect to have an app context, and
when any old context will do.

[pauljensen, 2015-03-06 16:21:15]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:44:
CronetLibraryLoader.ensureInitialized(context, config);
On 2015/03/06 15:58:28, mmenke wrote:
> Should this also use getApplicationContext?  

I tried to explain this in the CL description.  I think the best practice is to
use whatever Context a caller passed in whenever possible as this one contains
the most wrappers which may be adding functionality.  An application context
should only be used when necessary.

InitApplicationContext is AFAIK the only thing that needs an application
Context.  I imagine InitApplicationContext only really needs an application
Context instead of a normal Context because it makes sure the Context pointer it
receives isn't changing (which is the case for an application Context only).
I don't believe ensureInitiailized() requires an application Context, so I'd
recommend just passing |context| straight in.

> Also, I think we should be clearer
> on what is the app context and what is the...erm...non-app context.  Looking
> through the files, I have no idea when we expect to have an app context, and
> when any old context will do.
I tried in this CL to rename all parameters accordingly:  "context" when any old
Context will do.  "appContext" or "app_context" when only an application Context
will do.  Unfortunately AFAIK there isn't a way to enforce this with types.

[mmenke, 2015-03-06 16:52:24]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:44:
CronetLibraryLoader.ensureInitialized(context, config);
On 2015/03/06 16:21:15, pauljensen wrote:
> On 2015/03/06 15:58:28, mmenke wrote:
> > Should this also use getApplicationContext?  
> 
> I tried to explain this in the CL description.  I think the best practice is
to
> use whatever Context a caller passed in whenever possible as this one contains
> the most wrappers which may be adding functionality.  An application context
> should only be used when necessary.
> 
> InitApplicationContext is AFAIK the only thing that needs an application
> Context.  I imagine InitApplicationContext only really needs an application
> Context instead of a normal Context because it makes sure the Context pointer
it
> receives isn't changing (which is the case for an application Context only).
> I don't believe ensureInitiailized() requires an application Context, so I'd
> recommend just passing |context| straight in.
> 
> > Also, I think we should be clearer
> > on what is the app context and what is the...erm...non-app context.  Looking
> > through the files, I have no idea when we expect to have an app context, and
> > when any old context will do.
> I tried in this CL to rename all parameters accordingly:  "context" when any
old
> Context will do.  "appContext" or "app_context" when only an application
Context
> will do.  Unfortunately AFAIK there isn't a way to enforce this with types.

Hrm...For some reason I was thinking you were adding more
getApplicationContext() than you are...  Looks like you're doing as well as can
be expected here, given the rather funky nature of the whole app context vs
context divide.

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:608:
mActivity.getApplicationContext(), new UrlRequestContextConfig());
Shouldn't you just be passing in the mActivity here?

[mef, 2015-03-06 16:55:55]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File components/cronet/android/chromium_url_request_context.cc (right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/chromium_url_request_context.cc:88: // Set application
context.
Do we need to call it at all if ensureInitialized does it anyway?

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:28:
context, UserAgent.from(context), config.toString());
Just to make sure I understand correctly: it is ok to call
context.getApplicationContext() on appContext, right? The reason I'm asking is
that we call it again inside of ChromiumUrlRequestContext() constructor.

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:198:
private static native long nativeCreateRequestContextAdapter(Context appContext,
String config);
does this really need appContext? It doesn't seem to use it.

[pauljensen, 2015-03-06 16:57:28]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:608:
mActivity.getApplicationContext(), new UrlRequestContextConfig());
On 2015/03/06 16:52:23, mmenke wrote:
> Shouldn't you just be passing in the mActivity here?
That's what I do on the previous line.
I'm trying three easy situations concurrently:
1. mActivity
2. mActivity's application context (kind of the parent of mActivity)
3. a wrapper on mActivity (kind of a child of mActivity)

[pauljensen, 2015-03-06 17:01:53]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:600:
public void testInitDifferentContexts() throws Exception {
On 2015/03/06 14:50:31, xunjieli wrote:
> Btw, should we test creating multiple ChromiumUrlRequestContext too?

Done.

[mmenke, 2015-03-06 17:07:48]

Thanks for bearing with me.  LGTM.

[pauljensen, 2015-03-06 17:16:52]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File components/cronet/android/chromium_url_request_context.cc (right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/chromium_url_request_context.cc:88: // Set application
context.
On 2015/03/06 16:55:55, mef wrote:
> Do we need to call it at all if ensureInitialized does it anyway?

ChromiumUrlRequestContext() doesn't call ensureInitialized().
ChromiumUrlRequestFactory() seems to be the main caller of
ChromiumUrlRequestContext() and does call ensureInitialized(), however:
ChromiumUrlRequestContext() is protected so I guess someone could define a
subclass and call it without calling ensureInitialized.
So unless we rework these things, I think this still needs to call
InitApplicationContext() here.
Perhaps something for another CL.

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java
(right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/java/src/org/chromium/net/ChromiumUrlRequestFactory.java:28:
context, UserAgent.from(context), config.toString());
On 2015/03/06 16:55:55, mef wrote:
> Just to make sure I understand correctly: it is ok to call
> context.getApplicationContext() on appContext, right? The reason I'm asking is
> that we call it again inside of ChromiumUrlRequestContext() constructor.

Calling getApplicationContext() on an application Context just returns itself.
I think there is really only one functional implementation of
getApplicationContext(), the one in ContextImpl:
http://androidxref.com/5.0.0_r2/search?q=&defs=getApplicationContext&refs=&pa...
which should just return the same thing every time.

[mef, 2015-03-06 17:23:14]

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
File components/cronet/android/chromium_url_request_context.cc (right):

https://codereview.chromium.org/981013002/diff/20001/components/cronet/androi...
components/cronet/android/chromium_url_request_context.cc:88: // Set application
context.
On 2015/03/06 17:16:52, pauljensen wrote:
> On 2015/03/06 16:55:55, mef wrote:
> > Do we need to call it at all if ensureInitialized does it anyway?
> 
> ChromiumUrlRequestContext() doesn't call ensureInitialized().
> ChromiumUrlRequestFactory() seems to be the main caller of
> ChromiumUrlRequestContext() and does call ensureInitialized(), however:
> ChromiumUrlRequestContext() is protected so I guess someone could define a
> subclass and call it without calling ensureInitialized.
> So unless we rework these things, I think this still needs to call
> InitApplicationContext() here.
> Perhaps something for another CL.

Good point. I think we do need to call ensureInitialized() from
ChromiumUrlRequestContext() because subclass scenario will otherwise crash in
call to nativeCreateRequestContextAdapter() if native library is not loaded.

[pauljensen, 2015-03-09 13:09:32]

The CQ bit was checked by pauljensen@chromium.org

[pauljensen, 2015-03-09 13:09:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
xunjieli@chromium.org
Link to the patchset: https://codereview.chromium.org/981013002/#ps40001 (title:
"add ChromiumUrlRequestContext test too")

[commit-bot: I haz the power, 2015-03-09 13:09:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/981013002/40001

[commit-bot: I haz the power, 2015-03-09 13:41:46]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-03-09 13:42:23]

Patchset 2 (id:??) landed as
https://crrev.com/6c10657ea6fc4bfe370e61fde5d0405aa22b0e81
Cr-Commit-Position: refs/heads/master@{#319622}