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,

[Felix Dahlke, 2014/08/15 15:32:29]

I think this comment should go into the catch block below, something like:
"Ignoring exceptions until the calling code can tolerate them better."

[Eric, 2014/09/29 11:50:07]

OK.

Since I've written this code, I've come to think that this try-catch statement
has a longer lifetime in the code base than I first anticipated. The reason is
that the first thing we'll have is a bare minimum of exception safety, and that
won't include doing something useful in the case of low memory. Eventually we'll
want to rethrow bad_alloc, but (perhaps) not rethrow other exceptions (whatever
they might be; I don't see any others but that doesn't mean they aren't there).

I'll add an explicit catch statement for bad_alloc and put the comment there.

-   *   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.

[Eric, 2014/09/29 11:50:07]

Done.

-     */
-    std::unique_ptr<wchar_t[]> result(new wchar_t[result_length]); // can throw bad_alloc
-    /*
-     * Casting away const here is harmless because we're not using the in-place modification mode of UrlUnescape

[Felix Dahlke, 2014/08/15 15:32:29]

This is pretty obvious from the documentation of UrlUnescape and not worth a
comment IMO.

We're doing this in various places when interacting with WinAPI without
specifically pointing out why the cast is safe.

[Felix Dahlke, 2014/09/30 09:10:28]

We have the exact same problem in several cases - that's the nature of working
with WinAPI. Are you suggesting we should point out that the function we call
won't modify the string we pass in for every const_cast we do for this purpose?
The fact that we do a const_cast already says that it's safe - otherwise we
wouldn't do it. If someone reading the code feels this could be wrong, they can
look up the docs.

But I guess we won't agree. Sergei, your verdict?

[Eric, 2014/09/30 15:22:21]

UrlUnescape is worse than the usual Windows API, because some modes do alter the
string and some do not. While typical of Microsoft, that's just an awful
interface; there's no way to call it cleanly in all cases.

Other API calls, at least, always treat the argument as constant even if they
don't declare it.
A good design never needs const_cast, but the world is not made up of good
designs, so it's a necessary evil. The unfortunate situation, though, is that
most of the uses of const_cast I see in other people's code are a result of
laziness. Therefore personally, I consider every use of const_cast guilty before
proven innocent. It always looks jarring to me; indeed, I added the comment
after I first wrote the call to UrlUnescape because it kept looking wrong to me.

I appreciate the point that const_cast won't enter the code base without review,
but review is not infallible.
Oh, Felix, it takes at least two round-trips to know that this is true.

I would rather commit the code without this comment than to let it linger any
more than it already has, so if I haven't persuaded you with the two points
above, I'm removing it.

[Felix Dahlke, 2014/09/30 16:08:16]

Yeah, that's true. Still - at the end of the day the problem is that WinAPI
can't be arsed about const at all, and we have to const_cast around that.
While the last part is true, we are trying very hard to keep const_cast (and any
kind of casting really) to an absolute minimum. We're not there yet when it
comes to the legacy code in ABP for IE, but the general assumption should be
that at least two people thought about each cast, trying to avoid it. So I don't
think we need to explicitly legitimise casts with comments - people can still
look up the docs to see why it had to be that way.
That's up to you of course :) I still think the comment is pointing out the
obvious and thus hurts more than it helps.

[sergei, 2014/10/06 09:08:54]

I would say that this comment could be here or may be even a little bit changed
like
// Casting away const here is required by the method signature
// for the case of in place modification, despite it's not our case
// and url will not be modified.
HRESULT hr = UrlUnescapeW(
/* src */ const_cast<wchar_t*>(url.c_str()),
/* dst */ result.get(),
/* dst size without terminating '\0' */ &result_length,
/* flags, 0 - don't modify src in place */ 0);

But the current version is also OK.

Firstly, const_cast indeed is attracting the attention, each time I see
const_cast I start to suspect something bad is going here.

Also I don't remember the entire Windows API and for this particular function
each time I have to read its documentation, especially here are attracting
const_cast and magic number 0. Since anyway I personally have to read the
documentation the comment seems to be redundant. It would be great if I don't
need to open the reference of the function or at least it can simplify the
reading if it explains the reason of the decision, so the everything I need is
mere to check whether it's truth or not. BTW, in most cases I would like to
avoid comments for arguments by proper naming.

-     */
-    HRESULT hr = UrlUnescapeW(const_cast<wchar_t *>(url.c_str()), result.get(), &result_length, 0);

[Felix Dahlke, 2014/08/15 15:32:29]

No space between wchar_t and *.

[Eric, 2014/09/29 11:50:07]

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.

[Felix Dahlke, 2014/08/15 15:32:29]

How about changing "the previous version" to "the wrapped version"?

[Eric, 2014/09/29 11:50:07]

I'm moving this comment to the function header, since it's really about the
specification, not the implementation.

[Felix Dahlke, 2014/09/30 09:10:28]

Yeah, good point. I think the comment remaining here is redundant now - it's
quite obvious that we leave the string alone when hr != S_OK.

[Eric, 2014/09/30 15:22:21]

I'll fix the comment again. 

What's not obvious is that when we do nothing we're masking non-trivial error
returns from UrlUnescape. It's a situation that deserves to be logged, however
rare a case it might be.

[Felix Dahlke, 2014/09/30 16:08:16]

Fair enough.

-     * 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(...)
   {
-    // 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;
 }