Make sure IE and Engine are actually being closed by custom action

installer/src/installer-lib/process.cpp

 #include <stdexcept>
 #include <functional>
 #include <wctype.h>
 // <thread> is C++11, but implemented in VS2012
 #include <thread>
 
 #include "installer-lib.h"
 #include "process.h"
+#include "handle.h"
+#include "session.h"
 
 //-------------------------------------------------------
 //-------------------------------------------------------
 typedef int (__stdcall *IsImmersiveDynamicFunc)(HANDLE);
 bool process_by_any_exe_not_immersive::operator()( const PROCESSENTRY32W & process )
 {
   if (processNames.find(process.szExeFile) != processNames.end())
   {   
     // Make sure the process is still alive
-    HANDLE procHandle = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, process.th32ProcessID);
+    Windows_Handle procHandle = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, process.th32ProcessID);
     if ((procHandle == NULL) || (procHandle == INVALID_HANDLE_VALUE)) return false;
 
     DWORD exitCode;
     if (!GetExitCodeProcess(procHandle, &exitCode)) return false;
 
     if (exitCode != STILL_ACTIVE) return false;
 
     // Check if this is a Windows Store app process (we don't care for IE in Modern UI)
-    HINSTANCE user32Dll = LoadLibrary(L"user32.dll");

[Eric, 2014/06/25 15:08:51]

What's the reason for a dynamic load of this library? It's one of the oldest in
Windows. It's almost certainly already loaded. GetModuleHandle() would seem to
be more appropriate.

[Oleksandr, 2014/06/26 13:25:57]

Why make assumptions? From MSDN
http://msdn.microsoft.com/en-us/library/windows/desktop/ms684175(v=vs.85).aspx:
"When no path is specified, the function searches for loaded modules whose base
name matches the base name of the module to be loaded. If the name matches, the
load succeeds."

On 2014/06/25 15:08:51, Eric wrote:

+    Windows_Module_Handle user32Dll(LoadLibrary(L"user32.dll"));
     if (!user32Dll) return true;
 
     IsImmersiveDynamicFunc IsImmersiveDynamicCall = (IsImmersiveDynamicFunc)GetProcAddress(user32Dll, "IsImmersiveProcess");
     if (!IsImmersiveDynamicCall) return true;
 
     BOOL retValue = !IsImmersiveDynamicCall(procHandle);

[Eric, 2014/06/25 15:08:51]

IsImmersiveDynamicCall commingles a false result with an error condition; both
return zero. As a rule, I'd expect to see a call to GetLastError(). In this
particular case, I'm not sure how much it matters. I think I'd pick a policy to
mask the error (what's already happening) but also log it to the installer log
file.

[Oleksandr, 2014/06/26 13:25:57]

I agree it wouldn't hurt to log the GetLastError here, but since we don't have a
session handle here, adding that log functionality will really just clutter a
lot of our code for no particular reason. I don't see when we could use that
error code anyway.

On 2014/06/25 15:08:51, Eric wrote:

-    CloseHandle(procHandle);

[Eric, 2014/06/25 15:08:51]

Resource Leak. CloseHandle() won't be called under several conditions: exitCode
== STILL_ACTIVE, IsImmersiveDynamicCall == 0, etc. A simple wrapper class that
puts OpenProcess(...) in the constructor and CloseHandle in the destructor would
fix this and leave the body of this function clean.

 
     return retValue;
   }
 }
 
 //-------------------------------------------------------
 // creator_process
 //-------------------------------------------------------
 DWORD creator_process( HWND window )
 {
@@ skipping 254 matching lines @@
       if ( ! is_running() )
       {
         return true ;
       }
     }
     // Assert is_running()
   }
   // No control path leaves the for-loop.
 } ;