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 43 matching lines @@
         /*
         * Send WM_CLOSE to every admissible window.
         */
         send_message m( WM_CLOSE, 0, 0 ) ;
         iterate_our_windows( m ) ;
       }
       break ;
 
     case 4:
       /*
-      * We're out of ways to try to shut down. Oh well. Take cover. It gets violent here.

[Eric, 2015/03/20 08:53:16]

Should restore the first half of the comment to its original location.

+      * 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) continue;
+        if (!tmpHandle) 
+        {
+          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;
+        }
         Windows_Handle procHandle(tmpHandle);
-        TerminateProcess(tmpHandle, 0);

[Eric, 2015/03/20 08:53:16]

Given how unreliable it's turning out to be to shut down IE, it would be a good
idea to log the error code associated with a failure of TerminateProcess() here.
That would mean changing the signature of ShutDown() to pass a log instance. Had
I realized what trouble this would be, I would have done that the first time.

The error that would most useful to log here would be absence of the
PROCESS_TERMINATE access right. In practice, that would require running an MSI
as administrator, but if that's the trick to make it work, there's no
information coming out of a failure here that would even provide a clue toward
that.

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

Merely want to add that MSI is already configured to run with elevated
privileges and running MSI as Administrator still could be not enough to
terminate the process.

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

As well as it would be good to log the state to better understand which part
failed to shut down IE.

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

The process 'msiexec.exe' runs with elevated privileges. Custom actions, such as
the present code, run in a separate container process and do not necessarily
have the same privileges.

There are ways of doing that, but we're not using them. Given that the whole
purpose of this CA is to avoid an annoyance (rebooting), it's not clear to me
that forcibly closing IE is worth it; the cure (side-effects of premature
termination of IE) may be worse than the disease (the annoyance of rebooting).

+        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 ) ) ;
       Refresh() ;
       if ( ! IsRunning() )
       {
         return true ;
       }
     }
     // Assert is_running()
   }
   // No control path leaves the for-loop.
 } ;