Tooltip notification. Check for update fixes.

src/engine/Main.cpp

Index: src/engine/Main.cpp
===================================================================
--- a/src/engine/Main.cpp
+++ b/src/engine/Main.cpp
@@ -12,12 +12,15 @@
 #include "Debug.h"
 #include "Updater.h"
 

[Felix Dahlke, 2013/09/11 15:22:59]

Super nit, but I think a single empty line between the includes and the
namespace suffices.

+
+
 namespace
 {
   std::auto_ptr<AdblockPlus::FilterEngine> filterEngine;
   std::auto_ptr<Updater> updater;
   int activeConnections = 0;
   CriticalSection activeConnectionsLock;
+  HWND callbackWindow;
 
   void WriteStrings(Communication::OutputBuffer& response,
       const std::vector<std::string>& strings)
@@ -44,24 +47,35 @@
   }
 
   bool updateAvailable;
+  volatile bool checkingForUpdate = false;

[Felix Dahlke, 2013/09/11 15:22:59]

Why use the volatile keyword here? I doubt it's what we want.

   void UpdateCallback(const std::string res)
   {
+    checkingForUpdate = false;

[Wladimir Palant, 2013/09/11 13:07:06]

This needs to be set when the function is finished. As it is now this is a race
condition - a new PROC_CHECK_FOR_UPDATES call might come in on a different
thread after you set checkingForUpdate to false and the code below will send the
response to the wrong thread.

IMHO this function should now go like this:

UINT message;
if (updateAvailable)
  message = WM_DOWNLOADING_UPDATE
else if (res.length() == 0)
  message = ...;
else
  message = ...;

if (callbackWindow)
{
  SendMessage(callbackWindow, message, 0, 0);
  callbackWindow = 0;
}

This will also give you a single exit point where you can reset the
checkingForUpdate flag.

     if (updateAvailable)
+    {
+      if (callbackWindow != 0)
+      {
+        SendMessage(callbackWindow, WM_DOWNLOADING_UPDATE, 0, 0);
+        callbackWindow = 0;
+      }
       return;
+    }
     Dictionary* dictionary = Dictionary::GetInstance();

[Wladimir Palant, 2013/09/11 13:07:06]

The dictionary is no longer being used.

     if (res.length() == 0)
     {
-      std::wstring upToDateText = dictionary->Lookup("updater", "update-already-up-to-date-text");
-      std::wstring upToDateTitle = dictionary->Lookup("updater", "update-already-up-to-date-title");
-      MessageBoxW(NULL, upToDateText.c_str(), upToDateTitle.c_str(), MB_OK);
+      if (callbackWindow != 0)
+      {
+        SendMessage(callbackWindow, WM_ALREADY_UP_TO_DATE, 0, 0);
+      }
     }
     else
     {
-      std::wstring errorText = dictionary->Lookup("updater", "update-error-text");
-      std::wstring errorTitle = dictionary->Lookup("updater", "update-error-title");
-      ReplaceString(errorText, L"?1?", ToUtf16String(res));
-      MessageBoxW(NULL, errorText.c_str(), errorTitle.c_str(), MB_OK);
+      if (callbackWindow != 0)
+      {
+        SendMessage(callbackWindow, WM_UPDATE_CHECK_ERROR, 0, 0);
+      }
     }
+    callbackWindow = 0;
     return;
   }
 
@@ -243,8 +257,13 @@
       }
       case Communication::PROC_CHECK_FOR_UPDATES:
       {
+        request >> (int32_t&)callbackWindow;
         updateAvailable = false;

[Wladimir Palant, 2013/09/11 13:07:06]

Setting updateAvailable definitely belongs into the |if (!checkingForUpdate)|
block.

Not sure whether replacing the callbackWindow variable makes sense if the update
check is triggered twice - the client simply shouldn't open two windows. Using
static_cast<int32_t>() is certainly preferable here however.

-        filterEngine->ForceUpdateCheck(UpdateCallback);
+        if (!checkingForUpdate)
+        {
+          checkingForUpdate = true;

[Wladimir Palant, 2013/09/11 13:07:06]

This is a race condition - another PROC_CHECK_FOR_UPDATES request can come in on
a different thread after you checked checkingForUpdate but before you've set it
to true. This will still result in two concurrent update checks.

Access to checkingForUpdate needs to be protected with a lock, using "volatile"
keyword won't do.

+          filterEngine->ForceUpdateCheck(UpdateCallback);
+        }
         break;
       }
       case Communication::PROC_IS_FIRST_RUN_ACTION_NEEDED: