Issue 1283 - wrong usage of memset, fix sizeof, make proper initializing

src/plugin/PluginDomTraverserBase.h

 #ifndef _PLUGIN_DOM_TRAVERSER_BASE_H_
 #define _PLUGIN_DOM_TRAVERSER_BASE_H_
 
 
 #include "PluginTypedef.h"
 #include "PluginTab.h"
 
 
 class CPluginDomTraverserCacheBase
 {
@@ skipping 14 matching lines @@
   CPluginDomTraverserBase(CPluginTab* tab);
   ~CPluginDomTraverserBase();
 
   void TraverseHeader(bool isHeaderTraversed);
 
   void TraverseDocument(IWebBrowser2* pBrowser, const CString& domain, const CString& documentName);
   void TraverseSubdocument(IWebBrowser2* pBrowser, const CString& domain, const CString& documentName);
 
   virtual void ClearCache();
 
-  void ShowNotification(CPluginTab* tab);
+  void ShowNotification(CPluginTab* /*tab*/){}

[Eric, 2014/09/26 16:36:40]

ShowNotification is completely dead code. The symbol doesn't appear anywhere
else in the code. Even this stub declaration can be deleted.

[Oleksandr, 2014/10/06 20:19:32]

+1
On 2014/09/26 16:36:40, Eric wrote:

 
 protected:
 
   virtual bool OnIFrame(IHTMLElement* pEl, const CString& url, CString& indent) { return true; }
   virtual bool OnElement(IHTMLElement* pEl, const CString& tag, T* cache, bool isDebug, CString& indent) { return true; }
 
   virtual bool IsEnabled();
 
 protected:
 
   void TraverseDocument(IWebBrowser2* pBrowser, bool isMainDoc, CString indent);
   void TraverseChild(IHTMLElement* pEl, IWebBrowser2* pBrowser, CString& indent, bool isCached=true);
 
+  typedef ATL::CComCritSecLock<CComAutoCriticalSection> CriticalSectionLock;

[Eric, 2014/10/08 16:26:55]

"Sentry" is the word that Stroustrup uses for these kinds of classes. In "The
C++ Programming Language", he talks about them in conjunction with I/O classes.

I have come to dislike the promiscuous use of the word "lock" for almost
everything related to multiprocessing access control. There are lots of things
it can mean, and it's frequently confusing. When there's a more specific word
available, I prefer that.
I would prefer standard library mutexes over ATL mutexes, for two basic reasons:
(1) it reduces the dependency upon ATL and (2) it uses standard library
exceptions rather than the ATL-specific exceptions. Swapping these classes out
doesn't need to be done as a separate project, but we can do it incrementally
when we touch the code. I've added issue #1467 for this.

   CComAutoCriticalSection m_criticalSection;
 
   CString m_domain;
   CString m_documentName;
 
   bool m_isHeaderTraversed;
 
-  // Caching    
-  long m_cacheDomElementCount;
-
-  int m_cacheIndexLast;
-  int m_cacheElementsMax;
   std::set<CString> m_cacheDocumentHasFrames;
   std::set<CString> m_cacheDocumentHasIframes;
 
-  T* m_cacheElements;
+  std::vector<T> m_cacheElements;
 
   CPluginTab* m_tab;
   CComPtr<IWebBrowser2> m_pBrowser;
 };
 
 template <class T>
 CPluginDomTraverserBase<T>::CPluginDomTraverserBase(CPluginTab* tab) : 
-  m_tab(tab), m_isHeaderTraversed(false), m_cacheDomElementCount(0), m_cacheIndexLast(0), m_cacheElementsMax(5000)
+  m_tab(tab), m_isHeaderTraversed(false)
 {
-  m_cacheElements = new T[m_cacheElementsMax];
+  m_cacheElements.reserve(5000);
 }
 
-
 template <class T>
 CPluginDomTraverserBase<T>::~CPluginDomTraverserBase()
 {
-  delete [] m_cacheElements;
 }
 
 template <class T>
 void CPluginDomTraverserBase<T>::TraverseHeader(bool isHeaderTraversed)
 {
   m_isHeaderTraversed = isHeaderTraversed;
 }
 
 template <class T>
 void CPluginDomTraverserBase<T>::TraverseDocument(IWebBrowser2* pBrowser, const CString& domain, const CString& documentName)
 {
   m_domain = domain;
-
   TraverseDocument(pBrowser, true, "");
 }
 
 
 template <class T>
 void CPluginDomTraverserBase<T>::TraverseSubdocument(IWebBrowser2* pBrowser, const CString& domain, const CString& documentName)
 {
   m_domain = domain;
-
   TraverseDocument(pBrowser, false, "");
 }
 
 
 template <class T>
 bool CPluginDomTraverserBase<T>::IsEnabled()
 {
   return CPluginSettings::GetInstance()->IsPluginEnabled();
 }
 
 
 template <class T>
 void CPluginDomTraverserBase<T>::TraverseDocument(IWebBrowser2* pBrowser, bool isMainDoc, CString indent)
 {
   DWORD res = WaitForSingleObject(m_tab->m_filter->hideFiltersLoadedEvent, ENGINE_STARTUP_TIMEOUT);
   if (!IsEnabled()) return;
 
-  VARIANT_BOOL isBusy;    
-  if (SUCCEEDED(pBrowser->get_Busy(&isBusy)))
+  VARIANT_BOOL isBusy;
+  if (FAILED(pBrowser->get_Busy(&isBusy)) || isBusy)
   {
-    if (isBusy)
-    {
-      return;
-    }
+    return;
   }
 
   // Get document
   CComPtr<IDispatch> pDocDispatch;
   HRESULT hr = pBrowser->get_Document(&pDocDispatch);
   if (FAILED(hr) || !pDocDispatch)
   {
     return;
   }
 
@@ skipping 11 matching lines @@
 
   CComPtr<IHTMLElement> pBodyEl;
 
   if (m_isHeaderTraversed)
   {
     pBodyEl = pBody;
   }
   else
   {
     CComPtr<IHTMLElementCollection> pBodyCollection;
-    if (FAILED(pDoc->getElementsByTagName(L"body", &pBodyCollection)) || !pBodyCollection)
+    if (FAILED(pDoc->getElementsByTagName(ATL::CComBSTR(L"body"), &pBodyCollection)) || !pBodyCollection)

[Oleksandr, 2014/10/06 20:19:32]

I would be willing to have this committed without the try-catch'es for low
memory cases. I actually think just crashing in that situation is not the worst
thing to do.

On 2014/09/26 16:36:40, Eric wrote:

     {
       return;
     }
 
     CComVariant vIndex(0);        
     CComPtr<IDispatch> pBodyDispatch;
     if (FAILED(pBodyCollection->item(vIndex, vIndex, &pBodyDispatch)) || !pBodyDispatch)
     {
       return;
     }
 
     if (FAILED(pBodyDispatch->QueryInterface(IID_IHTMLElement, (LPVOID*)&pBodyEl)) || !pBodyEl)
     {
       return;
     }
   }
 
   // Clear cache (if eg. refreshing) ???
   if (isMainDoc)
   {
     CComVariant vCacheIndex;
 
-    if (FAILED(pBodyEl->getAttribute(L"abp", 0, &vCacheIndex)) || vCacheIndex.vt == VT_NULL)
+    if (FAILED(pBodyEl->getAttribute(ATL::CComBSTR(L"abp"), 0, &vCacheIndex)) || vCacheIndex.vt == VT_NULL)
     {
       ClearCache();
     }
   }
 
   // Hide elements in body part
   TraverseChild(pBodyEl, pBrowser, indent);
 
   // Check frames and iframes
   bool hasFrames = false;
   bool hasIframes = false;
 
   m_criticalSection.Lock();
   {
     hasFrames = m_cacheDocumentHasFrames.find(m_documentName) != m_cacheDocumentHasFrames.end();
     hasIframes = m_cacheDocumentHasIframes.find(m_documentName) != m_cacheDocumentHasIframes.end();
   }
   m_criticalSection.Unlock();
 
   // Frames
   if (hasFrames)
   {
     // eg. http://gamecopyworld.com/
     long frameCount = 0;
     CComPtr<IHTMLElementCollection> pFrameCollection;
-    if (SUCCEEDED(pDoc->getElementsByTagName(L"frame", &pFrameCollection)) && pFrameCollection)
+    if (SUCCEEDED(pDoc->getElementsByTagName(ATL::CComBSTR(L"frame"), &pFrameCollection)) && pFrameCollection)
     {
       pFrameCollection->get_length(&frameCount);
     }
 
     // Iterate through all frames
     for (long i = 0; i < frameCount; i++)
     {
       CComVariant vIndex(i);
       CComVariant vRetIndex;
       CComPtr<IDispatch> pFrameDispatch;
@@ skipping 19 matching lines @@
         }
       }
     }
   }
 
   // Iframes
   if (hasIframes)
   {
     long frameCount = 0;
     CComPtr<IHTMLElementCollection> pFrameCollection;
-    if (SUCCEEDED(pDoc->getElementsByTagName(L"iframe", &pFrameCollection)) && pFrameCollection)
+    if (SUCCEEDED(pDoc->getElementsByTagName(ATL::CComBSTR(L"iframe"), &pFrameCollection)) && pFrameCollection)
     {
       pFrameCollection->get_length(&frameCount);
     }
 
     // Iterate through all iframes
     for (long i = 0; i < frameCount; i++)
     {
       CComVariant vIndex(i);
       CComVariant vRetIndex;
       CComPtr<IDispatch> pFrameDispatch;
 
       if (SUCCEEDED(pFrameCollection->item(vIndex, vRetIndex, &pFrameDispatch)) && pFrameDispatch)
       {
         CComQIPtr<IHTMLElement> pFrameEl = pFrameDispatch;
         if (pFrameEl)
         {
           CComVariant vAttr;
-
-          if (SUCCEEDED(pFrameEl->getAttribute(L"src", 0, &vAttr)) && vAttr.vt == VT_BSTR && ::SysStringLen(vAttr.bstrVal) > 0)
+          if (SUCCEEDED(pFrameEl->getAttribute(ATL::CComBSTR(L"scr"), 0, &vAttr)) && vAttr.vt == VT_BSTR && ::SysStringLen(vAttr.bstrVal) > 0)
           {
             CString src = vAttr.bstrVal;
 
             // Some times, domain is missing. Should this be added on image src's as well?''
 
             // eg. gadgetzone.com.au
             if (src.Left(2) == L"//")
             {
               src = L"http:" + src;
             }
@@ skipping 18 matching lines @@
         }
       }
     }
   }
 }
 
 
 template <class T>
 void CPluginDomTraverserBase<T>::TraverseChild(IHTMLElement* pEl, IWebBrowser2* pBrowser, CString& indent, bool isCached)
 {
-  int  cacheIndex = -1;
+  uint32_t cacheIndex = -1;
   long cacheAllElementsCount = -1;
-
-  m_criticalSection.Lock();
   {
+    CriticalSectionLock lock(m_criticalSection);
     CComVariant vCacheIndex;
-    if (isCached && SUCCEEDED(pEl->getAttribute(L"abp", 0, &vCacheIndex)) && vCacheIndex.vt == VT_I4)
+    if (isCached
+      && SUCCEEDED(pEl->getAttribute(ATL::CComBSTR(L"abp"), 0, &vCacheIndex))
+      && SUCCEEDED(vCacheIndex.ChangeType(VT_UI4)))
     {
-      cacheIndex = vCacheIndex.intVal;
-
+      cacheIndex = vCacheIndex.uintVal;
+      // right now the user can set arbitrary value of abp attribute which in the best case will
+      // cause the crash. Test cacheIndex belongs to [0, m_cacheElements.size())
+      if (cacheIndex >= m_cacheElements.size())
+      {
+        return;

[Oleksandr, 2014/10/06 20:19:32]

I agree. This makes it very easy for someone to disable ABP on a page.
On 2014/09/26 16:36:40, Eric wrote:
Let's not go that far :) Usual debug log would be enough, IMHO.

+      }
       cacheAllElementsCount = m_cacheElements[cacheIndex].m_elements;
     }
     else
     {
       isCached = false;
 
-      cacheIndex = m_cacheIndexLast++;
-
-      // Resize cache???            
-      if (cacheIndex >= m_cacheElementsMax)
-      {
-        T* oldCacheElements = m_cacheElements;
-
-        m_cacheElements = new T[2*m_cacheElementsMax];
-
-        memcpy(m_cacheElements, oldCacheElements, m_cacheElementsMax*sizeof(T));
-
-        m_cacheElementsMax *= 2;
-
-        delete [] oldCacheElements;                
-      }
-
+      // it's important to set cacheIndex before extending m_cacheElements because it's zero based.
+      cacheIndex = m_cacheElements.size();
+      m_cacheElements.emplace_back(T());
       m_cacheElements[cacheIndex].Init();
 
-      vCacheIndex.vt = VT_I4;
-      vCacheIndex.intVal = cacheIndex;
-
-      pEl->setAttribute(L"abp", vCacheIndex);
+      vCacheIndex = cacheIndex;
+      pEl->setAttribute(ATL::CComBSTR(L"abp"), vCacheIndex);
     }
   }
-  m_criticalSection.Unlock();
 
   // Get number of elements in the scope of pEl
   long allElementsCount = 0;
 
   CComPtr<IDispatch> pAllCollectionDisp;
 
   if (SUCCEEDED(pEl->get_all(&pAllCollectionDisp)) && pAllCollectionDisp)
   {
     CComPtr<IHTMLElementCollection> pAllCollection;
 
     if (SUCCEEDED(pAllCollectionDisp->QueryInterface(IID_IHTMLElementCollection, (LPVOID*)&pAllCollection)) && pAllCollection)
     {
       // If number of elements = cached number, return
       if (SUCCEEDED(pAllCollection->get_length(&allElementsCount)) && allElementsCount == cacheAllElementsCount)
       {
         return;
       }
     }
   }
 
   // Update cache

[Eric, 2014/10/08 16:26:55]

Yes, this would perform better, at the expense of a small amount of extra code.
Seems worth it to me.

-  m_criticalSection.Lock();
   {
+    CriticalSectionLock lock(m_criticalSection);
     m_cacheElements[cacheIndex].m_elements = allElementsCount;
   }
-  m_criticalSection.Unlock();
 
   // Get tag
   CComBSTR bstrTag;
   if (FAILED(pEl->get_tagName(&bstrTag)) || !bstrTag)
   {
     return;
   }
 
   CString tag = bstrTag;
   tag.MakeLower();
 
   // Custom OnElement
   if (!OnElement(pEl, tag, &m_cacheElements[cacheIndex], false, indent))
   {
     return;
   }
 
   // Update frame/iframe cache
   if (tag == "iframe")
   {
-    m_criticalSection.Lock();
-    {
-      m_cacheDocumentHasIframes.insert(m_documentName);
-    }
-    m_criticalSection.Unlock();
+    CriticalSectionLock lock(m_criticalSection);
+    m_cacheDocumentHasIframes.insert(m_documentName);
   }
   else if (tag == "frame")
   {
-    m_criticalSection.Lock();
-    {
-      m_cacheDocumentHasFrames.insert(m_documentName);
-    }
-    m_criticalSection.Unlock();
+    CriticalSectionLock lock(m_criticalSection);
+    m_cacheDocumentHasFrames.insert(m_documentName);
   }
 
   // Iterate through children of this element
   if (allElementsCount > 0)
   {
     long childElementsCount = 0;
 
     CComPtr<IDispatch> pChildCollectionDisp;
     if (SUCCEEDED(pEl->get_children(&pChildCollectionDisp)) && pChildCollectionDisp)
     {
@@ skipping 23 matching lines @@
         }
       }
     }
   }
 }
 
 
 template <class T>
 void CPluginDomTraverserBase<T>::ClearCache()
 {
-  m_criticalSection.Lock();
-  {
-    m_cacheIndexLast = 0;
-    m_cacheDocumentHasFrames.clear();
-    m_cacheDocumentHasIframes.clear();
-  }
-  m_criticalSection.Unlock();
-}
-template <class T>
-void CPluginDomTraverserBase<T>::ShowNotification(CPluginTab* tab)
-{
-  if (tab->m_plugin->GetTabHWND() == NULL)
-  {
-    return;
-  }
-
-
+  CriticalSectionLock lock(m_criticalSection);
+  m_cacheElements.clear();
+  m_cacheDocumentHasFrames.clear();
+  m_cacheDocumentHasIframes.clear();
 }
 
-
 #endif // _PLUGIN_DOM_TRAVERSER_BASE_H_