Issue 1201 - Enabling/disabling ABP everywhere applies only in the next opened tab

src/plugin/PluginClientFactory.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/>.
  */
 
 #include "PluginStdAfx.h"
 
 #include "PluginClientFactory.h"
 #include "PluginMimeFilterClient.h"
 
 
 CPluginMimeFilterClient* CPluginClientFactory::s_mimeFilterInstance = NULL;
 
 CComAutoCriticalSection CPluginClientFactory::s_criticalSection;
 
 CPluginMimeFilterClient* CPluginClientFactory ::GetMimeFilterClientInstance() 

[sergei, 2015/04/01 18:49:32]

Right now we don't need this return value anymore, right?
However if we decide to implement suggested Enable()/Disable(), it will be
needed, what do you think about returning a reference instead of a pointer?

 {
   CPluginMimeFilterClient* localInstance = NULL;
 
   s_criticalSection.Lock();
   {
     if (!s_mimeFilterInstance)
     {
       //we cannot copy the client directly into the instance variable
       //if the constructor throws we do not want to alter instance

[Eric, 2015/03/27 13:35:50]

I don't think I had looked at this comment before, but if the constructor throws
here, you want to catch it and, at the very least, return 'nullptr'. And then
disable ABP for the scope of a null return.

This isn't the problem you're trying to address, I understand, but this
illustrates how screwed up the mime-filter life cycle code is.

[sergei, 2015/04/01 18:49:32]

Firstly, when CPluginMimeFilterClient throws the exception:
- the assignment localInstance = new CPluginMimeFilterClient(); is not executed,
so we can safely use `s_mimeFilterInstance = new CPluginMimeFilterClient();` and
get rid of `localInstance` because the variable being assigned to will not be
altered.
- anyway, I would say compiler removes `localInstance` as optimization

What concerns the handling of the exception, if we cannot install our protocol
handlers we still can block by CSS, although it's not a full plugnin
functionality. If some windows API call fails in CPluginMimeFilterClient then we
could set some "global" flag in the body of CPluginMimeFilterClient methods to
indicate to the user that the plugin cannot fully operate and don't throw any
exception. If the exception is thrown not by us, then it should be processed by
entry point wrapper, now this class has clearly visible single responsibility
and I don't think we should introduce here additional logic of disabling of ABP.

       localInstance = new CPluginMimeFilterClient();
 
       s_mimeFilterInstance = localInstance;
     }
     else
     {
       localInstance = s_mimeFilterInstance;
     }
   }
   s_criticalSection.Unlock();
 
   return localInstance;
 }
 
 void CPluginClientFactory::ReleaseMimeFilterClientInstance() 
 {
   s_criticalSection.Lock();
   {
-    delete s_mimeFilterInstance;
-
+    if (s_mimeFilterInstance)

[Eric, 2015/03/27 13:35:50]

See comment in Plugin.cpp.

+    {
+      delete s_mimeFilterInstance;
+    }
     s_mimeFilterInstance = NULL;
   }
   s_criticalSection.Unlock();
 }