Issue 119 - Switch to injecting CSS for element hiding

src/plugin/PluginClass.h

Index: src/plugin/PluginClass.h
diff --git a/src/plugin/PluginClass.h b/src/plugin/PluginClass.h
index 864b8b0e23a271c3d704548a886a4dce9592348e..d6f6fb30351dc2923b0a2add4c5fed5fee2e7074 100644
--- a/src/plugin/PluginClass.h
+++ b/src/plugin/PluginClass.h
@@ -40,7 +40,7 @@
 #define WM_LAUNCH_INFO					(WM_APP + 10)
 
 class CPluginMimeFilterClient;
-
+class WebBrowserEventsListener;
 
 class ATL_NO_VTABLE CPluginClass :
   public ATL::CComObjectRootEx<ATL::CComMultiThreadModel>,
@@ -52,10 +52,6 @@ class ATL_NO_VTABLE CPluginClass :
 
   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)
@@ -70,7 +66,6 @@ public:
   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)
@@ -137,17 +132,15 @@ private:
                                            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;
@@ -157,6 +150,15 @@ private:
   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;