Issue 1109 - Support notifications

src/engine/Main.cpp

Index: src/engine/Main.cpp
diff --git a/src/engine/Main.cpp b/src/engine/Main.cpp
index 68e1481f29239778c3b21f3be676f18c51f177b3..5c06428d324eebf1263deac78d6703e7c5fcccc4 100644
--- a/src/engine/Main.cpp
+++ b/src/engine/Main.cpp
@@ -15,10 +15,17 @@
  * 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"
@@ -32,9 +39,54 @@
 #include "Debug.h"
 #include "Updater.h"
 #include "Registry.h"
+#include "NotificationWindow.h"
 
 namespace
 {
+  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;
@@ -412,6 +464,15 @@ namespace
       {
         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);
       }
     }
@@ -442,7 +503,6 @@ namespace
       return L"";
     }
   }
-}
 
 std::auto_ptr<AdblockPlus::FilterEngine> CreateFilterEngine(const std::wstring& locale)
 {
@@ -476,7 +536,144 @@ std::auto_ptr<AdblockPlus::FilterEngine> CreateFilterEngine(const std::wstring&
   return filterEngine;
 }
 
-int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int)
+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 = 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 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
+    wc.hInstance,
+    0);           // windows creation data.
+  if (!m_msgHWnd)
+  {
+    DebugLastError("Cannot create message only window");
+    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 = {};
+  while (GetMessage(&msg, /*hwnd*/nullptr, /*msgFilterMin*/0, /*msgFilterMax*/0))
+  {
+    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 cmdShow)
 {
   AutoHandle mutex(CreateMutexW(0, false, L"AdblockPlusEngine"));
   if (!mutex)
@@ -499,32 +696,41 @@ int WINAPI WinMain(HINSTANCE, HINSTANCE, LPSTR, int)
   filterEngine = CreateFilterEngine(locale);
   updater.reset(new Updater(filterEngine->GetJsEngine()));
 
-  for (;;)
+  std::thread communicationThread([]
   {
-    try
+    for (;;)
     {
-      auto pipe = std::make_shared<Communication::Pipe>(Communication::pipeName, Communication::Pipe::MODE_CREATE);
-
-      // TODO: we should wait for the finishing of the thread before exiting from this function.
-      // It works now in most cases because the browser waits for the response in the pipe, and the
-      // thread has time to finish while this response is being processed and the browser is
-      // disposing all its stuff.
-      std::thread([pipe]()
+      try
       {
-        ClientThread(pipe.get());
-      }).detach();
-    }
-    catch(const std::system_error& ex)
-    {
-      DebugException(ex);
-      return 1;
-    }
-    catch (const std::runtime_error& e)
-    {
-      DebugException(e);
-      return 1;
+        auto pipe = std::make_shared<Communication::Pipe>(Communication::pipeName, Communication::Pipe::MODE_CREATE);
+    
+        // TODO: we should wait for the finishing of the thread before exiting from this function.
+        // It works now in most cases because the browser waits for the response in the pipe, and the
+        // thread has time to finish while this response is being processed and the browser is
+        // disposing all its stuff.
+        std::thread([pipe]()
+        {
+          ClientThread(pipe.get());
+        }).detach();
+      }
+      catch(const std::system_error& ex)
+      {
+        DebugException(ex);
+        return 1;
+      }
+      catch (const std::runtime_error& e)
+      {
+        DebugException(e);
+        return 1;
+      }
     }
+  });
+
+  int retValue = _AtlModule.WinMain(cmdShow);
+  if (communicationThread.joinable())
+  {
+    communicationThread.join();
   }
 
-  return 0;
+  return retValue;
 }