Issue 1148 - ABP on chrome blocks ads but IE doesnt

src/plugin/PluginFilter.cpp

 #include "PluginStdAfx.h"
 
 #include "PluginFilter.h"
 #include "PluginSettings.h"
 #include "PluginClient.h"
 #include "PluginClientFactory.h"
 #include "PluginMutex.h"
 #include "PluginSettings.h"
 #include "PluginSystem.h"
 #include "PluginClass.h"
 #include "mlang.h"
 
 #include "..\shared\CriticalSection.h"
 #include "..\shared\Utils.h"
 
 
 // The filters are described at http://adblockplus.org/en/filters
 
 static CriticalSection s_criticalSectionFilterMap;
 
 namespace
 {
-  // Returns attribute value and the bool flag which indicates whether the attribute is found.
-  // For the cases like <some-tag on-some-event="do();"/> IE returns the value of 'on-some-event'
-  // attribute as 'function(event){\ndo();\n}'. Our filters are not designed for such
-  // transformations, so this method retrives the value of (body) of /^.*?\{(body)\}$/ as well.
-  std::pair<std::wstring, bool> GetHtmlElementAttribute(IHTMLElement& htmlElement, const ATL::CComBSTR& attributeName)
-  {
-    std::pair<std::wstring, bool> retResult;
-    retResult.second = false;
+  struct GetHtmlElementAttributeResult

[Felix Dahlke, 2014/11/27 13:58:46]

How about "HtmlAttributeMatch" or something in that spirit?

[sergei, 2014/11/27 14:47:25]

I also don't feel it to be a good name, but I find "HtmlAttributeMatch" even
more confusing.

[Felix Dahlke, 2014/11/27 14:57:27]

Fair enough, yeah. Let's go with GetHtmlElementAttributeResult then.

+  {
+    GetHtmlElementAttributeResult() : isAttributeFound(false)
+    {
+    }
+    std::wstring attributeValue;
+    bool isAttributeFound;
+  };
+
+  GetHtmlElementAttributeResult GetHtmlElementAttribute(IHTMLElement& htmlElement,
+    const ATL::CComBSTR& attributeName)
+  {
+    GetHtmlElementAttributeResult retValue;
     ATL::CComVariant vAttr;
-    // Performs a property search that is not case-sensitive,
-    // and returns an interpolated value if the property is found.
-    LONG flags = 0;
-    if (FAILED(htmlElement.getAttribute(attributeName, flags, &vAttr)))
-    {
-      return retResult;
+    ATL::CComPtr<IHTMLElement4> htmlElement4;
+    if (FAILED(htmlElement.QueryInterface(&htmlElement4)) || !htmlElement4)
+    {
+      return retValue;
+    }
+    ATL::CComPtr<IHTMLDOMAttribute> attributeNode;
+    if (FAILED(htmlElement4->getAttributeNode(attributeName, &attributeNode)) || !attributeNode)
+    {
+      return retValue;
     }
     // we set that attribute found but it's not necessary that we can retrieve its value
-    retResult.second = true;
+    retValue.isAttributeFound = true;
+    if (FAILED(attributeNode->get_nodeValue(&vAttr)))
+    {
+      return retValue;
+    }
     if (vAttr.vt == VT_BSTR && vAttr.bstrVal)
     {
-      retResult.first.assign(vAttr.bstrVal);
+      retValue.attributeValue = vAttr.bstrVal;
     }
     else if (vAttr.vt == VT_I4)
     {
-      retResult.first = std::to_wstring(vAttr.iVal);
-    }
-    else if (vAttr.vt == VT_DISPATCH)
-    {
-      wchar_t* toStringMethod = L"toString";
-      DISPID methodId = 0;
-      if (FAILED(vAttr.pdispVal->GetIDsOfNames(/*must be null iid*/IID_NULL, &toStringMethod,
-        /*cNames*/1, LOCALE_SYSTEM_DEFAULT, &methodId)))
-      {
-        return retResult;
-      }
-      ATL::CComVariant variantFunctionAsString; // result of IDispatch::Invoke
-      DISPID dispidNamed = DISPATCH_METHOD | DISPATCH_PROPERTYGET;
-      DISPPARAMS dispparams;
-      dispparams.rgvarg = &variantFunctionAsString;
-      dispparams.cArgs = 1;
-      dispparams.cNamedArgs = 1;
-      dispparams.rgdispidNamedArgs = &dispidNamed;
-      UINT ArgErr = 0;
-      if (FAILED(vAttr.pdispVal->Invoke(methodId, /*must be null iid*/IID_NULL,
-        LOCALE_SYSTEM_DEFAULT, dispidNamed, &dispparams, &variantFunctionAsString, /*exception info*/nullptr,
-        &ArgErr)))
-      {
-        return retResult;
-      }
-      if (variantFunctionAsString.vt != VT_BSTR)
-      {
-        return retResult;
-      }
-      std::wstring functionAsString = variantFunctionAsString.bstrVal;
-      auto bodyBeginsAt = functionAsString.find(L'{');
-      if (bodyBeginsAt == std::wstring::npos)
-      {
-        return retResult;
-      }
-      // eat spaces
-      while (::iswspace(functionAsString[++bodyBeginsAt]));
-      auto bodyEndsAt = functionAsString.rfind(L'}');
-      if (bodyEndsAt == std::wstring::npos || bodyEndsAt < bodyBeginsAt)
-      {
-        return retResult;
-      }
-      // eat spaces
-      while(::iswspace(functionAsString[--bodyEndsAt]));
-      retResult.first = functionAsString.substr(bodyBeginsAt, bodyEndsAt - bodyBeginsAt + 1);
-    }
-    return retResult;
+      retValue.attributeValue = std::to_wstring(vAttr.iVal);
+    }
+    return retValue;
   }
 }
 
 // ============================================================================
 // CFilterElementHideAttrSelector
 // ============================================================================
 
 CFilterElementHideAttrSelector::CFilterElementHideAttrSelector() : m_type(TYPE_NONE), m_pos(POS_NONE), m_bstrAttr(NULL)
 {
 }
@@ skipping 250 matching lines @@
     if ((hr != S_OK) || (tagName != CComBSTR(m_tag)))
     {
       return false;
     }
   }
 
   // Check attributes
   for (std::vector<CFilterElementHideAttrSelector>::const_iterator attrIt = m_attributeSelectors.begin(); 
         attrIt != m_attributeSelectors.end(); ++ attrIt)
   {
     ATL::CString value;

[Felix Dahlke, 2014/11/27 13:58:46]

This change seems unrelated, hm? Plus we're still using CString (without ATL::)
elsewhere in this file.

[sergei, 2014/11/27 14:47:25]

Yes, it's not important here, just was irritating the eye. I would not say that
we don't use it in this file elsewhere is an argument. We should tend to use
ATL:: for the same reason that we don't have `using namespace std` in any
header.

[Felix Dahlke, 2014/11/27 14:57:27]

Yes I agree. It's just that we should avoid unrelated changes, particularly with
bug fixes. A commit should make one logical change. That way we can trace down
regressions and back commits out much easier.

     bool attrFound = false;
     if (attrIt->m_type == CFilterElementHideAttrType::STYLE)
     {
       CComPtr<IHTMLStyle> pStyle;
       if (SUCCEEDED(pEl->get_style(&pStyle)) && pStyle)
       {
         CComBSTR bstrStyle;
 
         if (SUCCEEDED(pStyle->get_cssText(&bstrStyle)) && bstrStyle)
         {
@@ skipping 16 matching lines @@
     {
       CComBSTR bstrId;
       if (SUCCEEDED(pEl->get_id(&bstrId)) && bstrId)
       {
         value = bstrId;
         attrFound = true;
       }
     }
     else
     {
-      auto attribute = GetHtmlElementAttribute(*pEl, attrIt->m_bstrAttr);
-      if (attrFound = attribute.second)
-      {
-        value = ToCString(attribute.first);
+      auto attributeValue = GetHtmlElementAttribute(*pEl, attrIt->m_bstrAttr);
+      if (attrFound = attributeValue.isAttributeFound)
+      {
+        value = ToCString(attributeValue.attributeValue);
       }
     }
 
     if (attrFound)
     {
       if (attrIt->m_pos == CFilterElementHideAttrPos::EXACT)
       {
         // TODO: IE rearranges the style attribute completely. Figure out if anything can be done about it.
         if (value != attrIt->m_value)
           return false;
@@ skipping 67 matching lines @@
 
 
 // ============================================================================
 // CPluginFilter
 // ============================================================================
 
 CPluginFilter::CPluginFilter(const CString& dataPath) : m_dataPath(dataPath)
 {
   m_contentMapText[CFilter::contentTypeDocument] = "DOCUMENT";
   m_contentMapText[CFilter::contentTypeObject] = "OBJECT";
-  m_contentMapText[CFilter::contentTypeObjectSubrequest] = "OBJECT-SUBREQUEST";
+  m_contentMapText[CFilter::contentTypeObjectSubrequest] = "OBJECT_SUBREQUEST";
   m_contentMapText[CFilter::contentTypeImage] = "IMAGE";
   m_contentMapText[CFilter::contentTypeScript] = "SCRIPT";
   m_contentMapText[CFilter::contentTypeOther] = "OTHER";
   m_contentMapText[CFilter::contentTypeUnknown] = "OTHER";
   m_contentMapText[CFilter::contentTypeSubdocument] = "SUBDOCUMENT";
   m_contentMapText[CFilter::contentTypeStyleSheet] = "STYLESHEET";
   m_contentMapText[CFilter::contentTypeXmlHttpRequest] = "XMLHTTPREQUEST";
 
   ClearFilters(); 
 }
@@ skipping 268 matching lines @@
         CPluginDebug::DebugResultBlocking(type, srcCString, domain);
 #endif
     }
     return true;
   }
 #ifdef ENABLE_DEBUG_RESULT
   CPluginDebug::DebugResultIgnoring(type, srcCString, domain);
 #endif
   return false;
 }