[IE] First round of ATL removal

src/plugin/AdblockPlusClient.cpp

Index: src/plugin/AdblockPlusClient.cpp
===================================================================
--- a/src/plugin/AdblockPlusClient.cpp
+++ b/src/plugin/AdblockPlusClient.cpp
@@ -10,12 +10,51 @@
 
 #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 = {};
@@ -46,7 +85,7 @@
     {
       // 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
@@ -69,7 +108,7 @@
       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);
     }
 
@@ -157,7 +196,7 @@
 
 bool CAdblockPlusClient::CallEngine(Communication::OutputBuffer& message, Communication::InputBuffer& inputBuffer)
 {
-  DEBUG_GENERAL("CallEngine start");
+  DEBUG_GENERAL(L"CallEngine start");
   CriticalSection::Lock lock(enginePipeLock);
   try
   {
@@ -168,10 +207,10 @@
   }
   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;
 }
 
@@ -209,7 +248,7 @@
 }
 
 
-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;
 
@@ -219,7 +258,7 @@
 
   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)
@@ -253,7 +292,7 @@
   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();
@@ -266,7 +305,7 @@
 
 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);
 
@@ -277,7 +316,7 @@
   bool isWhitelisted;
   response >> isWhitelisted;
 
-  DEBUG_GENERAL((L"IsWhitelistedUrl: " + url + L" end").c_str());
+  DEBUG_GENERAL(L"IsWhitelistedUrl: " + url + L" end");
   return isWhitelisted;
 }
 
@@ -365,7 +404,7 @@
 
 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;
@@ -413,7 +452,7 @@
 }
 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);
 
@@ -426,19 +465,19 @@
   {
     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);
 
@@ -451,18 +490,18 @@
   {
     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);
 
@@ -475,12 +514,12 @@
   {
     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;
   }
 }
@@ -494,7 +533,7 @@
 
 std::wstring CAdblockPlusClient::GetDocumentationLink()
 {
-  DEBUG_GENERAL("GetDocumentationLink");
+  DEBUG_GENERAL(L"GetDocumentationLink");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_GET_DOCUMENTATION_LINK, response)) 
     return L"";
@@ -505,7 +544,7 @@
 
 bool CAdblockPlusClient::TogglePluginEnabled()
 {
-  DEBUG_GENERAL("TogglePluginEnabled");
+  DEBUG_GENERAL(L"TogglePluginEnabled");
   Communication::InputBuffer response;
   if (!CallEngine(Communication::PROC_TOGGLE_PLUGIN_ENABLED, response)) 
     return false;
@@ -515,7 +554,7 @@
 }
 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);