Issue #1686 - Kill iexplore.exe and AdblockplusEngine.exe processes from the installer

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"
@@ skipping 198 matching lines @@
 * The present use of this method is not closing dozens of applications, so delay performance is not critical.
 *
 * \return
 *   The negation of is_running.
 *   If is_running() was true at the beginning, then this function will have run refresh() before returning.
 *
 * \sa
 *   - MSDN [WM_QUERYENDSESSION message](http://msdn.microsoft.com/en-us/library/windows/desktop/aa376890%28v=vs.85%29.aspx)
 *   - MSDN [WM_ENDSESSION message](http://msdn.microsoft.com/en-us/library/windows/desktop/aa376889%28v=vs.85%29.aspx)
 */
-bool ProcessCloser::ShutDown()
+bool ProcessCloser::ShutDown(ImmediateSession& session)
 {
   /*
   * If we're not running, we don't need to shut down.
   */
   if ( ! IsRunning() )
   {
     return true ;
   }
 
   /*
@@ skipping 41 matching lines @@
     case 3 :
       {
         /*
         * Send WM_CLOSE to every admissible window.
         */
         send_message m( WM_CLOSE, 0, 0 ) ;
         iterate_our_windows( m ) ;
       }
       break ;
 
-    default :

[Eric, 2015/03/13 16:49:48]

This should be "case 4 :" and we should retain the previous "default:". The
reason is that we want to execute the wait loop below to give the system some
time to finish terminating the process.

We don't want a false negative because of a zombie process that exists longer
than normal.

+    case 4:
       /*
-      * We're out of ways to try to shut down. Oh well. Take cover. It gets violent here.

[Eric, 2015/03/13 16:49:48]

Good comment.

+      * Oh well. Take cover. It gets violent here. Try to kill all matching processes.
       */
       for (auto it = pid_set.begin(); it != pid_set.end(); ++it)
       {
         HANDLE tmpHandle = OpenProcess(PROCESS_TERMINATE, FALSE, *it);
-        if (tmpHandle != NULL)

[Eric, 2015/03/13 16:49:48]

We can use implicit conversion to bool here.

I would use "if (!tmpHandle) continue;", but that's just me.

+        if (!tmpHandle) 
         {
-          Windows_Handle procHandle(tmpHandle);
-          TerminateProcess(tmpHandle, 0);
+          std::ostringstream stream;
+          stream << "Can't open process for termination. Error: " << GetLastError();

[Eric, 2015/05/14 15:09:25]

This message should indicate a warning rather than an error. This avoids
confusing reporting with respect to a race condition.

The issue is that we are working from a process snapshot in the past, not the
perfectly-current process set. During the interval between then and now, the IE
process may have closed itself down as previously instructed. As a result, a
failure of 'OpenProcess' does not in itself reflect whether we have a process
that terminated itself and is now gone (not an error, just the effect of the
race condition), and a hard error from a process that's still present that we
can't open.

It would be possible to add more checks to distinguish these two, but I'm not
sure it's worth it.

A suggestion:
"Did not open a process scheduled for termination; it may or may not still
exist. OpenProcess() error code = "

+          session.Log(stream.str());
+          continue;
         }
-      }
-      Refresh();
-
-      return !IsRunning() ;
+        Windows_Handle procHandle(tmpHandle);
+        if (!TerminateProcess(tmpHandle, 0))
+        {
+          std::ostringstream stream;
+          stream << "Can't terminate process. Error: " << GetLastError();
+          session.Log(stream.str());
+        }
+      }
+      break;
+
+    default:
+      // We're out of ways to try to shut down. 
+      return false;
     }
 
     /*
     * Wait loop.
     */
     for ( unsigned int j = 0 ; j < 50 ; ++ j )
     {
       std::this_thread::sleep_for( std::chrono::milliseconds( 30 ) ) ;

[sergei, 2015/04/02 08:04:09]

no spaces?

       Refresh() ;
       if ( ! IsRunning() )
       {
         return true ;
       }
     }
     // Assert is_running()
   }
   // No control path leaves the for-loop.
 } ;