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>
 
@@ skipping 17 matching lines @@
     ScopedAtlAxInitializer()
     {
       ATL::AtlAxWinInit();
     }
     ~ScopedAtlAxInitializer()
     {
       ATL::AtlAxWinTerm();
     }
   };
 
-  class ABPAtlModule : public ATL::CAtlExeModuleT<ABPAtlModule> {
+  class ABPAtlModule : public ATL::CAtlExeModuleT<ABPAtlModule>
+  {
     enum CustomMessages
     {
-      TaskPosted = WM_USER + 1

[Oleksandr, 2015/07/29 10:47:16]

Nit: TASK_POSTED. Our enums are almost always uppercase.

+      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();
-  private:
+    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 365 matching lines @@
       {
         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)
@@ skipping 49 matching lines @@
     PreconfigurationValueFromRegistry(L"suppress_first_run_page") == L"true");
   std::auto_ptr<AdblockPlus::FilterEngine> filterEngine(new AdblockPlus::FilterEngine(jsEngine, preconfig));
   return filterEngine;
 }
 
 void ABPAtlModule::Finalize()
 {
   std::condition_variable cv;
   std::mutex cvMutex;
   std::unique_lock<std::mutex> lock(cvMutex);
-  dispatchTask([&cvMutex, &cv, this]
+  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();
     }
@@ skipping 36 matching lines @@
     DebugLastError("Cannot create message only window");
     return E_FAIL;
   }
 
   filterEngine->SetShowNotificationCallback([this](const AdblockPlus::NotificationPtr& notification)
   {
     if (!notification)
     {
       return;
     }
-    dispatchTask([notification, this]
+    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::TaskPosted)
-    {
-      processTasks();
+    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]
@@ skipping 12 matching lines @@
     }
   });
   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)

[Oleksandr, 2015/07/29 10:47:16]

Nit: How about "DispatchTask" instead? We usually have methods starting with an
upper case.

+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::TaskPosted, 0, 0);
+  PostMessageW(m_msgHWnd, CustomMessages::TASK_POSTED, 0, 0);
 }
 
-void ABPAtlModule::processTasks()

[Oleksandr, 2015/07/29 10:47:16]

Nit: ProcessTasks(). Upper case?

+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();
   }
 }
@@ skipping 54 matching lines @@
   });
 
   int retValue = _AtlModule.WinMain(cmdShow);
   if (communicationThread.joinable())
   {
     communicationThread.join();
   }
 
   return retValue;
 }