Expect MSI installers

src/engine/Updater.cpp

Index: src/engine/Updater.cpp
===================================================================
--- a/src/engine/Updater.cpp
+++ b/src/engine/Updater.cpp
@@ -1,11 +1,15 @@
 #include <functional>
 #include <memory>
 #include <sstream>
+
+#include <Windows.h>
+#include <Msi.h>
+
 #include <AdblockPlus/FileSystem.h>
 #include <AdblockPlus/WebRequest.h>
 
 #include "../shared/Dictionary.h"
 #include "../shared/Utils.h"
 #include "Debug.h"
 #include "Resource.h"
 #include "Updater.h"
@@ -79,63 +83,66 @@ namespace
   {
     std::auto_ptr<ThreadCallbackType> callback(reinterpret_cast<ThreadCallbackType*>(param));
     (*callback)();
     return 0;
   }
 }
 
 Updater::Updater(AdblockPlus::JsEnginePtr jsEngine, const std::string& url)
-    : jsEngine(jsEngine), url(url), tempFile(GetAppDataPath() + L"\\update.exe")
+    : 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");
 
-    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)
     {

[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.

-      Dictionary* dict = Dictionary::GetInstance();
-      MessageBoxW(NULL,
-          dict->Lookup("updater", "download-error-neterror").c_str(),
-          dict->Lookup("updater", "download-error-title").c_str(),
-          0);
+      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 (result != IDOK)
-      return;
 
-    PROCESS_INFORMATION pi;
-    STARTUPINFO si;
-    ::ZeroMemory(&si, sizeof(si));
-    si.cb = sizeof(si);
-    si.wShowWindow = FALSE;
+    {
+      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);
 
-    if (!::CreateProcessW(tempFile.c_str(), NULL, NULL, NULL, FALSE, CREATE_BREAKAWAY_FROM_JOB, NULL, NULL, &si, &pi))
-    {
-      Dictionary* dict = Dictionary::GetInstance();
-      MessageBoxW(NULL,
-          dict->Lookup("updater", "download-error-runerror").c_str(),
-          dict->Lookup("updater", "download-error-title").c_str(),
-          0);
-      DebugLastError("Creating updater process failed");
-      return;
+        std::stringstream error;
+        error << "Installing update failed (error " << result << ")";
+        Debug(error.str());
+        return;
+      }
     }
-    ::CloseHandle(pi.hProcess);
-    ::CloseHandle(pi.hThread);
   }
 }
 
 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),