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

installer/src/installer-lib/process.cpp

Index: installer/src/installer-lib/process.cpp
===================================================================
--- a/installer/src/installer-lib/process.cpp
+++ b/installer/src/installer-lib/process.cpp
@@ -9,25 +9,32 @@
 
 //-------------------------------------------------------
 //-------------------------------------------------------
-bool process_by_any_exe_with_any_module::operator()( const PROCESSENTRY32W & process )
+typedef int (__stdcall *IsImmersiveDynamicFunc)(HANDLE);
+bool process_by_any_exe_not_immersive::operator()( const PROCESSENTRY32W & process )
 {
   if (processNames.find(process.szExeFile) != processNames.end())
-  {
-    if (moduleNames.empty())
-      return true;
+  {   
+    // Make sure the process is still alive
+    HANDLE procHandle = OpenProcess(PROCESS_QUERY_INFORMATION, FALSE, process.th32ProcessID);
+    if ((procHandle == NULL) || (procHandle == INVALID_HANDLE_VALUE)) return false;
 
-    Module_Snapshot ms(process.th32ProcessID);
-    const MODULEENTRY32W* me = ms.first();
-    while (me != 0)
-    {
-      if (moduleNames.find(me->szModule) != moduleNames.end())
-      {
-        return true;
-      }
-      me = ms.next();
-    }
+    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:

+    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;
   }
-  return false;
 }
 
 //-------------------------------------------------------
@@ -249,7 +256,7 @@
 
         if ( acc.permit_end_session )
         {
-          send_message m2( WM_ENDSESSION, 0, 0 ) ;
+          send_message m2( WM_ENDSESSION, 0, ENDSESSION_CLOSEAPP ) ;
           iterate_our_windows( m2 ) ;
         }
       }