[IE] First round of ATL removal

src/plugin/AdblockPlusClient.cpp

 #include "PluginStdAfx.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginFilter.h"
 #include "PluginClientFactory.h"
 #include "PluginMutex.h"
 #include "PluginClass.h"
 
 #include "AdblockPlusClient.h"
 
 #include "../shared/Utils.h"
 
+#include <memory>
+
 namespace
 {
+  /**
+   * A temporary wchar buffer, initialized from a wstring.
+   * Used to pass parameters to CreateProcess* API functions, which may modify the buffer contents.
+   *
+   * This class is a wrapper around unique_ptr.
+   */
+  class temporary_wchar_buffer {

[sergei, 2014/07/08 11:58:34]

Despite below it's mentioned that we can proceed even without additional
copying, this class can be replaced by `std::vector<wchar_t>`, there is
non-const `data()` method which returns a pointer to the underlying data, which
should be allocated in the continues memory.

[Eric, 2014/07/08 17:46:37]

On 2014/07/08 11:58:34, sergei wrote:
< this class can be replaced by `std::vector<wchar_t>`

No, it can't be. The API call requires a null-terminated string, and
std::wstring is not. (quote: "Accessing the value at data()+size() produces
undefined behavior") This means we need a custom constructor and thus a separate
class.

Now we might look at deriving such a class from a standard container,
admittedly. std::vector::data() is a C++11 extension, so if we're going to use a
C++11 construct, it might as well be std::array.

+    /**
+     * Length of input string, not including terminating null character.
+     */
+    size_t len ;
+
+    /**
+     * The underlying unique_ptr
+     */
+    std::unique_ptr< wchar_t[] > buffer ;

[Oleksandr, 2014/06/26 00:48:43]

Nit: Here and in other places no space after < and before >. Or after ( and
before ). Or [ and ].

+
+  public:
+    /**
+     * Ordinary constructor.
+     */
+    temporary_wchar_buffer( std::wstring s )

[sergei, 2014/07/08 11:58:34]

Just in case, it's safer to have such constructors to be `explicit`, to avoid
any confusing behaviour.

[Eric, 2014/07/08 17:46:37]

I have no problem with that.

+      : len( s.length() ), buffer( new wchar_t[ len + 1 ] )
+    {
+      s.copy( buffer.get(), len );
+      buffer[ len ] = L'\0' ;
+    }
+
+    /**
+     * Conversion operator returns pointer to allocated buffer.
+     */
+    operator wchar_t *()
+    {
+      return buffer.get();
+    }
+  };
+
   void SpawnAdblockPlusEngine()
   {
     std::wstring engineExecutablePath = GetDllDir() + L"AdblockPlusEngine.exe";
-    CString params = L"AdblockPlusEngine.exe " + CPluginSystem::GetInstance()->GetBrowserLanguage();
+    temporary_wchar_buffer params( L"AdblockPlusEngine.exe " + CPluginSystem::GetInstance()->GetBrowserLanguage() );
 
     STARTUPINFO startupInfo = {};
     PROCESS_INFORMATION processInformation = {};
 
     HANDLE token;
     OpenProcessToken(GetCurrentProcess(), TOKEN_DUPLICATE | TOKEN_ADJUST_DEFAULT | TOKEN_QUERY | TOKEN_ASSIGN_PRIMARY, &token);
 
     TOKEN_APPCONTAINER_INFORMATION *acs = NULL;
     DWORD length = 0;
 
@@ skipping 10 matching lines @@
           throw std::runtime_error("Out of memory");
         }
     }
 
     BOOL createProcRes = 0;
     // Running inside AppContainer or in Windows XP
     if ((acs != NULL && acs->TokenAppContainer != NULL) || (!IsWindowsVistaOrLater()))
     {
       // We need to break out from AppContainer. Launch with default security - registry entry will eat the user prompt
       // See http://msdn.microsoft.com/en-us/library/bb250462(v=vs.85).aspx#wpm_elebp
-      createProcRes = CreateProcessW(engineExecutablePath.c_str(), params.GetBuffer(params.GetLength() + 1),
+      createProcRes = CreateProcessW(engineExecutablePath.c_str(), (LPWSTR) params,

[Oleksandr, 2014/06/26 00:48:43]

We've had a similar issue (a need to get a writable wstring's buffer) in
libadblockplus\src\Utils.cpp, and the solution there was to use something like 

std::wstring params;
CreateProcessW(..., &params[0],...)

I agree that the temporary_wchar_buffer can be considered safer then the
approach above, since in theory some compilers can have strings in multiple
memory blocks. But in C++11 there is a requirement that std::basic_string's
buffer was allocated contiguously in memory, and in most C++03 compilers it is
de facto the case. I would argue to use the same approach here for code
simplicity. We don't need temporary_wchar_buffer then.

[Eric, 2014/06/26 15:13:56]

This issue is not contiguous memory. Rather, it's that CreateProcessW is
specifically documented to need writable memory.

*> The Unicode version of this function, CreateProcessW, can modify 
*> the contents of this string. Therefore, this parameter cannot be
*> a pointer to read-only memory (such as a const variable or a literal
*> string). If this parameter is a constant string, the function may
*> cause an access violation.

Using &params[0] or params.c_str() both give access to the internal
representation of the string, which is not a writable buffer. It's possible
there's not actual problem here, but without knowing implementation details of
basic_string, it's a wish and a prayer to do otherwise. And even if there's no
problem now, the implementation could change in a future version.

I don't like the separate copy; it's inefficient. In this specific case, though,
this function gets called infrequently, so there's barely a performance hit.

[Felix Dahlke, 2014/06/30 13:35:39]

c_str() returns a const char, but we using &params[0] as writable memory seems
to be well-defined in C++11. I vote for that. (Note that I was _against_ doing
this with C++03, but we still do it. Nobody's aware of any compiler that does
not store strings contiguously.)

[sergei, 2014/07/08 11:58:34]

I also vote for `&params[0]`.

[Eric, 2014/07/08 17:46:37]

We have two issues:
1) Contiguous memory.
2) Writable memory.

We get contiguous memory by explicit guarantee in std::array and std::vector,
and in practice with std::wstring. However, we don't get writable memory with
std::wstring, since its data() method is 'const'. That's the whole reason for
the buffer class above.

                               0, 0, false, 0, 0, 0, (STARTUPINFOW*)&startupInfo, &processInformation);
     }
     else
     {
       // Launch with Low Integrity explicitly
       HANDLE newToken;
       DuplicateTokenEx(token, 0, 0, SecurityImpersonation, TokenPrimary, &newToken);
 
       PSID integritySid = 0;
       ConvertStringSidToSid(L"S-1-16-4096", &integritySid);
       std::tr1::shared_ptr<SID> sharedIntegritySid(static_cast<SID*>(integritySid), FreeSid); // Just to simplify cleanup
 
       TOKEN_MANDATORY_LABEL tml = {};
       tml.Label.Attributes = SE_GROUP_INTEGRITY;
       tml.Label.Sid = integritySid;
 
       // Set the process integrity level
       SetTokenInformation(newToken, TokenIntegrityLevel, &tml, sizeof(TOKEN_MANDATORY_LABEL) + GetLengthSid(integritySid));
 
       STARTUPINFO startupInfo = {};
       PROCESS_INFORMATION processInformation = {};
 
-      createProcRes = CreateProcessAsUserW(newToken, engineExecutablePath.c_str(), params.GetBuffer(params.GetLength() + 1),
+      createProcRes = CreateProcessAsUserW(newToken, engineExecutablePath.c_str(), (LPWSTR) params,

[sergei, 2014/07/08 11:58:34]

First of all, the compile should understand that the conversion is required even
without explicit cast, secondly it's better to use `static_cast` instead of
C-style cast.

[Eric, 2014/07/08 17:46:37]

I have to admit that the C-style cast is copied from the Microsoft documentation
page. Brain fart. As long as the type of 'params' admits a writable buffer (as
this code is) and not a std::wstring, then '&params[0]' is fine, both here and
above, since they're each a variant on the same kind of system call.

                               0, 0, false, 0, 0, 0, (STARTUPINFOW*)&startupInfo, &processInformation);
     }
 
     if (!createProcRes)
     {
       throw std::runtime_error("Failed to start Adblock Plus Engine");
     }
 
     CloseHandle(processInformation.hProcess);
     CloseHandle(processInformation.hThread);
@@ skipping 67 matching lines @@
 
 CAdblockPlusClient* CAdblockPlusClient::s_instance = NULL;
 
 CAdblockPlusClient::CAdblockPlusClient() : CPluginClientBase()
 {
   m_filter = std::auto_ptr<CPluginFilter>(new CPluginFilter());
 }
 
 bool CAdblockPlusClient::CallEngine(Communication::OutputBuffer& message, Communication::InputBuffer& inputBuffer)
 {
-  DEBUG_GENERAL("CallEngine start");
+  DEBUG_GENERAL(L"CallEngine start");
   CriticalSection::Lock lock(enginePipeLock);
   try
   {
     if (!enginePipe)
       enginePipe.reset(OpenEnginePipe());
     enginePipe->WriteMessage(message);
     inputBuffer = enginePipe->ReadMessage();
   }
   catch (const std::exception& e)
   {
-    DEBUG_GENERAL(e.what());
+    DEBUG_GENERAL( ABP::debug::widen( e.what() ) );
     return false;
   }
-  DEBUG_GENERAL("CallEngine end");
+  DEBUG_GENERAL(L"CallEngine end");
   return true;
 }
 
 bool CAdblockPlusClient::CallEngine(Communication::ProcType proc, Communication::InputBuffer& inputBuffer)
 {
   Communication::OutputBuffer message;
   message << proc;
   return CallEngine(message, inputBuffer);
 }
 
@@ skipping 17 matching lines @@
     }
 
     instance = s_instance;
   }
   s_criticalSectionLocal.Unlock();
 
   return instance;
 }
 
 
-bool CAdblockPlusClient::ShouldBlock(CString src, int contentType, const CString& domain, bool addDebug)
+bool CAdblockPlusClient::ShouldBlock( std::wstring src, int contentType, const std::wstring & domain, bool addDebug)

[Oleksandr, 2014/06/26 00:48:43]

Nit: no space after const std::wstring

 {
   bool isBlocked = false;
 
   bool isCached = false;
 
   CPluginSettings* settings = CPluginSettings::GetInstance();
 
   m_criticalSectionCache.Lock();
   {
-    std::map<CString,bool>::iterator it = m_cacheBlockedSources.find(src);
+    std::map< std::wstring, bool >::iterator it = m_cacheBlockedSources.find(src);
 
     isCached = it != m_cacheBlockedSources.end();
     if (isCached)
     {
       isBlocked = it->second;
     }
   }
   m_criticalSectionCache.Unlock();
 
   if (!isCached)
@@ skipping 13 matching lines @@
         m_cacheBlockedSources[src] = isBlocked;
       }
       m_criticalSectionCache.Unlock();
     }
   }
 
 
   return isBlocked;
 }
 
-bool CAdblockPlusClient::IsElementHidden(const CString& tag, IHTMLElement* pEl, const CString& domain, const CString& indent, CPluginFilter* filter)
+bool CAdblockPlusClient::IsElementHidden(const std::wstring & tag, IHTMLElement* pEl, const std::wstring & domain, const std::wstring & indent, CPluginFilter* filter)
 {
   bool isHidden;
   m_criticalSectionFilter.Lock();
   {
     isHidden = filter && filter->IsElementHidden(tag, pEl, domain, indent);
   }
   m_criticalSectionFilter.Unlock();
   return isHidden;
 }
 
 bool CAdblockPlusClient::IsWhitelistedUrl(const std::wstring& url)
 {
-  DEBUG_GENERAL((L"IsWhitelistedUrl: " + url + L" start").c_str());
+  DEBUG_GENERAL(L"IsWhitelistedUrl: " + url + L" start");
   Communication::OutputBuffer request;
   request << Communication::PROC_IS_WHITELISTED_URL << ToUtf8String(url);
 
   Communication::InputBuffer response;
   if (!CallEngine(request, response)) 
     return false;
 
   bool isWhitelisted;
   response >> isWhitelisted;
 
-  DEBUG_GENERAL((L"IsWhitelistedUrl: " + url + L" end").c_str());
+  DEBUG_GENERAL(L"IsWhitelistedUrl: " + url + L" end");
   return isWhitelisted;
 }
 
 int CAdblockPlusClient::GetIEVersion()
 {
   //HKEY_LOCAL_MACHINE\Software\Microsoft\Internet Explorer
   HKEY hKey;
   LSTATUS status = RegOpenKey(HKEY_LOCAL_MACHINE, L"Software\\Microsoft\\Internet Explorer", &hKey);
   if (status != 0)
   {
@@ skipping 67 matching lines @@
 std::vector<std::wstring> CAdblockPlusClient::GetExceptionDomains()
 {
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_GET_EXCEPTION_DOMAINS, response)) 
     return std::vector<std::wstring>();
   return ReadStrings(response);
 }
 
 bool CAdblockPlusClient::IsFirstRun()
 {
-  DEBUG_GENERAL("IsFirstRun");
+  DEBUG_GENERAL(L"IsFirstRun");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_IS_FIRST_RUN_ACTION_NEEDED, response)) return false;
   bool res;
   response >> res;
   return res;
 }
 void CAdblockPlusClient::AddFilter(const std::wstring& text)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_ADD_FILTER << ToUtf8String(text);
@@ skipping 27 matching lines @@
   request << Communication::PROC_SET_PREF << ToUtf8String(name) << value;
   CallEngine(request);
 }
 
 std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, const wchar_t* defaultValue)
 {
   return GetPref(name, std::wstring(defaultValue));
 }
 std::wstring CAdblockPlusClient::GetPref(const std::wstring& name, const std::wstring& defaultValue)
 {
-  DEBUG_GENERAL((L"GetPref: " + name + L" start").c_str());
+  DEBUG_GENERAL(L"GetPref: " + name + L" start");
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_PREF << ToUtf8String(name);
 
   Communication::InputBuffer response;
   if (!CallEngine(request, response)) 
     return defaultValue;
   bool success;
   response >> success;
   if (success)
   {
     std::string value;
     response >> value;
-    DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
+    DEBUG_GENERAL(L"GetPref: " + name + L" end");
     return ToUtf16String(value);
   }
   else
   {
-    DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
+    DEBUG_GENERAL(L"GetPref: " + name + L" end");
     return defaultValue;
   }
 }
 
 bool CAdblockPlusClient::GetPref(const std::wstring& name, bool defaultValue)
 {
-  DEBUG_GENERAL((L"GetPref: " + name + L" start").c_str());
+  DEBUG_GENERAL(L"GetPref: " + name + L" start");
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_PREF << ToUtf8String(name);
 
   Communication::InputBuffer response;
   if (!CallEngine(request, response)) 
     return defaultValue;
   bool success;
   response >> success;
   if (success)
   {
     bool value;
     response >> value;
-    DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
+    DEBUG_GENERAL(L"GetPref: " + name + L" end");
     return value;
   }
   else
   {
-    DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
+    DEBUG_GENERAL(L"GetPref: " + name + L" end");
     return defaultValue;
   }
 }
 int64_t CAdblockPlusClient::GetPref(const std::wstring& name, int64_t defaultValue)
 {
-  DEBUG_GENERAL((L"GetPref: " + name + L" start").c_str());
+  DEBUG_GENERAL(L"GetPref: " + name + L" start");
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_PREF << ToUtf8String(name);
 
   Communication::InputBuffer response;
   if (!CallEngine(request, response)) 
     return defaultValue;
   bool success;
   response >> success;
   if (success)
   {
     int64_t value;
     response >> value;
-    DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
+    DEBUG_GENERAL(L"GetPref: " + name + L" end");
     return value;
   }
   else
   {
-    DEBUG_GENERAL((L"GetPref: " + name + L" end").c_str());
+    DEBUG_GENERAL(L"GetPref: " + name + L" end");
     return defaultValue;
   }
 }
 
 void CAdblockPlusClient::CheckForUpdates(HWND callbackWindow)
 {
   Communication::OutputBuffer request;
   request << Communication::PROC_CHECK_FOR_UPDATES << reinterpret_cast<int32_t>(callbackWindow);
   CallEngine(request);
 }
 
 std::wstring CAdblockPlusClient::GetDocumentationLink()
 {
-  DEBUG_GENERAL("GetDocumentationLink");
+  DEBUG_GENERAL(L"GetDocumentationLink");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_GET_DOCUMENTATION_LINK, response)) 
     return L"";
   std::wstring docLink;
   response >> docLink;
   return docLink;
 }
 
 bool CAdblockPlusClient::TogglePluginEnabled()
 {
-  DEBUG_GENERAL("TogglePluginEnabled");
+  DEBUG_GENERAL(L"TogglePluginEnabled");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_TOGGLE_PLUGIN_ENABLED, response)) 
     return false;
   bool currentEnabledState;
   response >> currentEnabledState;
   return currentEnabledState;
 }
 std::wstring CAdblockPlusClient::GetHostFromUrl(const std::wstring& url)
 {
-  DEBUG_GENERAL("GetHostFromUrl");
+  DEBUG_GENERAL(L"GetHostFromUrl");
   Communication::OutputBuffer request;
   request << Communication::PROC_GET_HOST << ToUtf8String(url);
 
   Communication::InputBuffer response;
   if (!CallEngine(request, response)) 
     return L"";
   std::string host;
   response >> host;
   return ToUtf16String(host);
 }