Issue 119 - Switch to injecting CSS for element hiding

src/plugin/PluginClass.h

 /*
  * 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
@@ skipping 22 matching lines @@
 #include "NotificationMessage.h"
 
 #define ICON_PLUGIN_DISABLED 0
 #define ICON_PLUGIN_ENABLED 1
 #define ICON_PLUGIN_DEACTIVATED 2
 #define ICON_MAX 3
 
 #define WM_LAUNCH_INFO                                  (WM_APP + 10)
 
 class CPluginMimeFilterClient;
-
+class WebBrowserEventsListener;
 
 class ATL_NO_VTABLE CPluginClass :
   public ATL::CComObjectRootEx<ATL::CComMultiThreadModel>,
   public ATL::CComCoClass<CPluginClass, &CLSID_PluginClass>,
   public ATL::IObjectWithSiteImpl<CPluginClass>,
   public IOleCommandTarget,
   public ATL::IDispEventImpl<1, CPluginClass, &DIID_DWebBrowserEvents2, &LIBID_SHDocVw, 1, 1>
 {
 
   friend class CPluginTab;
 
-private:
-
-  CPluginTab* m_tab;

[Eric, 2015/12/02 17:00:14]

You've converted this to a 'unique_ptr' in the change set, which is an
improvement if you think it needs to be separately allocated.

I don't think it needs to be allocated at all. There's a 1-1 relationship
between BHO objects (this class 'CPluginClass') and 'CPluginTab'. There's no
good reason this shouldn't have been a ordinary, non-allocated pointer before.

[Eric, 2015/12/02 17:02:02]

That should be "ordinary, non-allocated object".

[Eric, 2015/12/02 18:45:18]

While auditing code, I found the same problem in 'CPluginTab'. Here's what the
fix for that problem looks like:
    https://codereview.adblockplus.org/29331767/

I think we'll be able to do the same thing here, but we'll only be able to tell
that after we get all the life cycle issues sorted out.

[sergei, 2016/02/25 17:48:27]

When I was doing it I tried to avoid changes unrelated to the current issue. The
code review was already very big and cleaning changes would only complicate it.

I have changed it from raw pointer mainly because now its lifetime is scoped by
the lifetime of `struct Data`, using `std::unique_ptr` looks the most elegant
way to achieve it.

Although it seems now we can change it from a pointer to an ordinary member I
think it should be in another commit.

-
 public:
 
   DECLARE_REGISTRY_RESOURCEID(IDR_PLUGIN_CLASS)
 
   DECLARE_PROTECT_FINAL_CONSTRUCT()
 
   BEGIN_COM_MAP(CPluginClass)
     COM_INTERFACE_ENTRY(IObjectWithSite)
     COM_INTERFACE_ENTRY(IOleCommandTarget)
   END_COM_MAP()
 
   BEGIN_SINK_MAP(CPluginClass)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_BEFORENAVIGATE2, OnBeforeNavigate2)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_DOWNLOADCOMPLETE, OnDownloadComplete)
-    SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_DOCUMENTCOMPLETE, OnDocumentComplete)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_WINDOWSTATECHANGED, OnWindowStateChanged)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_COMMANDSTATECHANGE, OnCommandStateChange)
     SINK_ENTRY_EX(1, DIID_DWebBrowserEvents2, DISPID_ONQUIT, OnOnQuit)
   END_SINK_MAP()
 
   CPluginClass();
   ~CPluginClass();
 
   HRESULT FinalConstruct();
   void FinalRelease();
@@ skipping 46 matching lines @@
   static void FirstRunThread();
 
   void STDMETHODCALLTYPE OnBeforeNavigate2(IDispatch* pDisp /**< [in] */,
                                            VARIANT* URL /**< [in] */,
                                            VARIANT* Flags /**< [in] */,
                                            VARIANT* TargetFrameName /**< [in] */,
                                            VARIANT* PostData /**< [in] */,
                                            VARIANT* Headers /**< [in] */,
                                            VARIANT_BOOL* Cancel /* [in, out] */);
   void STDMETHODCALLTYPE OnDownloadComplete();
-  void STDMETHODCALLTYPE OnDocumentComplete(IDispatch* frameBrowserDisp, VARIANT* /*urlOrPidl*/);

[Eric, 2015/12/02 17:00:14]

I don't think we'll need to put this event handler into a separate object.

I can tell you're working around an existing defect in our lifetime management
of objects and event registration. We already have three pending reviews on
these issues that we ought to fix first.

[sergei, 2016/02/25 17:48:27]

Can you give any arguments?
It's not related to lifetime management.

   void STDMETHODCALLTYPE OnWindowStateChanged(unsigned long flags, unsigned long validFlagsMask);
   void STDMETHODCALLTYPE OnCommandStateChange(long command, VARIANT_BOOL enable);
   void STDMETHODCALLTYPE OnOnQuit();
   void Unadvise();
+  void EnsureWebBrowserConnected(const ATL::CComPtr<IWebBrowser2>& webBrowser);
 
   void ShowStatusBar();
   bool IsStatusBarEnabled();
 
-public:
-  CComQIPtr<IWebBrowser2> m_webBrowser2;
 private:
   HWND m_hBrowserWnd;
   HWND m_hTabWnd;
   HWND m_hStatusBarWnd;
   HWND m_hPaneWnd;
   
   WNDPROC m_pWndProcStatus;
   int m_nPaneWidth;
   HANDLE m_hTheme;
+  struct Data
+  {
+    std::map<IWebBrowser2*, WebBrowserEventsListener*> connectedWebBrowsersCache;

[sergei, 2016/02/25 17:48:27]

These raw pointers is another point here regarding `shared_ptr/weak_ptr`. I
don't like them but I doubt that we actually want to keep strong references to
child 'WebBrowser's because in that case we can leak heavy 'WebBrowser's.

That point deserves to be deeply investigated, however the current version can
be used, so far.

[sergei, 2016/02/26 08:34:51]

BTW, in addition we probably need to serialize the access to that map, I will
look into it.

[sergei, 2016/02/26 16:57:56]

According to my tests destructors of `WebBrowserEventsListener` are always
called from the same thread in which they have been constructed, BTW it is the
same from which `SetSite` is called.

+    std::unique_ptr<CPluginTab> tab;
+    ATL::CComPtr<IWebBrowser2> webBrowser2;
+  };
+  // we need to have it as a shared pointer to get weak pointer to it to avoid
+  // wrong usage after destroying of this class.
+  std::shared_ptr<Data> m_data;

[Eric, 2015/12/02 17:00:14]

This struct looks like a Very Bad Idea.

If we get our lifetime management correct, there won't be any issue about events
coming in to non-existent objects, and thus a weak pointer won't be needed at
all.

[sergei, 2016/02/25 17:48:27]

It's not about events coming to non-existing objects.

I have observed that there is a race condition in IE regarding the destroying of
'WebBrowser's, even though `m_data` is destroyed (which happens only in
`~CPluginClass`) instances of `WebBrowserEventsListener` are still alive. It
means that our plugin is already destroyed but child 'WebBrowser's are still
alive.

Beside the comment above regarding raw pointers, we can definitely try to listen
to DISPID_ONQUIT from children and unsubscribe our `WebBrowserEventsListener` or
unsubscribe them when our plugin is being destroyed. However, it seems, in that
case the code will be merely more sophisticated. For example, it can happen that
DISPID_ONQUIT events from children come during unsubscribing operation, so we
need to serialize this part, it means again more locks and yet one "global"
variable.

So to operate safely and easier I'm using shared_ptr/weak_ptr.

We can definitely try to elaborate it, but I guess it would be better to make
later.
@Oleksandr?

 
   CriticalSection m_csStatusBar;
 
   NotificationMessage notificationMessage;
 
   bool m_isAdvised;
   bool m_isInitializedOk;
 
   // Atom pane class
   static ATOM s_atomPaneClass;
@@ skipping 22 matching lines @@
 
   // Async browser
   static CComQIPtr<IWebBrowser2> s_asyncWebBrowser2;
   static CComQIPtr<IWebBrowser2> GetAsyncBrowser();
 };
 
 OBJECT_ENTRY_AUTO(__uuidof(PluginClass), CPluginClass)
 
 
 #endif // _PLUGIN_CLASS_H_