Allow OpenSSLPrivateKeyStore and Android global app context leaking.

Chromium's key generation is running on a non-joinable worker thread, which requirs us to use leakable Singleton/LazyInstance object since.

See crbug.com/134118 for details

BUG=134118
TEST=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157083

[digit1, 2012-06-20 15:18:36]

On 2012/06/20 13:01:29, Johnny(Jianning) Ding wrote:

Quick question, what about using a leaky LazyInstance<> instead?
I mean using something like:

  base::LazyInstance<Class,base::LeakyLazyInstanceTraits>  instance =
LAZY_INSTANCE_INITIALIZER

As far as I understand, these are not destroyed by the AtExitManager(), or
anything else (on the other hand, they really leak).

[joth, 2012-06-20 16:11:32]

On 20 June 2012 06:01, <jnd@chromium.org> wrote:

> Reviewers: wtc, joth,
>
> Description:
> Run Keygen in SequencedWorkerPool instead of WorkerPool.
>
> keygen_handler.**GenKeyAndSignChallenge uses LazyInstance, See
> BrowserMessageFilter::Send() which uses LazyInstance in
> MessageLoop::current() and openssl implementation of
> KeygenHandler::**GenKeyAndSignChallenge which calls
> OpenSSLPrivateKeyStore::**GetInstance().
>
> The LazyInstance is disallowed to use in worker thread because
> the  worker thread is not joinable, but the LazyInstance may be
> deleted by the AtExitManager, causing a non-joinable thread
> which is executing after ~AtExitManager to access a deleted
> pointer. See crbug.com/63678.
>
> We should use the new SequencedWorkerPool which is (by default)
> joinable.
>
> BUG=63678
>
I'm not sure this is the right BUG= line? 63678 is specifically about
BrowserThread using message loop. AFAICT this patch does not fix that issue.

Is there another bug you can link specifically about
the OpenSSLPrivateKeyStore lazy instance?




> TEST=KeygenHandlerTest(part of net unittests) remain pass.
>
> Please review this at
http://codereview.chromium.**org/10580038/<http://codereview.chromium.org/105...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src/<http://svn.chromium.org/chrome/trunk/src/>
>
> Affected files:
>  M     content/browser/renderer_host/**render_message_filter.cc
>  M     net/base/keygen_handler_**unittest.cc
>
>
> Index: content/browser/renderer_host/**render_message_filter.cc
> ==============================**==============================**=======
> --- content/browser/renderer_host/**render_message_filter.cc
>  (revision 143163)
> +++ content/browser/renderer_host/**render_message_filter.cc
>  (working copy)
> @@ -823,13 +823,13 @@
>   }
>
>   VLOG(1) << "Dispatching keygen task to worker pool.";
> -  // Dispatch to worker pool, so we do not block the IO thread.
> -  if (!base::WorkerPool::PostTask(
> +  // Dispatch to sequenced worker pool, so we do not block the IO thread
> and
> +  // the task is guaranteed to run before shutdown.
> +  if (!BrowserThread::**PostBlockingPoolTask(
>            FROM_HERE,
>            base::Bind(
>                &RenderMessageFilter::**OnKeygenOnWorkerThread, this,
> -               key_size_in_bits, challenge_string, url, reply_msg),
> -           true)) {
> +               key_size_in_bits, challenge_string, url, reply_msg))) {
>     NOTREACHED() << "Failed to dispatch keygen task to worker pool";
>     ViewHostMsg_Keygen::**WriteReplyParams(reply_msg, std::string());
>     Send(reply_msg);
> Index: net/base/keygen_handler_**unittest.cc
> ==============================**==============================**=======
> --- net/base/keygen_handler_**unittest.cc (revision 143163)
> +++ net/base/keygen_handler_**unittest.cc (working copy)
> @@ -10,7 +10,8 @@
>  #include "base/bind.h"
>  #include "base/location.h"
>  #include "base/logging.h"
> -#include "base/threading/worker_pool.h"
> +#include "base/memory/ref_counted.h"
> +#include "base/threading/sequenced_**worker_pool.h"
>  #include "base/threading/thread_**restrictions.h"
>  #include "base/synchronization/**waitable_event.h"
>  #include "build/build_config.h"
> @@ -109,13 +110,14 @@
>   const int NUM_HANDLERS = 5;
>   base::WaitableEvent* events[NUM_HANDLERS] = { NULL };
>   std::string results[NUM_HANDLERS];
> +  scoped_refptr<base::**SequencedWorkerPool> keygen_worker_pool(
> +      new base::SequencedWorkerPool(NUM_**HANDLERS,
> "keygen_worker_pool"));
>   for (int i = 0; i < NUM_HANDLERS; i++) {
>     events[i] = new base::WaitableEvent(false, false);
> -    base::WorkerPool::PostTask(
> +    keygen_worker_pool->**PostWorkerTask(
>         FROM_HERE,
>         base::Bind(**ConcurrencyTestCallback, events[i], "some challenge",
> -                   &results[i]),
> -        true);
> +                   &results[i]));
>   }
>
>   for (int i = 0; i < NUM_HANDLERS; i++) {
> @@ -127,6 +129,7 @@
>     VLOG(1) << "KeygenHandler " << i << " produced: " << results[i];
>     AssertValidSignedPublicKeyAndC**hallenge(results[i], "some
> challenge");
>   }
> +  keygen_worker_pool->Shutdown()**;
>  }
>
>  }  // namespace
>
>
>

[joth, 2012-06-20 16:14:46]

Yes Looks like leaky instance/singleton could work in this case, as
OpenSSLKeyStoreAndroid has no state and is not doing anything in its destructor.

http://code.google.com/searchframe#OAMlx_jo-ck/src/net/base/openssl_private_k...

Adding wilchan as he's likely got some input on leaky instances vs. switching
from worker pool to sequenced task pool.

[wtc, 2012-06-20 18:37:25]

rsleevi: please take a look at this CL.

jnd: I also think that using a leaky Lazy Instance would be the
best solution.  But can we do that to the LazyInstance used
by MessageLoop::current()?  Is the LazyInstance in
MessageLoop::current() also disallowed on a non-joinable
worker thread?  That would be very restrictive.

[Ryan Sleevi, 2012-06-20 18:47:33]

On 2012/06/20 18:37:25, wtc wrote:
> rsleevi: please take a look at this CL.
> 
> jnd: I also think that using a leaky Lazy Instance would be the
> best solution.  But can we do that to the LazyInstance used
> by MessageLoop::current()?  Is the LazyInstance in
> MessageLoop::current() also disallowed on a non-joinable
> worker thread?  That would be very restrictive.

NACK. Do not use the Browser's SequencedWorkerPool. I've discussed this with
Will before, about my concerns in general about the crypto worker threads. The
Browser's pool is fixed at three threads, and that can easily be monopolized by
keygen.

It's not clear how Issue 63678 is related, based on the description you
provided, and regardless, sounds like a separate issue.

If Keygen on OpenSSL requires a LazyInstance for the <keygen> implementation, it
should be a LazyInstance::Leaky. We already use leaky instances for crypto on
all other platforms, so that's certainly appropriate.

If the render message filter handler for Keygen requires posting back to a
particular thread, and the task that runs on the thread is using
BrowserThread::PostTask(Some_ID), then the correct solution is that the
keygen-on-worker thread should be getting passed a callback that will post back
to the correct thread using a MessageLoopProxy.

eg:

void RunsOnWorkerThread(MessageLoopProxy* proxy, const base::Callback<int>&
response_task, int arg1, int arg2, etc) {
  int result = RunSomeMethod(arg1, arg2);
  proxy->PostTask(base::Bind(response_task, result));
}

However, you can do even better with PostTaskAndReplyWithResult

int RunOnWorkerThread(int arg1, int arg2) {
  return RunSomeMethod(arg1, arg2);
}

void RunOnCallerThread(int result) {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::foo));
}

base::PostTaskAndReplyWithResult(
  worker_thread_task_runner,
  FROM_HERE,
  base::Bind(&RunOnWorkerThread, arg1, arg2),
  base::Bind(&RunOnCallerThread));

[Johnny(Jianning) Ding, 2012-06-22 12:59:13]

File new bug 134118 for this issue.

According to my investigation, using LazyInstance::Leak will introduce real leak
issue.
In OpenSSLPrivateKeyStoreAndroid, it calls Android side APIs via JNI, however we
have to a global LazyInstance to store the global app context which is required
by the net library JNI. (See net_library.cc), and we do want to cleanup the java
side app context once Chrome shuts down in order to avoid keep unused java
object in the memory.

The keygen APIs in Chromium Android does need to call the Android system APIs
via JNI and the JNI has global (LazyInstance) variables and needs to be cleanup
when shutting down chrome, unless we can provide the explicit cleanup routine
and call it during shutdown, we have to make the key APIs run on a joinable
thread.

On 2012/06/20 18:47:33, Ryan Sleevi wrote:
> On 2012/06/20 18:37:25, wtc wrote:
> > rsleevi: please take a look at this CL.
> > 
> > jnd: I also think that using a leaky Lazy Instance would be the
> > best solution.  But can we do that to the LazyInstance used
> > by MessageLoop::current()?  Is the LazyInstance in
> > MessageLoop::current() also disallowed on a non-joinable
> > worker thread?  That would be very restrictive.
> 
> NACK. Do not use the Browser's SequencedWorkerPool. I've discussed this with
> Will before, about my concerns in general about the crypto worker threads. The
> Browser's pool is fixed at three threads, and that can easily be monopolized
by
> keygen.
> 
> It's not clear how Issue 63678 is related, based on the description you
> provided, and regardless, sounds like a separate issue.
> 
> If Keygen on OpenSSL requires a LazyInstance for the <keygen> implementation,
it
> should be a LazyInstance::Leaky. We already use leaky instances for crypto on
> all other platforms, so that's certainly appropriate.
> 
> If the render message filter handler for Keygen requires posting back to a
> particular thread, and the task that runs on the thread is using
> BrowserThread::PostTask(Some_ID), then the correct solution is that the
> keygen-on-worker thread should be getting passed a callback that will post
back
> to the correct thread using a MessageLoopProxy.
> 
> eg:
> 
> void RunsOnWorkerThread(MessageLoopProxy* proxy, const base::Callback<int>&
> response_task, int arg1, int arg2, etc) {
>   int result = RunSomeMethod(arg1, arg2);
>   proxy->PostTask(base::Bind(response_task, result));
> }
> 
> However, you can do even better with PostTaskAndReplyWithResult
> 
> int RunOnWorkerThread(int arg1, int arg2) {
>   return RunSomeMethod(arg1, arg2);
> }
> 
> void RunOnCallerThread(int result) {
>   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::foo));
> }
> 
> base::PostTaskAndReplyWithResult(
>   worker_thread_task_runner,
>   FROM_HERE,
>   base::Bind(&RunOnWorkerThread, arg1, arg2),
>   base::Bind(&RunOnCallerThread));

[joth, 2012-06-22 15:54:28]

Another option might be to continue to close the global context on
shutdown, but code the net_library code in worker thread to be defensive
against the context being null, and only use the context via a temporary
local ref to the same.

Strictly that means we should store the global context in an Atomic32.
Certainly, it would require a memory barrier between nulling it and
releasing it on shutdown.



On 22 June 2012 05:59, <jnd@chromium.org> wrote:

> File new bug 134118 for this issue.
>
> According to my investigation, using LazyInstance::Leak will introduce
> real leak
> issue.
> In OpenSSLPrivateKeyStoreAndroid, it calls Android side APIs via JNI,
> however we
> have to a global LazyInstance to store the global app context which is
> required
> by the net library JNI. (See net_library.cc), and we do want to cleanup
> the java
> side app context once Chrome shuts down in order to avoid keep unused java
> object in the memory.
>
> The keygen APIs in Chromium Android does need to call the Android system
> APIs
> via JNI and the JNI has global (LazyInstance) variables and needs to be
> cleanup
> when shutting down chrome, unless we can provide the explicit cleanup
> routine
> and call it during shutdown, we have to make the key APIs run on a joinable
> thread.
>
>
> On 2012/06/20 18:47:33, Ryan Sleevi wrote:
>
>> On 2012/06/20 18:37:25, wtc wrote:
>> > rsleevi: please take a look at this CL.
>> >
>> > jnd: I also think that using a leaky Lazy Instance would be the
>> > best solution.  But can we do that to the LazyInstance used
>> > by MessageLoop::current()?  Is the LazyInstance in
>> > MessageLoop::current() also disallowed on a non-joinable
>> > worker thread?  That would be very restrictive.
>>
>
>  NACK. Do not use the Browser's SequencedWorkerPool. I've discussed this
>> with
>> Will before, about my concerns in general about the crypto worker
>> threads. The
>> Browser's pool is fixed at three threads, and that can easily be
>> monopolized
>>
> by
>
>> keygen.
>>
>
>  It's not clear how Issue 63678 is related, based on the description you
>> provided, and regardless, sounds like a separate issue.
>>
>
>  If Keygen on OpenSSL requires a LazyInstance for the <keygen>
>> implementation,
>>
> it
>
>> should be a LazyInstance::Leaky. We already use leaky instances for
>> crypto on
>> all other platforms, so that's certainly appropriate.
>>
>
>  If the render message filter handler for Keygen requires posting back to a
>> particular thread, and the task that runs on the thread is using
>> BrowserThread::PostTask(Some_**ID), then the correct solution is that the
>> keygen-on-worker thread should be getting passed a callback that will post
>>
> back
>
>> to the correct thread using a MessageLoopProxy.
>>
>
>  eg:
>>
>
>  void RunsOnWorkerThread(**MessageLoopProxy* proxy, const
>> base::Callback<int>&
>> response_task, int arg1, int arg2, etc) {
>>   int result = RunSomeMethod(arg1, arg2);
>>   proxy->PostTask(base::Bind(**response_task, result));
>> }
>>
>
>  However, you can do even better with PostTaskAndReplyWithResult
>>
>
>  int RunOnWorkerThread(int arg1, int arg2) {
>>   return RunSomeMethod(arg1, arg2);
>> }
>>
>
>  void RunOnCallerThread(int result) {
>>   DCHECK(BrowserThread::**CurrentlyOn(BrowserThread::**foo));
>> }
>>
>
>  base::**PostTaskAndReplyWithResult(
>>   worker_thread_task_runner,
>>   FROM_HERE,
>>   base::Bind(&RunOnWorkerThread, arg1, arg2),
>>   base::Bind(&RunOnCallerThread)**);
>>
>
>
>
http://codereview.chromium.**org/10580038/<http://codereview.chromium.org/105...
>

[Ryan Sleevi, 2012-06-22 16:51:30]

On 2012/06/22 12:59:13, Johnny(Jianning) Ding wrote:
> File new bug 134118 for this issue.
> 
> According to my investigation, using LazyInstance::Leak will introduce real
leak
> issue.
> In OpenSSLPrivateKeyStoreAndroid, it calls Android side APIs via JNI, however
we
> have to a global LazyInstance to store the global app context which is
required
> by the net library JNI. (See net_library.cc), and we do want to cleanup the
java
> side app context once Chrome shuts down in order to avoid keep unused java
> object in the memory.
> 
> The keygen APIs in Chromium Android does need to call the Android system APIs
> via JNI and the JNI has global (LazyInstance) variables and needs to be
cleanup
> when shutting down chrome, unless we can provide the explicit cleanup routine
> and call it during shutdown, we have to make the key APIs run on a joinable
> thread.

I disagree with this conclusion - we don't have to run the key APIs on a
joinable thread, and we should be looking at other solutions. For one,
private-key interaction on Android require interactions on the UI thread, AIUI,
so running on a joinable thread runs the significant risk of deadlock while
shutdown.

Further, if the library instance is not usable on joinable threads, then you
have other issues throughout net/ code. For example, both certificate
verification and proxy resolution happen on non-joinable threads.

All of our uses of LeakyInstance only leak on shutdown, with the assumption that
the OS is smart enough to gracefully release memory. If you're trying to use the
system in a way that you can shut down at will, I'll tell you know, that is a
design pattern that is not supported within Chrome at all levels - and for good
reasons. If Android doesn't release resources when the running application
terminates, that sounds like a fundamental issue.

Perhaps we should have this discussion elsewhere, as I don't think codereview is
the most condusive for what sounds like a high-bandwidth conversion that needs
to work out design patterns.

[wtc, 2012-08-01 17:46:07]

jnd: what's the status of this CL?  If you have abandoned
this approach, could you mark the CL closed?  Thanks.

[Johnny(Jianning) Ding, 2012-08-01 18:02:07]

On 2012/08/01 17:46:07, wtc wrote:
> jnd: what's the status of this CL?  If you have abandoned
> this approach, could you mark the CL closed?  Thanks.

It's still blocked on whether we can leak the Java App Context object in order
to run openssl keygen in non-joinable thread with using LazyInstance::Leak.

@joth, who should we ask from Android side for above question?

Will loop Philippe in this thread since he is actively working on this area.

[joth, 2012-08-01 18:26:04]

On 1 August 2012 11:02, <jnd@chromium.org> wrote:

> On 2012/08/01 17:46:07, wtc wrote:
>
>> jnd: what's the status of this CL?  If you have abandoned
>> this approach, could you mark the CL closed?  Thanks.
>>
>
> It's still blocked on whether we can leak the Java App Context object in
> order
> to run openssl keygen in non-joinable thread with using LazyInstance::Leak.
>
> @joth, who should we ask from Android side for above question?
>
> 0/ turns out we never run AtExit on android anyway so it's kindof moot:
it's already being leaked (except for any tests that explicitly run
AtExit..)
1/ I think Grace previously did clarify the global *app* context if fine to
hold and leak. (c.f. activity context should never be leaked)
2/ until jay's change (http://goto.google.com/wjqeu) it was just held as a
raw jobject and AFAICT was  explicitly leaked.

Leaking certainly seems the right answer. (And just holding it as raw
jobject is by far the easiest way to do that)




> Will loop Philippe in this thread since he is actively working on this
> area.
>
>
http://codereview.chromium.**org/10580038/<http://codereview.chromium.org/105...
>

[Johnny(Jianning) Ding, 2012-08-01 18:36:04]

Thanks for the detailed answers. I will complete this CL asap.

On 2012/08/01 18:26:04, joth wrote:
> On 1 August 2012 11:02, <mailto:jnd@chromium.org> wrote:
> 
> > On 2012/08/01 17:46:07, wtc wrote:
> >
> >> jnd: what's the status of this CL?  If you have abandoned
> >> this approach, could you mark the CL closed?  Thanks.
> >>
> >
> > It's still blocked on whether we can leak the Java App Context object in
> > order
> > to run openssl keygen in non-joinable thread with using LazyInstance::Leak.
> >
> > @joth, who should we ask from Android side for above question?
> >
> > 0/ turns out we never run AtExit on android anyway so it's kindof moot:
> it's already being leaked (except for any tests that explicitly run
> AtExit..)
> 1/ I think Grace previously did clarify the global *app* context if fine to
> hold and leak. (c.f. activity context should never be leaked)
> 2/ until jay's change (http://goto.google.com/wjqeu) it was just held as a
> raw jobject and AFAICT was  explicitly leaked.
> 
> Leaking certainly seems the right answer. (And just holding it as raw
> jobject is by far the easiest way to do that)
> 
> 
> 
> 
> > Will loop Philippe in this thread since he is actively working on this
> > area.
> >
> >
>
http://codereview.chromium.**org/10580038/%3Chttp://codereview.chromium.org/1...>
> >

[Johnny(Jianning) Ding, 2012-09-05 14:10:57]

Tested on Chrome Android DEBUG version,  no crash caused by
base::ThreadRestrictions::AssertSingletonAllowed()

On 2012/08/01 18:36:04, Johnny(Jianning) Ding wrote:
> Thanks for the detailed answers. I will complete this CL asap.
> 
> On 2012/08/01 18:26:04, joth wrote:
> > On 1 August 2012 11:02, <mailto:jnd@chromium.org> wrote:
> > 
> > > On 2012/08/01 17:46:07, wtc wrote:
> > >
> > >> jnd: what's the status of this CL?  If you have abandoned
> > >> this approach, could you mark the CL closed?  Thanks.
> > >>
> > >
> > > It's still blocked on whether we can leak the Java App Context object in
> > > order
> > > to run openssl keygen in non-joinable thread with using
LazyInstance::Leak.
> > >
> > > @joth, who should we ask from Android side for above question?
> > >
> > > 0/ turns out we never run AtExit on android anyway so it's kindof moot:
> > it's already being leaked (except for any tests that explicitly run
> > AtExit..)
> > 1/ I think Grace previously did clarify the global *app* context if fine to
> > hold and leak. (c.f. activity context should never be leaked)
> > 2/ until jay's change (http://goto.google.com/wjqeu) it was just held as a
> > raw jobject and AFAICT was  explicitly leaked.
> > 
> > Leaking certainly seems the right answer. (And just holding it as raw
> > jobject is by far the easiest way to do that)
> > 
> > 
> > 
> > 
> > > Will loop Philippe in this thread since he is actively working on this
> > > area.
> > >
> > >
> >
>
http://codereview.chromium.**org/10580038/%253Chttp://codereview.chromium.org...>
> > >

[Ryan Sleevi, 2012-09-05 17:28:59]

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.c...
base/android/jni_android.cc:25: // to leak it.
style nit: Don't use "We" in comments.

Broader comment is this is often pointed out by base/ reviewers (particular
mark). Part of this is about ensuring the language is descriptive, rather than
imperative, as per the Google C++ Guide. The end result of removing "we" often
leads to language that is clearer as to what is guaranteed/expected, whereas
"we" may leave room for a contrasting "they" (eg: "we expect this to be foo" may
lead to a "but THEY expect this to be bar")

I see there are a few other "we"'s in this file, so an extra bonus would be
fixing those few places.

// Leak the global app context, as it is used from a
// non-joinable worker thread that may still be running at
// shutdown. There is no harm in doing this, since [the OS?]
// will clean this up.

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.c...
base/android/jni_android.cc:25: // to leak it.
nit: Within an unnamed namespace, try to preserve the same declaration ordering
as classes.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Declar...

- Typedefs and Enums [Also, types such as classes/structs]
- Constants (static const data members)
- Methods, including static methods

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.c...
base/android/jni_android.cc:307: void CheckException(JNIEnv* env) {
Here's where the comments could be cleaned up.

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:51: // from a non-joinable worker
that is not stopped on shutdown.
comment nit: "we"

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:52: return
Singleton<OpenSSLKeyStoreAndroid,
style nit: In general, templates such as this tend to be indented like method
declaration/definitions, rather than method calls. So if the successive template
arguments cannot be aligned with the first argument, follow the four-space rule.

Singleton<
    OpenSSLKeyStoreAndroid,
    LeakySingletonTraits<OpenSSLKeyStoreAndroid> >::get()

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:53:
LeakySingletonTraits<OpenSSLKeyStoreAndroid> >::get();
nit: It may be helpful to have this as a typedef (around line 17), just to
highlight the Singleton-ness.

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:58: friend struct
DefaultSingletonTraits<OpenSSLKeyStoreAndroid>;
nit: not needed anymore with leaky.

[Johnny(Jianning) Ding, 2012-09-06 10:34:44]

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.c...
base/android/jni_android.cc:25: // to leak it.
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> style nit: Don't use "We" in comments.
> 
> Broader comment is this is often pointed out by base/ reviewers (particular
> mark). Part of this is about ensuring the language is descriptive, rather than
> imperative, as per the Google C++ Guide. The end result of removing "we" often
> leads to language that is clearer as to what is guaranteed/expected, whereas
> "we" may leave room for a contrasting "they" (eg: "we expect this to be foo"
may
> lead to a "but THEY expect this to be bar")
> 
> I see there are a few other "we"'s in this file, so an extra bonus would be
> fixing those few places.
> 
> // Leak the global app context, as it is used from a
> // non-joinable worker thread that may still be running at
> // shutdown. There is no harm in doing this, since [the OS?]
> // will clean this up.

Done.

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.c...
base/android/jni_android.cc:25: // to leak it.
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> style nit: Don't use "We" in comments.
> 
> Broader comment is this is often pointed out by base/ reviewers (particular
> mark). Part of this is about ensuring the language is descriptive, rather than
> imperative, as per the Google C++ Guide. The end result of removing "we" often
> leads to language that is clearer as to what is guaranteed/expected, whereas
> "we" may leave room for a contrasting "they" (eg: "we expect this to be foo"
may
> lead to a "but THEY expect this to be bar")
> 
> I see there are a few other "we"'s in this file, so an extra bonus would be
> fixing those few places.
> 
> // Leak the global app context, as it is used from a
> // non-joinable worker thread that may still be running at
> // shutdown. There is no harm in doing this, since [the OS?]
> // will clean this up.

Done.

http://codereview.chromium.org/10580038/diff/16001/base/android/jni_android.c...
base/android/jni_android.cc:307: void CheckException(JNIEnv* env) {
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> Here's where the comments could be cleaned up.

Done.

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:51: // from a non-joinable worker
that is not stopped on shutdown.
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> comment nit: "we"

Done.

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:52: return
Singleton<OpenSSLKeyStoreAndroid,
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> style nit: In general, templates such as this tend to be indented like method
> declaration/definitions, rather than method calls. So if the successive
template
> arguments cannot be aligned with the first argument, follow the four-space
rule.
> 
> Singleton<
>     OpenSSLKeyStoreAndroid,
>     LeakySingletonTraits<OpenSSLKeyStoreAndroid> >::get()

Done.

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:53:
LeakySingletonTraits<OpenSSLKeyStoreAndroid> >::get();
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> nit: It may be helpful to have this as a typedef (around line 17), just to
> highlight the Singleton-ness.

Done.

http://codereview.chromium.org/10580038/diff/16001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:58: friend struct
DefaultSingletonTraits<OpenSSLKeyStoreAndroid>;
it's still needed since the ctor is in private section.
On 2012/09/05 17:29:00, Ryan Sleevi wrote:
> nit: not needed anymore with leaky.

[Ryan Sleevi, 2012-09-06 18:23:35]

LGTM with a few nits, but no need for re-review.

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
base/android/jni_android.cc:55: base::LazyInstance<MethodIDMap> g_method_id_map
= LAZY_INSTANCE_INITIALIZER;
BUG: This was Leaky, but when you moved it, you removed the Leaky

It seems like this should have the same lifetime as the g_application_context,
for the same reasons.

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
base/android/jni_android.cc:159: // Method clazz.env() can not be used as that
may be from a different thread.
nit: s/Method //

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
base/android/jni_android.cc:314: // Clear the pending exception as it has been
referred.
nit: Clear the pending exception, since a local reference is now held.

http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:21:
OpenSSLKeyStoreAndroidLeakyTraits;
Sorry, this was my bug in my nit. This doesn't need to be a public typedef - it
can be moved between lines 60 and 61 (with a newline before the current line 61)

[Johnny(Jianning) Ding, 2012-09-07 04:33:07]

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.cc
File base/android/jni_android.cc (right):

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
base/android/jni_android.cc:55: base::LazyInstance<MethodIDMap> g_method_id_map
= LAZY_INSTANCE_INITIALIZER;
On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> BUG: This was Leaky, but when you moved it, you removed the Leaky
> 
> It seems like this should have the same lifetime as the g_application_context,
> for the same reasons.

Done.

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
base/android/jni_android.cc:159: // Method clazz.env() can not be used as that
may be from a different thread.
On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> nit: s/Method //

Done.

http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
base/android/jni_android.cc:314: // Clear the pending exception as it has been
referred.
On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> nit: Clear the pending exception, since a local reference is now held.

Done.

http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
File net/base/openssl_private_key_store_android.cc (right):

http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
net/base/openssl_private_key_store_android.cc:21:
OpenSSLKeyStoreAndroidLeakyTraits;
On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> Sorry, this was my bug in my nit. This doesn't need to be a public typedef -
it
> can be moved between lines 60 and 61 (with a newline before the current line
61)

Done.

[Johnny(Jianning) Ding, 2012-09-07 04:36:20]

+jar for base/ owner review.

On 2012/09/07 04:33:07, Johnny(Jianning) Ding wrote:
> http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.cc
> File base/android/jni_android.cc (right):
> 
>
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
> base/android/jni_android.cc:55: base::LazyInstance<MethodIDMap>
g_method_id_map
> = LAZY_INSTANCE_INITIALIZER;
> On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > BUG: This was Leaky, but when you moved it, you removed the Leaky
> > 
> > It seems like this should have the same lifetime as the
g_application_context,
> > for the same reasons.
> 
> Done.
> 
>
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
> base/android/jni_android.cc:159: // Method clazz.env() can not be used as that
> may be from a different thread.
> On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > nit: s/Method //
> 
> Done.
> 
>
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
> base/android/jni_android.cc:314: // Clear the pending exception as it has been
> referred.
> On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > nit: Clear the pending exception, since a local reference is now held.
> 
> Done.
> 
>
http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
> File net/base/openssl_private_key_store_android.cc (right):
> 
>
http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
> net/base/openssl_private_key_store_android.cc:21:
> OpenSSLKeyStoreAndroidLeakyTraits;
> On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > Sorry, this was my bug in my nit. This doesn't need to be a public typedef -
> it
> > can be moved between lines 60 and 61 (with a newline before the current line
> 61)
> 
> Done.

[Johnny(Jianning) Ding, 2012-09-07 12:24:01]

The failure on try bot  'android-test' is not relevant to this patch.

On 2012/09/07 04:36:20, Johnny(Jianning) Ding wrote:
> +jar for base/ owner review.
> 
> On 2012/09/07 04:33:07, Johnny(Jianning) Ding wrote:
> >
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.cc
> > File base/android/jni_android.cc (right):
> > 
> >
>
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
> > base/android/jni_android.cc:55: base::LazyInstance<MethodIDMap>
> g_method_id_map
> > = LAZY_INSTANCE_INITIALIZER;
> > On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > > BUG: This was Leaky, but when you moved it, you removed the Leaky
> > > 
> > > It seems like this should have the same lifetime as the
> g_application_context,
> > > for the same reasons.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
> > base/android/jni_android.cc:159: // Method clazz.env() can not be used as
that
> > may be from a different thread.
> > On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > > nit: s/Method //
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/10580038/diff/23001/base/android/jni_android.c...
> > base/android/jni_android.cc:314: // Clear the pending exception as it has
been
> > referred.
> > On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > > nit: Clear the pending exception, since a local reference is now held.
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
> > File net/base/openssl_private_key_store_android.cc (right):
> > 
> >
>
http://codereview.chromium.org/10580038/diff/23001/net/base/openssl_private_k...
> > net/base/openssl_private_key_store_android.cc:21:
> > OpenSSLKeyStoreAndroidLeakyTraits;
> > On 2012/09/06 18:23:35, Ryan Sleevi wrote:
> > > Sorry, this was my bug in my nit. This doesn't need to be a public typedef
-
> > it
> > > can be moved between lines 60 and 61 (with a newline before the current
line
> > 61)
> > 
> > Done.

[commit-bot: I haz the power, 2012-09-13 13:48:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jnd@chromium.org/10580038/18003

[commit-bot: I haz the power, 2012-09-13 13:48:50]

Presubmit check for 10580038-18003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    base/android

[Johnny(Jianning) Ding, 2012-09-13 13:52:06]

@willchan, @jar, would you please take an owner review?

On 2012/09/13 13:48:50, I haz the power (commit-bot) wrote:
> Presubmit check for 10580038-18003 failed and returned exit status 1.
> 
> 
> Running presubmit commit checks ...
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for files in these directories:
>     base/android

[jar (doing other things), 2012-09-14 02:17:26]

lgtm

[commit-bot: I haz the power, 2012-09-17 03:46:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jnd@chromium.org/10580038/18003

[commit-bot: I haz the power, 2012-09-17 06:15:12]

Change committed as 157083

[Tom Hudson, 2012-09-17 07:17:38]

This is one of two CLs on the blamelist for "browser_tests" on "Linux ChromiumOS
Tests (2)". If the author is around, please check in on IRC or with the
sheriffs?

[Johnny(Jianning) Ding, 2012-09-17 09:03:25]

This CL only affects Android build. The failure should not be caused by it.

On 2012/09/17 07:17:38, Tom Hudson wrote:
> This is one of two CLs on the blamelist for "browser_tests" on "Linux
ChromiumOS
> Tests (2)". If the author is around, please check in on IRC or with the
> sheriffs?