Close leaking FDs.

content/browser/android/sandboxed_process_launcher.cc

Index: content/browser/android/sandboxed_process_launcher.cc
diff --git a/content/browser/android/sandboxed_process_launcher.cc b/content/browser/android/sandboxed_process_launcher.cc
index 04d3512f693b2fbcb615138d464e37493ce412f0..07472a78fa4ed1cfa75e72735d580adc2a70e559 100644
--- a/content/browser/android/sandboxed_process_launcher.cc
+++ b/content/browser/android/sandboxed_process_launcher.cc
@@ -13,7 +13,6 @@
 using base::android::AttachCurrentThread;
 using base::android::ToJavaArrayOfStrings;
 using base::android::ScopedJavaLocalRef;
-using base::GlobalDescriptors;
 using content::StartSandboxedProcessCallback;
 
 namespace content {
@@ -37,8 +36,7 @@ static void OnSandboxedProcessStarted(JNIEnv*,
 
 void StartSandboxedProcess(
     const CommandLine::StringVector& argv,
-    int ipc_fd,
-    const GlobalDescriptors::Mapping& files_to_register,
+    const std::vector<content::FileDescriptorInfo>& files_to_register,
     const StartSandboxedProcessCallback& callback) {
   JNIEnv* env = AttachCurrentThread();
   DCHECK(env);
@@ -46,24 +44,35 @@ void StartSandboxedProcess(
   // Create the Command line String[]
   ScopedJavaLocalRef<jobjectArray> j_argv = ToJavaArrayOfStrings(env, argv);
 
-  ScopedJavaLocalRef<jintArray> j_file_to_register_id_files(env,
-      env->NewIntArray(files_to_register.size() * 2));
-  scoped_array<jint> file_to_register_id_files(
-      new jint[files_to_register.size() * 2]);
-  for (size_t i = 0; i < files_to_register.size(); ++i) {
-    const GlobalDescriptors::KeyFDPair& id_file = files_to_register[i];
-    file_to_register_id_files[2 * i] = id_file.first;
-    file_to_register_id_files[(2 * i) + 1] = id_file.second;
+  size_t file_count = files_to_register.size();

[Yaron, 2012/09/24 18:53:14]

assert > 0 ?

[Jay Civelli, 2012/09/24 20:59:59]

Done.

+
+  ScopedJavaLocalRef<jintArray> j_file_ids(env, env->NewIntArray(file_count));
+  jint* file_ids = env->GetIntArrayElements(j_file_ids.obj(), NULL);
+  ScopedJavaLocalRef<jintArray> j_file_fds(env, env->NewIntArray(file_count));
+  jint* file_fds = env->GetIntArrayElements(j_file_fds.obj(), NULL);
+  ScopedJavaLocalRef<jbooleanArray> j_file_autoclose(
+      env, env->NewBooleanArray(file_count));
+  jboolean* file_autoclose =
+      env->GetBooleanArrayElements(j_file_autoclose.obj(), NULL);
+  base::android::CheckException(env);

[Yaron, 2012/09/24 18:53:14]

You should probably CheckException for the GetIntArrayElement calls too, right?
Those aren't our wrappers and are straight JNI calls.

[Jay Civelli, 2012/09/24 20:59:59]

Good idea, also added some more after allocating.

+  for (size_t i = 0; i < file_count; ++i) {
+    const content::FileDescriptorInfo& fd_info = files_to_register[i];
+    file_ids[i] = fd_info.id;
+    file_fds[i] = fd_info.fd.fd;
+    file_autoclose[i] = fd_info.fd.auto_close;
   }
-  env->SetIntArrayRegion(j_file_to_register_id_files.obj(),
-                         0, files_to_register.size() * 2,
-                         file_to_register_id_files.get());
+  env->ReleaseIntArrayElements(j_file_ids.obj(), file_ids, JNI_ABORT);

[Yaron, 2012/09/24 18:53:14]

So we were previously leaking these local references too?

[Jay Civelli, 2012/09/24 20:59:59]

No, we were using the JNI SetIntArrayRegion method, that does not require any
releasing.
We now use GetIntArrayElements, which does require a release.

+  env->ReleaseIntArrayElements(j_file_fds.obj(), file_fds, JNI_ABORT);
+  env->ReleaseBooleanArrayElements(j_file_autoclose.obj(), file_autoclose,
+                                   JNI_ABORT);
+
   Java_SandboxedProcessLauncher_start(env,
-          base::android::GetApplicationContext(),
-          static_cast<jobjectArray>(j_argv.obj()),
-          static_cast<jint>(ipc_fd),
-          j_file_to_register_id_files.obj(),
-          reinterpret_cast<jint>(new StartSandboxedProcessCallback(callback)));
+      base::android::GetApplicationContext(),
+      j_argv.obj(),
+      j_file_ids.obj(),
+      j_file_fds.obj(),
+      j_file_autoclose.obj(),
+      reinterpret_cast<jint>(new StartSandboxedProcessCallback(callback)));
 }
 
 void StopSandboxedProcess(base::ProcessHandle handle) {