Allow OpenSSLPrivateKeyStore and Android global app context leaking.

base/android/jni_android.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/android/jni_android.h"
 
 #include <map>
 
 #include "base/android/build_info.h"
 #include "base/android/jni_string.h"
 #include "base/atomicops.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/threading/platform_thread.h"
 
 namespace {
 using base::android::GetClass;
 using base::android::GetMethodID;
 using base::android::ScopedJavaLocalRef;
 
 JavaVM* g_jvm = NULL;
 
-base::LazyInstance<base::android::ScopedJavaGlobalRef<jobject> >
+// We allow the global app context to be leaked, because it is used from a
+// non-joinable worker that is not stopped on shutdown and there is no harm
+// to leak it.

[Ryan Sleevi, 2012/09/05 17:29:00]

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.

[Ryan Sleevi, 2012/09/05 17:29:00]

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

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

Done.

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

Done.

+base::LazyInstance<base::android::ScopedJavaGlobalRef<jobject> >::Leaky
     g_application_context = LAZY_INSTANCE_INITIALIZER;
 
 struct MethodIdentifier {
   const char* class_name;
   const char* method;
   const char* jni_signature;
 
   bool operator<(const MethodIdentifier& other) const {
     int r = strcmp(class_name, other.class_name);
     if (r < 0) {
@@ skipping 260 matching lines @@
 }
 
 bool ClearException(JNIEnv* env) {
   if (!HasException(env))
     return false;
   env->ExceptionDescribe();
   env->ExceptionClear();
   return true;
 }
 
 void CheckException(JNIEnv* env) {

[Ryan Sleevi, 2012/09/05 17:29:00]

Here's where the comments could be cleaned up.

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

Done.

   if (!HasException(env)) return;
 
   // Ugh, we are going to die, might as well tell breakpad about it.
   jthrowable java_throwable = env->ExceptionOccurred();
   if (!java_throwable) {
     // Nothing we can do.
     CHECK(false);
   }
 
   // Clear the pending exception, we do have a reference to it.
   env->ExceptionClear();
 
   // Set the exception_string in BuildInfo so that breakpad can read it.
   // RVO should avoid any extra copies of the exception string.
   base::android::BuildInfo::GetInstance()->set_java_exception_info(
       GetJavaExceptionInfo(env, java_throwable));
 
   // Now, feel good about it and die.
   CHECK(false);
 }
 
 }  // namespace android
 }  // namespace base