Issue #1234 - std::wstring version of UnescapeUrl

src/plugin/PluginClientBase.cpp

 #include "PluginStdAfx.h"
 
 // Internet / FTP
 #include <wininet.h>
 
 // IP adapter
 #include <iphlpapi.h>
 
 #include "PluginSettings.h"
 #include "PluginSystem.h"
@@ skipping 22 matching lines @@
 CPluginClientBase::CPluginClientBase()
 {
 }
 
 
 CPluginClientBase::~CPluginClientBase()
 {
 }
 
 
-bool CPluginClientBase::IsValidDomain(const CString& domain)
-{
-  return domain != L"about:blank" &&
-    domain != L"about:tabs" &&
-    domain.Find(L"javascript:") != 0 &&
-    !domain.IsEmpty();
-}
-
-
 CString& CPluginClientBase::UnescapeUrl(CString& url)
 {
   CString unescapedUrl;
   DWORD cb = 2048;
 
   if (SUCCEEDED(::UrlUnescape(url.GetBuffer(), unescapedUrl.GetBufferSetLength(cb), &cb, 0)))
   {
     unescapedUrl.ReleaseBuffer();
     unescapedUrl.Truncate(cb);
 
     url.ReleaseBuffer();
     url = unescapedUrl;
   }
 
   return url;
 }
 
 void UnescapeUrl(std::wstring& url)
 {
-  /*
-   * When the code can tolerate exceptions better,
-   *   there's no reason to keep this try-catch statement in place.
-   */
   try
   {
     DWORD result_length = INTERNET_MAX_URL_LENGTH;
-    /*
-     * The buffer length is greater than 2 Kb, so we keep it off the stack and allocate it.
-     */
-    std::unique_ptr<wchar_t> result(new wchar_t[result_length]); // can throw bad_alloc

[sergei, 2014/08/05 12:53:04]

It's better to use `std::unique_ptr<wchar_t[]>`.

[Eric, 2014/08/05 13:09:10]

Yep. Missed that one.

-    /*
-     * Casting away const here is harmless because we're not using the in-place modification mode of UrlUnescape
-     */
-    HRESULT hr = UrlUnescapeW(const_cast<wchar_t *>(url.c_str()), result.get(), & result_length, 0);

[Oleksandr, 2014/08/05 12:20:58]

Nit: no space after &

[Eric, 2014/08/05 17:53:01]

Done.

+    std::unique_ptr<wchar_t[]> result(new wchar_t[result_length]);
+    HRESULT hr = UrlUnescapeW(const_cast<wchar_t*>(url.c_str()), result.get(), &result_length, 0);
     if (hr == S_OK)
     {
       url = std::wstring(result.get(), result_length);
     }
     /*
-     * If the call to UrlUnescape fails, we don't alter the string.
-     * This matches the behavior of the previous version of this function.
-     * Because there's no error handling, however, this masks failures in UrlUnescape.
+     * Do nothing. This masks error return values from UrlUnescape without logging the error.
+     */
+  }
+  catch(std::bad_alloc e)
+  {
+    /*
+     * When the code has a systematic way of handling bad_alloc, we'll rethrow (probably).
+     * Until then, we mask the exception and make no modification.
      */
   }
   catch(...)
   {

[Oleksandr, 2014/08/05 12:20:58]

I personally am very cautious with empty catch-all exceptions. Maybe at least
let's log something here?

[sergei, 2014/08/05 12:53:04]

+1 here.
Each time such all exception eating construction looks very dangerous.

I've seen Eric's comments above in the code. Let's create a ticket to make a
plan what we need and what we understand under the maintenance of this code. If
there is no big plans of evolving of the code, then it could be OK.

[Eric, 2014/08/05 13:09:10]

Look, I agree with both of you. I do not like blanket suppression of exceptions.
In the present case, however, it is acceptable as an interim measure. At the
worst, it has the same behavior as the previous version of this code, which is
to leave the URL unchanged.

In the general case, if you catch bad_alloc, it's generally time to gracefully
shut down the application or something related. We don't have such a mechanism
other than std::terminate, which would shut down IE as a whole.

I added a ticket such as Sergei wants yesterday.
#1173 Ensure that C++ exceptions do not propagate out of entry points
https://issues.adblockplus.org/ticket/1173
I did a bit of code audit to identify most (probably not all) of the places we
need it. This isn't the entirety of what we need for this task, admittedly. Some
kind of unified logging would also be appropriate. Nevertheless, if it were safe
to throw exceptions in the current code base, I would have left out the
exception handler entirely; bad_alloc is unusual enough that it could be handled
at the top level.

-    // no modification if exception
+    // no modification if any other exception
   }
 }
 
 void CPluginClientBase::LogPluginError(DWORD errorCode, int errorId, int errorSubid, const CString& description, bool isAsync, DWORD dwProcessId, DWORD dwThreadId)
 {
   // Prevent circular references
   if (CPluginSettings::HasInstance() && isAsync)
   {
     DEBUG_ERROR_CODE_EX(errorCode, description, dwProcessId, dwThreadId);
 
@@ skipping 41 matching lines @@
 
       hasError = true;
 
       s_pluginErrors.erase(it);
     }
   }
   s_criticalSectionLocal.Unlock();
 
   return hasError;
 }