Issue #2230, #3391 - Load filters on "download begin" event

src/plugin/PluginTabBase.cpp

Index: src/plugin/PluginTabBase.cpp
===================================================================
--- a/src/plugin/PluginTabBase.cpp
+++ b/src/plugin/PluginTabBase.cpp
@@ -29,8 +29,6 @@
   : m_isActivated(false)
   , m_continueThreadRunning(true)
 {
-  m_filter.hideFiltersLoadedEvent = CreateEvent(NULL, true, false, NULL);
-
   CPluginClient* client = CPluginClient::GetInstance();
   if (AdblockPlus::IE::InstalledMajorVersion() < 10)
   {
@@ -79,38 +77,29 @@
   m_isActivated = true;
 }
 
-namespace
+void CPluginTab::OnNavigate(const std::wstring& url)
 {
-  // Entry Point
-  void FilterLoader(CPluginFilter* filter, const std::wstring& domain)
+  SetDocumentUrl(url);
+}
+
+void CPluginTab::OnDownloadBegin()

[sergei, 2016/01/29 10:03:20]

DownloadComplete and DownloadBegin are fired several times on the same page, do
we really need to load filters several times?

[sergei, 2016/01/29 10:03:21]

Are all DownloadBegin and DownloadComplete called only from the same thread? If
they are called from several threads then there is access violation problem,
because we can access `CPluginFilter* f` which is already destroyed.

[Oleksandr, 2016/02/01 02:12:27]

In my tests they are called from the same thread. I think we are fine here.

[Eric, 2016/02/03 18:03:20]

See reply to Oleksandr's concern about the same issue.

[Eric, 2016/02/03 18:03:20]

I believe this is part of the "apartment-like" threading that IE uses to make
calls into the BHO.

+{
+  std::wstring domain = GetDocumentDomain();
+  ClearFrameCache(domain);
+  if (!filter)
   {
     try
     {
-      filter->LoadHideFilters(CPluginClient::GetInstance()->GetElementHidingSelectors(domain));
-      SetEvent(filter->hideFiltersLoadedEvent);
+      filter.reset(new CPluginFilter(domain));

[sergei, 2016/01/29 10:03:21]

That's really non-readable here that initialization is asynchronous if you are
not aware about internals of CPluginFilter.

[Eric, 2016/02/03 18:03:20]

It really doesn't matter here how 'CPluginFilter' initializes itself. All that
matters is whether the object exists or not already.

     }
-    catch (...)
+    catch (const std::exception& e)
     {
-      // As a thread-main function, we truncate any C++ exception.
+      DEBUG_EXCEPTION(e);
     }
   }
-}
-
-void CPluginTab::OnNavigate(const std::wstring& url)
-{
-  SetDocumentUrl(url);
-  ClearFrameCache(GetDocumentDomain());
-  std::wstring domainString = GetDocumentDomain();
-  ResetEvent(m_filter.hideFiltersLoadedEvent);
-  try
+  else
   {
-    std::thread filterLoaderThread(&FilterLoader, &m_filter, GetDocumentDomain());
-    filterLoaderThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
-  }
-  catch (const std::system_error& ex)
-  {
-    DEBUG_SYSTEM_EXCEPTION(ex, PLUGIN_ERROR_THREAD, PLUGIN_ERROR_MAIN_THREAD_CREATE_PROCESS,
-      "Class::Thread - Failed to start filter loader thread");
+    DEBUG_GENERAL("Suspicious: Reached CPluginTab::OnDownloadBegin with a non-null filter")
   }
 }
 
@@ -261,9 +250,15 @@
   std::wstring url = GetDocumentUrl();
   if (!client->IsWhitelistedUrl(url) && !client->IsElemhideWhitelistedOnDomain(url))
   {
-    CPluginDomTraverser traversal(&m_filter);
-    traversal.TraverseDocumentRoot(browser, GetDocumentDomain(), GetDocumentUrl());
+    CPluginFilter* f = filter.get();
+    if (f)

[sergei, 2016/01/29 10:03:20]

What the trick are you trying to use here? it looks very suspicious.

[Oleksandr, 2016/02/01 02:12:27]

+1. I don't understand what is this testing.

[Eric, 2016/02/03 18:03:20]

It's defensive programming. The declaration changed from a fixed member instance
of 'CPluginFilter' to a 'unique_ptr' to one. This is simply ensure the pointer
isn't null before indirecting it.

It should never be the case that 'OnDownloadComplete' is ever called without at
least a single call to 'DownloadBegin', so 'f' should never be null. It would
probably be better to throw 'logic_error' if 'f' is null, but I didn't go that
far here.

+    {
+      f->EnsureInitialized();

[sergei, 2016/01/29 10:03:21]

It would be better to have some wrapper around CPluginFilter with the getter
that waits for the finishing of the initialization or throws if the
initialization has thrown.

[Eric, 2016/02/03 18:03:20]

It's a trade-off. You could declare two classes, as you suggest, or just expose
'EnsureInitialized()' in the one class, as done here. I don't see that the
benefit of a second class is large enough to warrant all the extra source code.

+      CPluginDomTraverser traversal(f);
+      traversal.TraverseDocumentRoot(browser, GetDocumentDomain(), GetDocumentUrl());
+    }
   }
+  filter.reset();
   InjectABP(browser);
 }