Issue 1109 - Support notifications

src/engine/Main.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #pragma comment(linker,"\"/manifestdependency:type='win32' \
   name='Microsoft.Windows.Common-Controls' version='6.0.0.0' \
   processorArchitecture='*' publicKeyToken='6595b64144ccf1df' language='*'\"")

[Eric, 2015/08/19 17:43:37]

I did not say it wasn't available, I said it wasn't shipped by default. That's
not the same. It doesn't really matter that it's available from somewhere if
it's not present on disk when the BHO tries to load.

If you want to make it a requirement, you need to either (1) check for it in the
installer and fail out if it's not there, or (2) check for it in the installer
and install it if it's not present.

[Oleksandr, 2015/08/20 13:12:13]

Common Controls 6 are essentially Windows XP UI upgrades. They are by definition
included in Windows XP. A quick Google Search gave me this:
http://blogs.msdn.com/b/oldnewthing/archive/2008/01/29/7294949.aspx
http://www.geoffchappell.com/studies/windows/shell/comctl32/history/

On XP they are side-by-side with Common Controls 5. Which is why we need this
#pragma too.

 
 #include <AdblockPlus.h>
 #include <functional>
 #include <vector>
+#include <deque>
 #include <thread>
+#include <mutex>
+#include <condition_variable>
 #include <Windows.h>
 
 #include "../shared/AutoHandle.h"
 #include "../shared/Communication.h"
 #include "../shared/Dictionary.h"
 #include "../shared/Utils.h"
 #include "../shared/Version.h"
 #include "../shared/CriticalSection.h"
 #include "IeVersion.h"
 #include "AdblockPlus.h"
 #include "Debug.h"
 #include "Updater.h"
+#include "Registry.h"
 #include "NotificationWindow.h"
 
 namespace
 {
-  class MyModule : public ATL::CAtlExeModuleT<MyModule> {
+  struct ScopedAtlAxInitializer
+  {
+    ScopedAtlAxInitializer()
+    {
+      ATL::AtlAxWinInit();
+    }
+    ~ScopedAtlAxInitializer()
+    {
+      ATL::AtlAxWinTerm();
+    }
+  };
+
+  class ABPAtlModule : public ATL::CAtlExeModuleT<ABPAtlModule>
+  {
+    enum CustomMessages
+    {
+      TASK_POSTED = WM_USER + 1
+    };
+
   public:
+    ABPAtlModule() : m_msgHWnd(nullptr)
+    {
+    }
+    void Finalize();
+    HRESULT PreMessageLoop(int showCmd) throw();
+    void RunMessageLoop() throw();

[Eric, 2015/08/19 17:43:37]

The only thing this message loop is doing is to pump the notification queue.
There are other ways of doing this that are less problematic.

[sergei, 2015/08/20 14:45:42]

What does the notification queue refers here?
Right now it's used to dispatch messages to notification window and for
synchronization between threads. I personally would like to use it also for
update stuff (window and make asynchronous downloading), for possible messages
from uninstaller (installer) custom action as well as we could utilize it for
libadblockplus to get rid of those free-running threads (it should normalize the
lifetime of objects and simplify the design) and may be for pipes.
What are the other ways and what is especially problematic here?

+    static HRESULT InitializeCom() throw()
+    {
+      // The default implementation initializes multithreaded version but
+      // in this case hosted ActiveX does not properly work.
+      return CoInitialize(nullptr);
+    }
+  private:
+    void onNewNotification(const AdblockPlus::NotificationPtr& notification);
+    void DispatchTask(std::function<void()>&& task);
+    void ProcessTasks();
+
+    ScopedAtlAxInitializer m_scopedAtlAxInit;
+    HWND m_msgHWnd;
+    std::recursive_mutex m_tasksMutex;
+    std::deque<std::function<void()>> m_tasks;
+    std::unique_ptr<NotificationBorderWindow> m_notificationWindow;
   } _AtlModule;

[Eric, 2015/08/19 17:43:37]

'_AtlModule' is defined statically, yet its proper functioning depends on
resources that this class doesn't control, notably the filter engine (containing
v8). The filter engine is initialized dynamically (in 'WinMain') but defined
statically as an 'auto_ptr'. This combination isn't good. There are three basic
approaches to fixing it.

1) Treat the filter engine as a global. This would mean changing the
initialization to static initialization. We'd have to deal with exceptions, but
we need to do that anyway, since the current dynamic initialization can throw a
C++ exception right through the entry point 'WinMain'. Then as long as static
variable 'filterEngine' is declared (and initialized) _before_ this object, it
will be constructed earlier and destroyed later, ensuring that v8 is present
whenever this object is present.

2) Treat the filter engine as a local. In this case, the culprit is the way that
the fact that member variables 'm_tasks' and 'm_notificationWindow' don't
contain references to the resources they need. The reference would be in the
form of a 'shared_ptr<AdblockPlus::FilterEngine>', which would mean needing to
change the declaration of 'filterEngine'. In the case of the tasks, that would
mean declaring the lambda expression with a capture of such a reference. In the
case of the notification window, the reference would be part of the constructor.
This all probably means making a factory for the filter engine.

3) Stop trying to overload this object to both pump the notification queue and
manage the life cycle of the notification window (which could be done in a task
object). There's no solid reason to place both of these responsibilities here.

[Oleksandr, 2015/08/20 13:12:13]

It's not perfect right now, but good enough, IMHO. It isn't an actual bug, just
an architectural change. Since this code has already been pushed (sorry, we just
want to make a devbuild with it), I suggest just creating an issue for this, if
you think it is important.

[sergei, 2015/08/20 14:45:41]

I faced with the troubles that it's static and yet another global variable as
well as with troubles related to uncontrolled _filterEngine and JsEngine and
decided to defer the refactoring so far, only for the sake of smaller amount of
the changes and of simplifying of this review.

 
   std::auto_ptr<AdblockPlus::FilterEngine> filterEngine;
   std::auto_ptr<Updater> updater;
   int activeConnections = 0;
   CriticalSection activeConnectionsLock;
   HWND callbackWindow;
 
   void WriteSubscriptions(Communication::OutputBuffer& response,
       const std::vector<AdblockPlus::SubscriptionPtr>& subscriptions)
   {
@@ skipping 358 matching lines @@
 
     Debug("Client disconnected " + threadString);
 
     {
       CriticalSection::Lock lock(activeConnectionsLock);
       activeConnections--;
       if (activeConnections < 1)
       {
         Debug("No connections left, shutting down the engine");
         activeConnections = 0;
+
+        // The following exit(0) calls the destructor of _AtlModule from the
+        // current thread which results in the disaster because there is a
+        // running message loop as well as there can be alive notification
+        // window which holds v8::Value as well as m_tasks can hold v8::Value
+        // but JS Engine is destroyed before _AtlModule. BTW, various free
+        // running threads like Timeout also cause the crash because the engine
+        // is already destroyed.

[Eric, 2015/08/19 17:43:38]

If it's possible for a dependency of a class to be destroyed while that class
still depends upon it, it means the class design is broken.

The very fact that this comment is necessary, much less the 'Finalize' function
itself, should have been evidence enough that this code needed attention.

[sergei, 2015/08/20 14:45:41]

Yes, this code needs attention. I would like to note that, firstly, the design
of using `exit` and global variables is broken, secondly, the design of lifetime
of `AdblockPlus::JsEngine` and using of threads in libadblockplus is broken and
as the result we need this hack with `Finalize`.

+        _AtlModule.Finalize();
         exit(0);
       }
     }
 
   }
 
   void OnUpdateAvailable(AdblockPlus::JsValueList& params)
   {
     if (params.size() < 1)
     {
       Debug("updateAvailable event missing URL");
       return;
     }
     updateAvailable = true;
 
     updater->Update(params[0]->AsString());
+  }
+
+  std::wstring PreconfigurationValueFromRegistry(const std::wstring& preconfigName)
+  {
+    try
+    {
+      AdblockPlus::RegistryKey regKey(HKEY_CURRENT_USER, L"Software\\AdblockPlus");
+      return regKey.value_wstring(preconfigName);
+    }
+    catch (const std::runtime_error&)
+    {
+      return L"";
+    }
   }
 
 std::auto_ptr<AdblockPlus::FilterEngine> CreateFilterEngine(const std::wstring& locale)
 {
   AdblockPlus::AppInfo appInfo;
   appInfo.version = ToUtf8String(IEPLUGIN_VERSION);
   appInfo.name = "adblockplusie";
 #ifdef _WIN64
   appInfo.application = "msie64";
 #else
   appInfo.application = "msie32";
 #endif
   appInfo.applicationVersion = ToUtf8String(AdblockPlus::IE::InstalledVersionString());
   appInfo.locale = ToUtf8String(locale);
 #ifdef ADBLOCK_PLUS_TEST_MODE
   appInfo.developmentBuild = true;
 #else
   appInfo.developmentBuild = false;
 #endif
 
   AdblockPlus::JsEnginePtr jsEngine = AdblockPlus::JsEngine::New(appInfo);
   jsEngine->SetEventCallback("updateAvailable", &OnUpdateAvailable);
 
   std::string dataPath = ToUtf8String(GetAppDataPath());
   dynamic_cast<AdblockPlus::DefaultFileSystem*>(jsEngine->GetFileSystem().get())->SetBasePath(dataPath);
-  std::auto_ptr<AdblockPlus::FilterEngine> filterEngine(new AdblockPlus::FilterEngine(jsEngine));
+  std::map<std::string, AdblockPlus::JsValuePtr> preconfig;
+  preconfig["disable_auto_updates"] = jsEngine->NewValue(
+    PreconfigurationValueFromRegistry(L"disable_auto_updates") == L"true");
+  preconfig["suppress_first_run_page"] = jsEngine->NewValue(
+    PreconfigurationValueFromRegistry(L"suppress_first_run_page") == L"true");
+  std::auto_ptr<AdblockPlus::FilterEngine> filterEngine(new AdblockPlus::FilterEngine(jsEngine, preconfig));
   return filterEngine;
 }
 
-class ScopedTimer
-{
-public:
-  explicit ScopedTimer(HWND hwnd, UINT_PTR timerId, UINT elapse, TIMERPROC timerProc)
-    : m_hwnd(hwnd), m_timerId(timerId)
-  {
-    m_timerId = SetTimer(m_hwnd, m_timerId, elapse, timerProc);
-  }
-  ~ScopedTimer()
-  {
-    Close();
-  }
-
-  UINT_PTR GetID()
-  {
-    return m_timerId;
-  }
-
-  operator bool() const
-  {
-    return m_timerId != 0;
-  }
-
-  void Close()
-  {
-    if (m_timerId)
-    {
-      KillTimer(m_hwnd, m_timerId);
-      m_timerId = 0;
-    }
-  }
-private:
-  ScopedTimer(const ScopedTimer&);
-  ScopedTimer& operator=(const ScopedTimer&);
-private:
-  HWND m_hwnd;
-  UINT_PTR m_timerId;
-};
-
-struct ScopedAtlAxInitializer
-{
-  ScopedAtlAxInitializer()
-  {
-    ATL::AtlAxWinInit();
-  }
-  ~ScopedAtlAxInitializer()
-  {
-    ATL::AtlAxWinTerm();
-  }
-};
-
-void ApplicationMessageLoop(HINSTANCE hInstance)

[Oleksandr, 2015/07/22 09:45:35]

I still think using CreateTimerQueueTimer instead of a window listener would be
better, but I don't have a strong opinion. Also, since
https://codereview.adblockplus.org/29317074/ is closed we don't need a timer at
all now?

[sergei, 2015/07/23 14:17:57]

What is a window listener? Anyway the timer is removed.

+void ABPAtlModule::Finalize()
+{
+  std::condition_variable cv;
+  std::mutex cvMutex;
+  std::unique_lock<std::mutex> lock(cvMutex);
+  DispatchTask([&cvMutex, &cv, this]
+  {
+    if (m_notificationWindow)
+    {
+      m_notificationWindow->SendMessage(WM_CLOSE);
+    }
+    SendMessage(m_msgHWnd, WM_QUIT, 0, 0);
+    {
+      std::lock_guard<std::recursive_mutex> lock(m_tasksMutex);
+      m_tasks.clear();
+    }
+    std::unique_lock<std::mutex> lock(cvMutex);
+    cv.notify_one();
+  });
+  cv.wait(lock);
+}
+
+HRESULT ABPAtlModule::PreMessageLoop(int showCmd) throw()
 {
   const std::wstring className = L"ABPEngineMessageWindow";
   WNDCLASS wc;
   wc.style = 0;
   wc.lpfnWndProc = DefWindowProcW;
   wc.cbClsExtra = 0;
   wc.cbWndExtra = 0;
-  wc.hInstance = hInstance;
+  wc.hInstance = ATL::_AtlBaseModule.GetModuleInstance();
   wc.hIcon = nullptr;
   wc.hCursor = nullptr;
   wc.hbrBackground = nullptr;
   wc.lpszMenuName = nullptr;
   wc.lpszClassName = className.c_str();
   ATOM atom = RegisterClass(&wc);
   if (!atom)
   {
     DebugLastError("Cannot register class for message only window");
-    return;
-  }
-  HWND msgHWnd = CreateWindowW(className.c_str(),
+    return E_FAIL;
+  }
+  m_msgHWnd = CreateWindowW(className.c_str(),
     nullptr,      // window name
     0,            // style
     0, 0, 0, 0,   // geometry (x, y, w, h)
     HWND_MESSAGE, // parent
     nullptr,      // menu handle
-    hInstance,
+    wc.hInstance,
     0);           // windows creation data.
-  if (!msgHWnd)
+  if (!m_msgHWnd)
   {
     DebugLastError("Cannot create message only window");
-    return;
-  }
+    return E_FAIL;
+  }
+
+  filterEngine->SetShowNotificationCallback([this](const AdblockPlus::NotificationPtr& notification)
+  {
+    if (!notification)
+    {
+      return;
+    }
+    DispatchTask([notification, this]
+    {
+      onNewNotification(notification);
+    });
+  });
+
+  HRESULT retValue = __super::PreMessageLoop(showCmd);
+  // __super::PreMessageLoop returns S_FALSE because there is nothing to
+  // register but S_OK is required to run message loop.
+  return FAILED(retValue) ? retValue : S_OK;
+}
+
+void ABPAtlModule::RunMessageLoop() throw()
+{
   MSG msg = {};
-  ScopedTimer notificationTimer(msgHWnd, /*timer ID*/1, 3000/*msec*/, nullptr);
-  std::unique_ptr<NotificationBorderWindow> notificationWindow;
   while (GetMessage(&msg, /*hwnd*/nullptr, /*msgFilterMin*/0, /*msgFilterMax*/0))
   {
-    if (msg.hwnd == msgHWnd)
-    {
-      if (msg.message == WM_TIMER)
-      {
-        if (msg.wParam == notificationTimer.GetID())
-        {
-          notificationTimer.Close();
-          auto notification = filterEngine->GetNextNotificationToShow();
-          if (notification)
-          {
-            notificationWindow.reset(new NotificationBorderWindow(*notification, GetExeDir() + L"html\\templates\\"));
-            notificationWindow->SetOnDestroyed([&notificationWindow]
-            {
-              notificationWindow.reset();
-            });
-            notificationWindow->SetOnLinkClicked([](const std::wstring& url)
-            {
-              ATL::CComPtr<IWebBrowser2> webBrowser;
-              if (SUCCEEDED(webBrowser.CoCreateInstance(__uuidof(InternetExplorer))) && webBrowser)
-              {
-                ATL::CComVariant emptyVariant;
-                webBrowser->Navigate(ATL::CComBSTR(url.c_str()), &emptyVariant, &emptyVariant, &emptyVariant, &emptyVariant);
-                webBrowser->put_Visible(VARIANT_TRUE);
-              }
-            });
-            notificationWindow->Create(/*parent window*/nullptr);
-            if (notificationWindow->operator HWND() != nullptr)
-            {
-              notificationWindow->ShowWindow(SW_SHOWNOACTIVATE);
-              notificationWindow->UpdateWindow();
-            }
-          }
-        }
-      }
+    if (msg.hwnd == m_msgHWnd && msg.message == CustomMessages::TASK_POSTED)
+    {
+      ProcessTasks();
     }
     TranslateMessage(&msg);
     DispatchMessage(&msg);
   }
 }
 
+void ABPAtlModule::onNewNotification(const AdblockPlus::NotificationPtr& notification)
+{
+  m_notificationWindow.reset(new NotificationBorderWindow(*notification, GetExeDir() + L"html\\templates\\"));
+  m_notificationWindow->SetOnDestroyed([notification, this]
+  {
+    notification->MarkAsShown();
+    m_notificationWindow.reset();
+  });
+  m_notificationWindow->SetOnLinkClicked([](const std::wstring& url)
+  {
+    ATL::CComPtr<IWebBrowser2> webBrowser;
+    if (SUCCEEDED(webBrowser.CoCreateInstance(__uuidof(InternetExplorer))) && webBrowser)
+    {
+      ATL::CComVariant emptyVariant;
+      webBrowser->Navigate(ATL::CComBSTR(url.c_str()), &emptyVariant, &emptyVariant, &emptyVariant, &emptyVariant);
+      webBrowser->put_Visible(VARIANT_TRUE);
+    }
+  });
+  m_notificationWindow->Create(/*parent window*/nullptr);
+  if (m_notificationWindow->operator HWND() != nullptr)
+  {
+    m_notificationWindow->ShowWindow(SW_SHOWNOACTIVATE);
+    m_notificationWindow->UpdateWindow();
+  }
+}
+
+void ABPAtlModule::DispatchTask(std::function<void()>&& task)
+{
+  {
+    std::lock_guard<std::recursive_mutex> lock(m_tasksMutex);
+    m_tasks.emplace_back(std::move(task));
+  }
+  PostMessageW(m_msgHWnd, CustomMessages::TASK_POSTED, 0, 0);
+}
+
+void ABPAtlModule::ProcessTasks()
+{
+  std::lock_guard<std::recursive_mutex> lock(m_tasksMutex);
+  while(!m_tasks.empty())
+  {
+    auto task = *m_tasks.begin();
+    m_tasks.pop_front();
+    if (task)
+      task();
+  }
+}
+
 } // namespace {
 
-int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE, LPSTR, int)
+int WINAPI WinMain(HINSTANCE hInstance, HINSTANCE, LPSTR, int cmdShow)
 {
   AutoHandle mutex(CreateMutexW(0, false, L"AdblockPlusEngine"));
   if (!mutex)
   {
     DebugLastError("CreateMutex failed");
     return 1;
   }
 
   if (GetLastError() == ERROR_ALREADY_EXISTS)
   {
@@ skipping 32 matching lines @@
         return 1;
       }
       catch (const std::runtime_error& e)
       {
         DebugException(e);
         return 1;
       }
     }
   });
 
-  ScopedAtlAxInitializer atlAxInit;
-  ApplicationMessageLoop(hInstance);
+  int retValue = _AtlModule.WinMain(cmdShow);
   if (communicationThread.joinable())
   {
     communicationThread.join();
   }
 
-

[Oleksandr, 2015/07/22 09:45:35]

NIT: Extra line here

[sergei, 2015/07/23 14:17:57]

Fixed

-  return 0;
+  return retValue;
 }