Expect MSI installers

src/engine/Updater.cpp

 #include <functional>
 #include <memory>
 #include <sstream>
 
 #include <Windows.h>
-#include <Msi.h>
 
 #include <AdblockPlus/FileSystem.h>
 #include <AdblockPlus/WebRequest.h>
 
+#include "../shared/AutoHandle.h"
 #include "../shared/Dictionary.h"
 #include "../shared/Utils.h"
 #include "Debug.h"
 #include "Resource.h"
 #include "Updater.h"
 
 namespace
 {
   typedef std::function<void()> ThreadCallbackType;
   typedef std::function<void(HWND)> DialogCallbackType;
@@ skipping 57 matching lines @@
     }
     return FALSE;
   }
 
   DWORD RunThread(LPVOID param)
   {
     std::auto_ptr<ThreadCallbackType> callback(reinterpret_cast<ThreadCallbackType*>(param));
     (*callback)();
     return 0;
   }
+
+  std::wstring EscapeCommandLineArg(const std::wstring& arg)
+  {
+    // This does the inverse of CommandLineToArgvW(). See
+    // http://blogs.msdn.com/b/oldnewthing/archive/2010/09/17/10063629.aspx for
+    // a description of the rules - the backslash rules are very non-obvious.
+    std::wstring result = arg;
+    size_t pos = arg.find(L'"');
+    while (pos != std::wstring::npos)
+    {
+      // Protect the quotation mark
+      result.insert(pos, 1, L'\\');
+      pos++;
+
+      // Protect any of the preceding backslashes
+      for (int offset = -2; pos + offset >= 0 && result[pos + offset] == L'\\'; offset -= 2)
+      {
+        result.insert(pos + offset, 1, L'\\');
+        pos++;
+      }
+
+      // Find next quotation mark
+      pos = arg.find(L'"', pos);
+    }
+    return L'"' + result + L'"';
+  }
+
+  BOOL InstallUpdate(const std::wstring& path)
+  {
+    WCHAR sysDir[MAX_PATH];
+    UINT sysDirLen = GetSystemDirectoryW(sysDir, sizeof(sysDir) / sizeof(sysDir[0]));
+    if (sysDirLen == 0)
+      return false;
+
+    std::wstring msiexec = std::wstring(sysDir, sysDirLen) + L"\\msiexec.exe";
+
+    std::wstring params = L"/i " + EscapeCommandLineArg(path)
+        + L" ACTION=INSTALL INSTALLUILEVEL=2 REINSTALL=ALL"
+          L" REINSTALLMODE=vomus MSIENFORCEUPGRADECOMPONENTRULES=1";
+
+    HINSTANCE instance = ShellExecuteW(NULL, L"runas", msiexec.c_str(), params.c_str(), NULL, SW_HIDE);
+    if (reinterpret_cast<int>(instance) <= 32)
+      return false;
+
+    // As far as we are concerned everything is fine - MSI service will handle
+    // further errors.
+    return true;
+  }
 }
 
 Updater::Updater(AdblockPlus::JsEnginePtr jsEngine, const std::string& url)
     : jsEngine(jsEngine), url(url), tempFile(GetAppDataPath() + L"\\update.msi")
 {
 }
 
 void Updater::Update()
 {
   Debug("Update available: " + url);
 
   if (DialogBox(NULL, MAKEINTRESOURCE(IDD_UPDATEDIALOG), GetDesktopWindow(),
       reinterpret_cast<DLGPROC>(&UpdateDlgProc)) == IDOK)
   {
     Debug("User accepted update");
 
-    {

[Felix Dahlke, 2013/06/11 10:07:52]

I presume the two blocks are to avoid conflicts between the two variables called
"result"? I think it'd be more clear if those were named distinctively then,
e.g. dialogResult and installationResult.

[Wladimir Palant, 2013/06/11 14:42:10]

I would rather have distinct scopes for these two unrelated operations.

-      DialogCallbackType* callback = new DialogCallbackType(std::bind(&Updater::StartDownload,
-          this, std::placeholders::_1));
-      int result = DialogBoxParam(NULL, MAKEINTRESOURCE(IDD_DOWNLOADDIALOG), GetDesktopWindow(),
-          reinterpret_cast<DLGPROC>(&DownloadDlgProc),
-          reinterpret_cast<LPARAM>(callback));
-      if (result == DOWNLOAD_FAILED)
-      {
-        Dictionary* dict = Dictionary::GetInstance();
-        MessageBoxW(NULL,
-            dict->Lookup("updater", "download-error-neterror").c_str(),
-            dict->Lookup("updater", "download-error-title").c_str(),
-            0);
-      }
-      if (result != IDOK)
-        return;
-    }
-
-    {
-      UINT result = ::MsiInstallProductW(tempFile.c_str(), L"ACTION=INSTALL INSTALLUILEVEL=2");

[Oleksandr, 2013/06/10 16:47:56]

Wouldn't it be better just to launch like "msiexec.exe /q /i " +
tempfile.c_str()? We might still want to use an exe installer in future, and
this way will be very similar to the exe installer launch. And that way we will
leave it to Windows Installer to handle all the errors.

[Wladimir Palant, 2013/06/11 04:29:56]

MsiInstallProduct is IMHO a much cleaner solution - e.g. we don't rely on the
PATH variable to be set up correctly, we don't need to do command line escaping
etc. And we get a meaningful error code if something fails.

If we ever need to switch back to executable installers - the code is still
there in the repo history. However, I'm not in favor of doing something just
because "we might need it sometime".

[Felix Dahlke, 2013/06/11 10:07:52]

As long as "update.msi" is hardcoded above we'd need to push out an update to
switch to exe anyway.

[Oleksandr, 2013/06/11 10:24:49]

That's my point. Stuff happens, and maybe for some reason we won't be able to do
a proper update via an msi (for whatever reason). Exe is a plan b for that case,
and it would be nice to support both.

On 2013/06/11 10:07:52, Felix H. Dahlke wrote:

[Felix Dahlke, 2013/06/11 10:28:05]

Couldn't we just ship an exe with the msi and execute it? Guess I could live
with that. If we really do need to move to an exe installer later, we could have
a transitional msi wrapper for older versions.

[Wladimir Palant, 2013/06/11 14:42:10]

I really don't see why we would need a plan b (one that we cannot test
properly). And - yes, pushing out an update to switch to exe installers is
exactly the way to go.

[Felix Dahlke, 2013/06/12 13:03:40]

I'd vote for two functions then :)

-      if (result != ERROR_SUCCESS)
-      {
-        Dictionary* dict = Dictionary::GetInstance();
-        std::wstringstream message;
-        message << dict->Lookup("updater", "download-error-runerror");
-        message << std::endl << L"(error " << result << L")";
-        MessageBoxW(NULL,
-            message.str().c_str(),
-            dict->Lookup("updater", "download-error-title").c_str(),
-            0);
-
-        std::stringstream error;
-        error << "Installing update failed (error " << result << ")";
-        Debug(error.str());
-        return;
-      }
+    DialogCallbackType* callback = new DialogCallbackType(std::bind(&Updater::StartDownload,
+        this, std::placeholders::_1));
+    int result = DialogBoxParam(NULL, MAKEINTRESOURCE(IDD_DOWNLOADDIALOG), GetDesktopWindow(),
+        reinterpret_cast<DLGPROC>(&DownloadDlgProc),
+        reinterpret_cast<LPARAM>(callback));
+    if (result == DOWNLOAD_FAILED)
+    {
+      Dictionary* dict = Dictionary::GetInstance();
+      MessageBoxW(NULL,
+          dict->Lookup("updater", "download-error-neterror").c_str(),
+          dict->Lookup("updater", "download-error-title").c_str(),
+          0);
+    }
+    if (result != IDOK)
+      return;
+
+    if (!InstallUpdate(tempFile))
+    {
+      DebugLastError("Running updater failed");
+
+      Dictionary* dict = Dictionary::GetInstance();
+      MessageBoxW(NULL,
+          dict->Lookup("updater", "download-error-runerror").c_str(),
+          dict->Lookup("updater", "download-error-title").c_str(),
+          0);
     }
   }
 }
 
 void Updater::StartDownload(HWND dialog)
 {
   this->dialog = dialog;
   ThreadCallbackType* callback = new ThreadCallbackType(std::bind(&Updater::RunDownload, this));
   ::CreateThread(NULL, 0, reinterpret_cast<LPTHREAD_START_ROUTINE>(&RunThread),
       callback, 0, NULL);
@@ skipping 32 matching lines @@
   }
   catch (const std::exception& e)
   {
     DebugException(e);
     EndDialog(dialog, DOWNLOAD_FAILED);
     return;
   }
 
   EndDialog(dialog, IDOK);
 }