Issue #1466 - fix usage of BSTR

src/plugin/AdblockPlusDomTraverser.cpp

Index: src/plugin/AdblockPlusDomTraverser.cpp
===================================================================
--- a/src/plugin/AdblockPlusDomTraverser.cpp
+++ b/src/plugin/AdblockPlusDomTraverser.cpp
@@ -49,7 +49,7 @@
   {
     CComVariant vAttr;
 
-    if (SUCCEEDED(pEl->getAttribute(L"src", 0, &vAttr)) && vAttr.vt == VT_BSTR && ::SysStringLen(vAttr.bstrVal) > 0)
+    if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ::SysStringLen(vAttr.bstrVal) > 0)

[Eric, 2014/10/09 13:54:01]

As I said in the previous review, I object to using CComBSTR here because it
throws an ATL exception. In particular, it throws an ATL exception
(CAtlException) that contains a result code that needs to be separately parsed
which can direct the construction of the correct standard library exception. The
upshot is that handling ATL errors in the context of handling standard library
exception means that you (1) need a separate catch clause for each try-catch
statement that contains (2) a call to a function that examines the ATL exception
and rethrows a standard library exception. That's four extra lines of purely
repeated code in every exception handler.

It's a much better design to use a class here that simply throws std::bad_alloc.
One possibility is to write a wrapper class around CComBSTR that catches the ATL
exception and rethrows std::bad_alloc. Or even easier, just use the class I have
already written for the specific purpose of passing BSTR to IE COM functions. It
throws std::bad_alloc when it has an allocation error. See
http://codereview.adblockplus.org/5140323101573120/

[sergei, 2014/10/16 08:53:25]

- I would say that speaking about maintainability I am completely sure that ATL
is much better than our custom classes. There is a plenty of documentation and
it's proved by huge usage. As well if I understood correctly we decided to stick
with ATL.
So, since we use ATL and we definitely cannot control all cases when it can
throw an exception I would say that it should not be considered as something
avoidable. Instead of counting lines of code we should think how to implement it
in a good way. For example, we can make each entry method (see #1173) like
HRESULT SomeClassImpl::SomeMethod(arg1, arg2, ...)
{
  return catchExceptions([]()->HRESULT
  {
    // our current impl
  });
}

I don't see a big problem in several lines of code to catch additional ATL
exception inside `catchExceptions`. Even more, ATL exception allows to return
the real HRESULT instead of manual mapping each exception to HRESULT.
- I am also convinced that we should not pollute the code by try-catch
statements, it would be better to have it centralized.
- As well as I don't know how to compare what is worse right now, wrong usage of
wchar_t* strings instead of BSTR or propagating of exceptions in case of there
is no enough free memory.

Conclusion: use ATL, implement exception handling as #1173.
@Oleksandr, @Felix.

[Oleksandr, 2014/10/17 07:38:02]

- I agree that using ATL's class is better then our custom class. We will need
to catch that exception either way, so it's not like it would result in cleaner
code.
- I already admitted that I would be fine if we did not handle that exception at
all for now (that is really a very extreme case when it would be thrown, IMHO,
and crashing wouldn't be the worst option in that case), but if we do, Sergei's
suggestion with catchExceptions makes sense to me.
 
On 2014/10/16 08:53:25, sergei wrote:

[Eric, 2014/10/20 04:06:24]

The main way that the code becomes cleaner by using our own class is that we can
define a conversion function to std::wstring.

[Eric, 2014/10/20 04:06:24]

What we decided was that there was little rush to continue the process of
removing ATL. That is not at all the same thing as sticking with it. What we
really do not need is to use _more_ ATL.

[Eric, 2014/10/20 04:06:24]

From what I can tell, I don't think you spent any significant time even reading
the code I posted for them, neither the first iteration nor the second. I
certainly have seen little indication that you know or appreciate the range of
issues I've been concerned with in these classes.
I've studied the issue, and we can control the cases where ATL might throw. 

In the following, I'm excluding PassthroughAPP. Even though it has numerous
occurrences of ATLEXCEPTION and ATLTRACE, its code calls ours, not vice-versa,
so it's not our primary concern.

For the vast majority of our own code, the only ATL we're using is the two
classes CComBSTR and CComVariant.  I've studied the source code to CComBSTR and
also audited the context of how we use it. I am reasonably certain that the only
exception that CComBSTR can throw from within our code is for a failure to
allocate memory. Similarly, the only ones that CComVariant could throw are
out-of-memory and some outlier cases where an IE defect would return an
incorrect type. Most all of the uses of CComPtr don't throw (unless for some
bizarre reason AddRef throws). There are a few simple uses of CComQIPtr; these
would only throw in practice if IE returned an object of invalid type.

Other than these value-classes, the only other cases I know of that can throw
ATL exceptions are the places we're using default ATL implementations of certain
interfaces. These are 'IObjectWithSiteImpl' and 'IDispatchImpl'. There is only
one possible occurrence where we'd ever see an ATL exception, and it's in a call
to 'IObjectWithSiteImpl<CPluginClass>::SetSite'.

This is not an insurmountable problem.
This implementation pattern requires that all exceptions are handled identically
in all cases. There is an assumption here that all exceptions _should_ be
handled identically. I am very certain I am not ready to make that assumption at
the current time. I suspect we may only need a handful of exception patterns,
but that number is in my present opinion very likely to be greater than one.

I'm not opposed to considering the use of such a pattern once we understand
better what the commonalities are. We don't understand them yet.
This claim has false assumptions. For exceptions thrown by the standard library,
there is no such thing as a "real HRESULT". For exceptions thrown by systems
calls, we can define a subclass of 'std::runtime_exception' that not only
captures the HRESULT but also always calls 'GetLastError' for good measure.
Centralization is acceptable if you know you need common behavior. We do not yet
know that.

I have to say that I have a distaste for the idea that try-catch statements are
somehow a pollution. It's possible to overuse them, to be sure. There are plenty
of places in the code where error handling is non-existent or rather crude.
Whether such error handling gets expressed in a try-catch block or by explicit
checking of return values is less important than that the code handle the error
correctly. As an example, the current code does a poor job dealing with mutex
failures. 

[Eric, 2014/10/20 04:06:24]

It is possible to define a class for the kind of effectively-constant BSTR
arguments we're using here. There's no such thing as an actually-constant BSTR,
since they're all required to be system-allocated. Among other things, it means
you can't take advantage of 'constexpr' to define the BSTR memory layout at
compile time. (It probably even works today, but there's no guarantee it will
work tomorrow.) Nevertheless, there are ways of dealing with it.

One way is to use a global static variable. This moves the exception from
run-time to load-time. This is an improvement, since there's more likely to be
free memory when IE starts up than after there are two dozen tabs open. The
problem with this is that an exception during static initialization doesn't have
a handler.

Better is to use local variable for a singleton class that allocates on first
use but not thereafter. It means checking a flag each time the value is
referenced, but that's faster than allocating and copying the constant string
each time. You still a low-memory exception to handle.

At the cost of almost-certainly-too-much code overhead, you could do all your
allocations on the first call to DllMain. That gets rid of the exception, but I
doubt it's worth it.

The upshot is that I don't see a practical way to eliminate dealing with the
memory exception. This is, however, not really a problem that's unique here.
We've got a number of dynamic memory structures that could throw the same
exception as a result of processing pages. We're going to have to deal with it
anyway. My main concern with respect to this particular change set is that we
not put ourselves in a position of need to deal with multiple kinds of memory
allocation exceptions.

[Oleksandr, 2014/10/21 23:53:19]

I really don't think we should be having prolonged discussions about this. As I
understand Eric is only concerned that CComBSTR will throw an ATL exception, and
that would require extra 'catch' blocks. 
If that's the only concern, then can't we just override AtlThrow as discribed
here:
http://msdn.microsoft.com/en-us/library/z325eyx0.aspx
and then throw whatever we want from there? We can add that whenever we figure
out what to actually do with those exceptions.

And to iterate again, I really don't think we should go out of our way to
minimize the chances of memory allocation exception, seeing there's no way
around it anyway. Those chances are already extremely slim. And even if
situation like that ever occurs on user's machine I wouldn't be too sure IE
itself wouldn't just crash before we even get to allocate anything. 

On 2014/10/20 04:06:24, Eric wrote:
We have in fact decided that we will stick with CComBSTR. So we shouldn't be shy
of actually using it.

     {
       CString src = vAttr.bstrVal;
       CPluginClient::UnescapeUrl(src);