Issue #1234 - Replace CString in the traverser

src/plugin/AdblockPlusDomTraverser.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "PluginStdAfx.h"
 #include "AdblockPlusDomTraverser.h"
 #include "AdblockPlusClient.h"
 #include "PluginFilter.h"
 #include "PluginSettings.h"
+#include "..\shared\Utils.h"
 
 
 CPluginDomTraverser::CPluginDomTraverser(CPluginTab* tab) : CPluginDomTraverserBase(tab)
 {
 }
 
 
-bool CPluginDomTraverser::OnIFrame(IHTMLElement* pEl, const std::wstring& url, CString& indent)
+bool CPluginDomTraverser::OnIFrame(IHTMLElement* pEl, const std::wstring& url, const std::wstring& indent)
 {
   CPluginClient* client = CPluginClient::GetInstance();
 
   // If src should be blocked, set style display:none on iframe
   bool isBlocked = client->ShouldBlock(url,
     AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_SUBDOCUMENT, m_domain);
   if (isBlocked)
   {
-    HideElement(pEl, "iframe", url, true, indent);
+    HideElement(pEl, L"iframe", url, true, indent);
   }
 
   return !isBlocked;
 }
 
 
-bool CPluginDomTraverser::OnElement(IHTMLElement* pEl, const CString& tag, CPluginDomTraverserCache* cache, bool isDebug, CString& indent)
+bool CPluginDomTraverser::OnElement(IHTMLElement* pEl, const std::wstring& tag, CPluginDomTraverserCache* cache, bool isDebug, const std::wstring& indent)
 {
   if (cache->m_isHidden)
   {
     return false;
   }
 
   // Check if element is hidden
   CPluginClient* client = CPluginClient::GetInstance();
 
-  cache->m_isHidden = client->IsElementHidden(ToWstring(tag), pEl, m_domain, ToWstring(indent), m_tab->m_filter.get());
+  cache->m_isHidden = client->IsElementHidden(tag, pEl, m_domain, indent, m_tab->m_filter.get());
   if (cache->m_isHidden)
   {
     HideElement(pEl, tag, L"", false, indent);
     return false;
   }
 
   // Images
-  if (tag == "img")
+  if (tag == L"img")
   {
     CComVariant vAttr;
 
     if (SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"src"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ::SysStringLen(vAttr.bstrVal) > 0)
     {
       std::wstring src(vAttr.bstrVal, SysStringLen(vAttr.bstrVal));
 
       // If src should be blocked, set style display:none on image
       cache->m_isHidden = client->ShouldBlock(src,
         AdblockPlus::FilterEngine::ContentType::CONTENT_TYPE_IMAGE, m_domain);
       if (cache->m_isHidden)
       {
-        HideElement(pEl, "image", src, true, indent);
+        HideElement(pEl, L"image", src, true, indent);
         return false;
       }
     }
   }
   // Objects
-  else if (tag == "object")
+  else if (tag == L"object")
   {
     CComBSTR bstrInnerHtml;
-
     if (SUCCEEDED(pEl->get_innerHTML(&bstrInnerHtml)) && bstrInnerHtml)
     {
-      CString sObjectHtml = bstrInnerHtml;
-      CString src;
-
-      int posBegin = sObjectHtml.Find(L"VALUE=\"");
-      int posEnd = posBegin >= 0 ? sObjectHtml.Find('\"', posBegin + 7) : -1;
-
-      while (posBegin >= 0 && posEnd >= 0)
+      const std::wstring objectInnerHtml(ToWstring(bstrInnerHtml));
+      std::wstring::size_type posBegin = 0;
+      while (true)
       {
-        posBegin += 7;
-
-        src = sObjectHtml.Mid(posBegin, posEnd - posBegin);
-
+        posBegin = objectInnerHtml.find(L"VALUE=\"", posBegin);

[sergei, 2015/12/14 12:17:55]

It's not related to this issue, but shouldn't we perform case insensitive search
for "value" attribute?

[Eric, 2015/12/14 12:40:23]

If we change it, we shouldn't be parsing the innerHTML attribute at all. The
"value" attribute should be appearing on a <param> tag inside the present
<object> tag. It would be more reliable to use DOM calls here.

When I saw this, I assumed that IE must be normalizing the innerHTML of <object>
elements. Given that we don't have a rash of reports of not blocking Flash etc.,
I'm assuming that this behavior is consistent.

I'm not inclined to change this, since we have bigger issues to worry about. If
you'd like to create a ticket for it, though, it does seem like a valid concern.

[Oleksandr, 2015/12/14 12:45:34]

It wouldn't hurt, but it doesn't depend on how it is written in the HTML source.
get_innerHTML normalizes HTML for us.

[sergei, 2015/12/14 13:28:02]

Yes, I thought about it too. The current approach seems to be a result of trade
between something simple and enough good working, and more sophisticated proper
solution but without real gain.

[sergei, 2015/12/14 13:28:02]

Could you provide with a reference for it? I've tried it in javascript and could
not reproduce automatic conversion to uppercase of innerHTML property.

+        if (posBegin == std::wstring::npos)
+        {
+          // No more "value" attributes to scan
+          break;
+        }
+        auto posPostInitialQuote = posBegin + 7;
+        auto posFinalQuote = objectInnerHtml.find(L'\"', posPostInitialQuote);
+        if (posFinalQuote == std::wstring::npos)
+        {
+          // We have an initial quotation mark but no final one.
+          // Ignore this tag because it has an HTML syntax error.
+          break;
+        }
+        auto src = objectInnerHtml.substr(posPostInitialQuote, posFinalQuote - posPostInitialQuote);
         // eg. http://w3schools.com/html/html_examples.asp
-        if (src.Left(2) == "//")
+        if (BeginsWith(src, L"//"))
         {
-          src = "http:" + src;
+          src = L"http:" + src;
         }
-
-        if (!src.IsEmpty())
+        if (!src.empty() && cache->m_isHidden)
         {
-          if (cache->m_isHidden)
-          {
-            HideElement(pEl, "object", ToWstring(src), true, indent);
-            return false;
-          }
+          HideElement(pEl, L"object", src, true, indent);
+          return false;
         }
-
-        posBegin = sObjectHtml.Find(L"VALUE=\"", posBegin);
-        posEnd = posBegin >= 0 ? sObjectHtml.Find(L"\"", posBegin + 7) : -1;
+        posBegin = posFinalQuote; // Don't scan content of quoted string
       }
     }
   }
 
   return true;
 }
 
 
 bool CPluginDomTraverser::IsEnabled()
 {
   CPluginClient* client = CPluginClient::GetInstance();
   return client && CPluginSettings::GetInstance()->IsPluginEnabled() && !client->IsWhitelistedUrl(m_domain);
 }
 
 
-void CPluginDomTraverser::HideElement(IHTMLElement* pEl, const CString& type, const std::wstring& url, bool isDebug, CString& indent)
+void CPluginDomTraverser::HideElement(IHTMLElement* pEl, const std::wstring& type, const std::wstring& url, bool isDebug, const std::wstring& indent)
 {
   CComPtr<IHTMLStyle> pStyle;
 
   if (SUCCEEDED(pEl->get_style(&pStyle)) && pStyle)
   {
     CComBSTR bstrDisplay;
-
-    if (SUCCEEDED(pStyle->get_display(&bstrDisplay)) && bstrDisplay && CString(bstrDisplay) == L"none")
+    if (SUCCEEDED(pStyle->get_display(&bstrDisplay)) && bstrDisplay && ToWstring(bstrDisplay) == L"none")
     {
       return;
     }
 
     static const CComBSTR sbstrNone(L"none");
 
     if (SUCCEEDED(pStyle->put_display(sbstrNone)))
     {
       DEBUG_HIDE_EL(ToWstring(indent) + L"HideEl::Hiding " + ToWstring(type) + L" url:" + url)
 
 #ifdef ENABLE_DEBUG_RESULT
         if (isDebug)
         {
-          CPluginDebug::DebugResultHiding(ToWstring(type), url, L"-");
+          CPluginDebug::DebugResultHiding(type, url, L"-");
         }
 #endif // ENABLE_DEBUG_RESULT
     }
   }
 }