Refactor NaClProcessHost. Reduce chances to leak the resource.

components/nacl/renderer/ppb_nacl_private_impl.cc

 // Copyright 2013 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 "components/nacl/renderer/ppb_nacl_private_impl.h"
 
 #include <numeric>
 #include <string>
 #include <vector>
 
@@ skipping 367 matching lines @@
                   void* imc_handle,
                   PP_CompletionCallback callback) {
   CHECK(ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->
             BelongsToCurrentThread());
   NaClAppProcessType process_type = PP_ToNaClAppProcessType(pp_process_type);
   // Create the manifest service proxy here, so on error case, it will be
   // destructed (without passing it to ManifestServiceChannel).
   scoped_ptr<ManifestServiceChannel::Delegate> manifest_service_proxy(
       new ManifestServiceProxy(instance, process_type));
 
-  FileDescriptor result_socket;
   IPC::Sender* sender = content::RenderThread::Get();
   DCHECK(sender);
   int routing_id = GetRoutingID(instance);
   NexeLoadManager* load_manager = GetNexeLoadManager(instance);
   DCHECK(load_manager);
   content::PepperPluginInstance* plugin_instance =
       content::PepperPluginInstance::Get(instance);
   DCHECK(plugin_instance);
   if (!routing_id || !load_manager || !plugin_instance) {
     if (nexe_file_info->handle != PP_kInvalidFileHandle) {
       base::File closer(nexe_file_info->handle);
     }
     ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
         FROM_HERE, base::Bind(callback.func, callback.user_data,
                               static_cast<int32_t>(PP_ERROR_FAILED)));
     return;
   }
 
   InstanceInfo instance_info;
   instance_info.url = GURL(alleged_url);
 
   uint32_t perm_bits = ppapi::PERMISSION_NONE;
   // Conditionally block 'Dev' interfaces. We do this for the NaCl process, so
   // it's clearer to developers when they are using 'Dev' inappropriately. We
   // must also check on the trusted side of the proxy.
   if (load_manager->DevInterfacesEnabled())
     perm_bits |= ppapi::PERMISSION_DEV;
   instance_info.permissions =
       ppapi::PpapiPermissions::GetForCommandLine(perm_bits);
-  std::string error_message_string;
-  NaClLaunchResult launch_result;
-
-  IPC::PlatformFileForTransit nexe_for_transit =
-      IPC::InvalidPlatformFileForTransit();
 
   std::vector<NaClResourcePrefetchRequest> resource_prefetch_request_list;
   if (process_type == kNativeNaClProcessType && uses_nonsfi_mode) {
     JsonManifest* manifest = GetJsonManifest(instance);
     if (manifest) {
       manifest->GetPrefetchableFiles(&resource_prefetch_request_list);
 
       for (size_t i = 0; i < resource_prefetch_request_list.size(); ++i) {
         const GURL gurl(resource_prefetch_request_list[i].resource_url);
         // Important security check. Do not remove.
         if (!CanOpenViaFastPath(plugin_instance, gurl)) {
           resource_prefetch_request_list.clear();
           break;
         }
       }
     }
   }
 
+  IPC::PlatformFileForTransit nexe_for_transit =
+      IPC::InvalidPlatformFileForTransit();
 #if defined(OS_POSIX)
   if (nexe_file_info->handle != PP_kInvalidFileHandle)
     nexe_for_transit = base::FileDescriptor(nexe_file_info->handle, true);
 #elif defined(OS_WIN)
   // Duplicate the handle on the browser side instead of the renderer.
   // This is because BrokerGetFileForProcess isn't part of content/public, and
   // it's simpler to do the duplication in the browser anyway.
   nexe_for_transit = nexe_file_info->handle;
 #else
-#error Unsupported target platform.
+# error Unsupported target platform.
 #endif
+
+  std::string error_message_string;
+  NaClLaunchResult launch_result;
   if (!sender->Send(new NaClHostMsg_LaunchNaCl(
           NaClLaunchParams(
               instance_info.url.spec(),
               nexe_for_transit,
               nexe_file_info->token_lo,
               nexe_file_info->token_hi,
               resource_prefetch_request_list,
               routing_id,
               perm_bits,
               PP_ToBool(uses_nonsfi_mode),
               process_type),
           &launch_result,
           &error_message_string))) {
     ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
         FROM_HERE,
         base::Bind(callback.func, callback.user_data,
                    static_cast<int32_t>(PP_ERROR_FAILED)));
     return;
   }
 
   load_manager->set_nonsfi(PP_ToBool(uses_nonsfi_mode));
 
   if (!error_message_string.empty()) {
+    // Even on error, some resources may be passed to here.

[Mark Seaborn, 2015/05/06 21:24:53]

"resources" -> "FDs/handles" to be more specific?

[hidehiko, 2015/05/07 01:29:07]

Done.

+    // We must release those resources.
+    // See also nacl_process_host.

[Mark Seaborn, 2015/05/06 21:24:53]

Nit: "nacl_process_host.cc" (might as well add .cc for completeness)

[hidehiko, 2015/05/07 01:29:07]

Done.

+    IPC::PlatformFileForTransitToFile(launch_result.imc_channel_handle);
+    base::SharedMemory::CloseHandle(launch_result.crash_info_shmem_handle);
+
     if (PP_ToBool(main_service_runtime)) {
       load_manager->ReportLoadError(PP_NACL_ERROR_SEL_LDR_LAUNCH,
                                     "ServiceRuntime: failed to start",
                                     error_message_string);
     }
     ppapi::PpapiGlobals::Get()->GetMainThreadMessageLoop()->PostTask(
         FROM_HERE,
         base::Bind(callback.func, callback.user_data,
                    static_cast<int32_t>(PP_ERROR_FAILED)));
     return;
   }
-  result_socket = launch_result.imc_channel_handle;
+
   instance_info.channel_handle = launch_result.ppapi_ipc_channel_handle;
   instance_info.plugin_pid = launch_result.plugin_pid;
   instance_info.plugin_child_id = launch_result.plugin_child_id;
 
   // Don't save instance_info if channel handle is invalid.
   if (IsValidChannelHandle(instance_info.channel_handle)) {
     NaClPluginInstance* nacl_plugin_instance = GetNaClPluginInstance(instance);
     nacl_plugin_instance->instance_info.reset(new InstanceInfo(instance_info));
   }
 
-  *(static_cast<NaClHandle*>(imc_handle)) = ToNativeHandle(result_socket);
+  *(static_cast<NaClHandle*>(imc_handle)) =
+      IPC::PlatformFileForTransitToPlatformFile(
+          launch_result.imc_channel_handle);
 
   // Store the crash information shared memory handle.
   load_manager->set_crash_info_shmem_handle(
       launch_result.crash_info_shmem_handle);
 
   // Create the trusted plugin channel.
   if (IsValidChannelHandle(launch_result.trusted_ipc_channel_handle)) {
     bool is_helper_nexe = !PP_ToBool(main_service_runtime);
     scoped_ptr<TrustedPluginChannel> trusted_plugin_channel(
         new TrustedPluginChannel(
@@ skipping 1201 matching lines @@
   &StreamPexe
 };
 
 }  // namespace
 
 const PPB_NaCl_Private* GetNaClPrivateInterface() {
   return &nacl_interface;
 }
 
 }  // namespace nacl