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

src/plugin/PluginTabBase.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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 11 matching lines @@
 #include "AdblockPlusDomTraverser.h"
 #include "PluginTabBase.h"
 #include "IeVersion.h"
 #include "../shared/Utils.h"
 #include <Mshtmhst.h>
 
 CPluginTab::CPluginTab()
   : m_isActivated(false)
   , m_continueThreadRunning(true)
 {
-  m_filter.hideFiltersLoadedEvent = CreateEvent(NULL, true, false, NULL);
-
   CPluginClient* client = CPluginClient::GetInstance();
   if (AdblockPlus::IE::InstalledMajorVersion() < 10)
   {
     m_isActivated = true;
   }
 
   try
   {
     m_thread = std::thread(&CPluginTab::ThreadProc, this);
   }
@@ skipping 28 matching lines @@
 {
   m_isActivated = true;
 }
 
 
 void CPluginTab::OnUpdate()
 {
   m_isActivated = true;
 }
 
-namespace
-{
-  // Entry Point
-  void FilterLoader(CPluginFilter* filter, const std::wstring& domain)
-  {
-    try
-    {
-      filter->LoadHideFilters(CPluginClient::GetInstance()->GetElementHidingSelectors(domain));
-      SetEvent(filter->hideFiltersLoadedEvent);
-    }
-    catch (...)
-    {
-      // As a thread-main function, we truncate any C++ exception.
-    }
-  }
-}
-
 void CPluginTab::OnNavigate(const std::wstring& url)
 {
   SetDocumentUrl(url);
-  ClearFrameCache(GetDocumentDomain());
-  std::wstring domainString = GetDocumentDomain();
-  ResetEvent(m_filter.hideFiltersLoadedEvent);
-  try
+}
+
+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)
   {
-    std::thread filterLoaderThread(&FilterLoader, &m_filter, GetDocumentDomain());
-    filterLoaderThread.detach(); // TODO: but actually we should wait for the thread in the dtr.
+    try
+    {
+      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 (const std::exception& e)
+    {
+      DEBUG_EXCEPTION(e);
+    }
   }
-  catch (const std::system_error& ex)
+  else
   {
-    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")
   }
 }
 
 namespace
 {
   /**
    * Determine if the HTML file is one of ours.
    * The criterion is that it appear in the "html/templates" folder within our installation.
    *
    * Warning: This function may fail if the argument is not a "file://" URL.
@@ skipping 130 matching lines @@
         || client->IsElemhideWhitelistedOnDomain(url, frameHierarchy);
   }
 }
 
 void CPluginTab::OnDownloadComplete(IWebBrowser2* browser)
 {
   CPluginClient* client = CPluginClient::GetInstance();
   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);
 }
 
 void CPluginTab::OnDocumentComplete(IWebBrowser2* browser, const std::wstring& url, bool isDocumentBrowser)
 {
   std::wstring documentUrl = GetDocumentUrl();
 
   if (isDocumentBrowser)
   {
     if (url != documentUrl)
@@ skipping 163 matching lines @@
           LogQueue::LogPluginError(pluginError.GetErrorCode(), pluginError.GetErrorId(), pluginError.GetErrorSubid(), pluginError.GetErrorDescription(), true, pluginError.GetProcessId(), pluginError.GetThreadId());
         }
 
         // Non-hanging sleep
         Sleep(50);
       }
 
       tabLoopIteration++;
   }
 }