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

src/plugin/PluginDomTraverserBase.h

Index: src/plugin/PluginDomTraverserBase.h
===================================================================
--- a/src/plugin/PluginDomTraverserBase.h
+++ b/src/plugin/PluginDomTraverserBase.h
@@ -32,7 +32,7 @@
 
   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:
 
@@ -46,6 +46,7 @@
   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;
@@ -53,15 +54,10 @@
 
   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;
@@ -69,16 +65,14 @@
 
 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>
@@ -91,7 +85,6 @@
 void CPluginDomTraverserBase<T>::TraverseDocument(IWebBrowser2* pBrowser, const CString& domain, const CString& documentName)
 {
   m_domain = domain;
-
   TraverseDocument(pBrowser, true, "");
 }
 
@@ -100,7 +93,6 @@
 void CPluginDomTraverserBase<T>::TraverseSubdocument(IWebBrowser2* pBrowser, const CString& domain, const CString& documentName)
 {
   m_domain = domain;
-
   TraverseDocument(pBrowser, false, "");
 }
 
@@ -118,13 +110,10 @@
   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
@@ -156,7 +145,7 @@
   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;
     }
@@ -179,7 +168,7 @@
   {
     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();
     }
@@ -205,7 +194,7 @@
     // 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);
     }
@@ -245,7 +234,7 @@
   {
     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);
     }
@@ -263,8 +252,7 @@
         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;
 
@@ -303,47 +291,37 @@
 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;
@@ -365,11 +343,10 @@
   }
 
   // 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;
@@ -390,19 +367,13 @@
   // 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
@@ -446,24 +417,10 @@
 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_