From 2a7e8c1e455124e0b1399dd5852515e566f54746 Mon Sep 17 00:00:00 2001 From: Reef Turner Date: Mon, 29 Nov 2021 22:34:25 +0800 Subject: [PATCH 1/3] Add a helper to getAccessibleChildren Make all usages of AccessibleChildren conform to a consitant approach. Management of resources is automatic. --- nvdaHelper/common/ia2utils.cpp | 22 +++++++++++ nvdaHelper/common/ia2utils.h | 8 ++++ .../adobeAcrobat/adobeAcrobat.cpp | 37 ++++++------------- .../vbufBackends/gecko_ia2/gecko_ia2.cpp | 30 ++++++--------- .../lotusNotesRichText/lotusNotesRichText.cpp | 19 +++++++--- nvdaHelper/vbufBackends/webKit/webKit.cpp | 28 +++++++------- 6 files changed, 80 insertions(+), 64 deletions(-) diff --git a/nvdaHelper/common/ia2utils.cpp b/nvdaHelper/common/ia2utils.cpp index e383248e4d3..3053a1d9549 100644 --- a/nvdaHelper/common/ia2utils.cpp +++ b/nvdaHelper/common/ia2utils.cpp @@ -74,6 +74,28 @@ void IA2AttribsToMap(const wstring &attribsString, map &attrib } } +std::pair, HRESULT> +getAccessibleChildren(IAccessible* pacc, long indexOfFirstChild, long maxChildCount) { + std::vector varChildren(maxChildCount); + const auto res = AccessibleChildren( + pacc, + indexOfFirstChild, + maxChildCount, + varChildren.data(), + &maxChildCount + ); + if (res != S_OK) { + return std::make_pair( + std::vector(0), + res + ); + } + // shrink the vector in case less children were returned. + varChildren.resize(maxChildCount); // so that varChildren.size() will equal actual filled size + varChildren.shrink_to_fit(); // so that excess capacity isn't kept + return std::make_pair(varChildren, S_OK); +} + CComPtr HyperlinkGetter::next() { return this->get(this->index++); } diff --git a/nvdaHelper/common/ia2utils.h b/nvdaHelper/common/ia2utils.h index e847b706041..9d56ea244ac 100644 --- a/nvdaHelper/common/ia2utils.h +++ b/nvdaHelper/common/ia2utils.h @@ -18,6 +18,8 @@ This license can be found at: #include #include #include +#include +#include #include #include @@ -34,6 +36,12 @@ bool fetchIA2Attributes(IAccessible2* pacc2, std::map &attribsMap); +/** +* Helper to collect the children for an IAccessible, uses memory managed types that will clear / delete automatically. +*/ +std::pair, HRESULT> +getAccessibleChildren(IAccessible* pacc, long indexOfFirstChild, long maxChildCount); + /** * Base class to support retrieving hyperlinks (embedded objects) from * IAccessibleHypertext or IAccessibleHypertext2. diff --git a/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp b/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp index 1b58711dbb6..ea4696232be 100644 --- a/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp +++ b/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp @@ -17,6 +17,7 @@ This license can be found at: #include #include #include +#include #include #include #include @@ -619,30 +620,22 @@ AdobeAcrobatVBufStorage_controlFieldNode_t* AdobeAcrobatVBufBackend_t::fillVBuf( } else if (childCount > 0) { // Iterate through the children. - LOG_DEBUG(L"Allocate memory to hold children"); - VARIANT* varChildren; - if((varChildren=(VARIANT*)malloc(sizeof(VARIANT)*childCount))==NULL) { - LOG_DEBUG(L"Error allocating varChildren memory"); - if (stdName) - SysFreeString(stdName); - return NULL; - } LOG_DEBUG(L"Fetch children with AccessibleChildren"); - if((res=AccessibleChildren(pacc,0,childCount,varChildren,(long*)(&childCount)))!=S_OK) { - LOG_DEBUG(L"AccessibleChildren returned "<QueryInterface(IID_IAccessible,(void**)(&childPacc)))!=S_OK) { - LOG_DEBUG(L"varChildren["<QueryInterface to IID_iAccessible returned "< childPacc(varChildren[i].pdispVal); + if(!childPacc) { + LOG_DEBUG(L"varChildren[" << i << L"]: QueryInterface to IID_iAccessible failed."); } - if(childPacc) { + else { if (this->isXFA) { // HACK: If this is an XFA document, we must call WindowFromAccessibleObject() so that AccessibleObjectFromEvent() will work for this node. HWND tempHwnd; @@ -654,15 +647,9 @@ AdobeAcrobatVBufStorage_controlFieldNode_t* AdobeAcrobatVBufBackend_t::fillVBuf( } else { LOG_DEBUG(L"Error in calling fillVBuf"); } - LOG_DEBUG(L"releasing child IAccessible object"); - childPacc->Release(); } } - VariantClear(&(varChildren[i])); } - LOG_DEBUG(L"Freeing memory holding children"); - free(varChildren); - } else { // No children, so this is a leaf node. if (!this->isXFA && !stdName) { diff --git a/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp b/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp index aa3d65225f9..5ef58457b35 100755 --- a/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp +++ b/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp @@ -950,25 +950,20 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf( } else if (renderChildren && childCount > 0) { // The object has no text, but we do want to render its children. - VARIANT* varChildren; - if(!(varChildren=(VARIANT*)malloc(sizeof(VARIANT)*childCount))) { - LOG_DEBUG(L"Error allocating varChildren memory"); - return NULL; + auto [varChildren, accChildRes] = getAccessibleChildren(pacc, 0, childCount); + if (S_OK != accChildRes || varChildren.size() == 0) { + LOG_DEBUG(L"AccessibleChildren failed, res: " << accChildRes); } - long accessibleChildrenCount = 0; - if(AccessibleChildren(pacc,0,childCount,varChildren,&accessibleChildrenCount)!=S_OK) { - LOG_DEBUG(L"AccessibleChildren failed"); - accessibleChildrenCount=0; - } - for(long i=0;iQueryInterface(IID_IAccessible2,(void**)&childPacc); + CComQIPtr< IAccessible2, &IID_IAccessible2> childPacc(child.pdispVal); if (!childPacc) { - VariantClear(&(varChildren[i])); + child.Clear(); continue; } tempNode = this->fillVBuf( @@ -986,11 +981,8 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf( } else LOG_DEBUG(L"Error in calling fillVBuf"); - childPacc->Release(); - VariantClear(&(varChildren[i])); + child.Clear(); } - free(varChildren); - } else if (renderSelectedItemOnly) { CComPtr item = this->getSelectedItem(pacc, IA2AttribsMap); if (item) { diff --git a/nvdaHelper/vbufBackends/lotusNotesRichText/lotusNotesRichText.cpp b/nvdaHelper/vbufBackends/lotusNotesRichText/lotusNotesRichText.cpp index c19f34da602..3cfa48868ce 100644 --- a/nvdaHelper/vbufBackends/lotusNotesRichText/lotusNotesRichText.cpp +++ b/nvdaHelper/vbufBackends/lotusNotesRichText/lotusNotesRichText.cpp @@ -17,6 +17,7 @@ This license can be found at: #include #include #include +#include #include #include #include "lotusNotesRichText.h" @@ -170,12 +171,18 @@ void lotusNotesRichTextVBufBackend_t::render(VBufStorage_buffer_t* buffer, int d VBufStorage_fieldNode_t* previousNode=NULL; long childCount=0; pacc->get_accChildCount(&childCount); - VARIANT* varChildren=(VARIANT*)malloc(sizeof(VARIANT)*childCount); - HRESULT hRes; - hRes=AccessibleChildren(pacc,0,childCount,varChildren,&childCount); - for(int i=0;irenderControlContent(buffer,parentNode,previousNode,docHandle,pacc,varChildren[i].lVal); + + auto [varChildren, hres] = getAccessibleChildren(pacc, 0, childCount); + for(CComVariant& child : varChildren) { + if(VT_I4 == child.vt) { + previousNode = this->renderControlContent( + buffer, + parentNode, + previousNode, + docHandle, + pacc, + child.lVal + ); } } } else { diff --git a/nvdaHelper/vbufBackends/webKit/webKit.cpp b/nvdaHelper/vbufBackends/webKit/webKit.cpp index e83db134455..9aa5073b8e8 100644 --- a/nvdaHelper/vbufBackends/webKit/webKit.cpp +++ b/nvdaHelper/vbufBackends/webKit/webKit.cpp @@ -19,6 +19,7 @@ This license can be found at: #include #include #include +#include #include #include "webKit.h" @@ -123,20 +124,19 @@ VBufStorage_fieldNode_t* WebKitVBufBackend_t::fillVBuf(int docHandle, IAccessibl // Iterate through the children. if (childCount > 0) { - auto varChildren = make_unique(childCount); - if(AccessibleChildren(pacc,0,childCount,varChildren.get(),(long*)(&childCount))!=S_OK) { - childCount=0; - } - for(int i=0;i childPacc = varChildren[i].pdispVal; - if(!childPacc) { - continue; - } - if((tempNode=this->fillVBuf(docHandle,childPacc,buffer,parentNode,previousNode))!=NULL) { - previousNode=tempNode; + auto [varChildren, accChildrenRes] = getAccessibleChildren(pacc, 0, childCount); + if (S_OK == accChildrenRes) { + for (CComVariant& child : varChildren) { + if (VT_DISPATCH != child.vt) { + continue; + } + CComQIPtr childPacc(child.pdispVal); + if (!childPacc) { + continue; + } + if ((tempNode = this->fillVBuf(docHandle, childPacc, buffer, parentNode, previousNode)) != NULL) { + previousNode = tempNode; + } } } } else { From 1d8c0e8036bd9ca16efe75f5c84090355ec375ea Mon Sep 17 00:00:00 2001 From: Reef Turner Date: Sun, 16 Jan 2022 17:43:19 +0800 Subject: [PATCH 2/3] Review actions --- nvdaHelper/common/ia2utils.cpp | 37 ++++++++++++------- .../adobeAcrobat/adobeAcrobat.cpp | 2 +- .../vbufBackends/gecko_ia2/gecko_ia2.cpp | 2 +- 3 files changed, 25 insertions(+), 16 deletions(-) diff --git a/nvdaHelper/common/ia2utils.cpp b/nvdaHelper/common/ia2utils.cpp index 3053a1d9549..a69598b3592 100644 --- a/nvdaHelper/common/ia2utils.cpp +++ b/nvdaHelper/common/ia2utils.cpp @@ -76,24 +76,33 @@ void IA2AttribsToMap(const wstring &attribsString, map &attrib std::pair, HRESULT> getAccessibleChildren(IAccessible* pacc, long indexOfFirstChild, long maxChildCount) { - std::vector varChildren(maxChildCount); - const auto res = AccessibleChildren( - pacc, - indexOfFirstChild, - maxChildCount, - varChildren.data(), - &maxChildCount - ); - if (res != S_OK) { + try { + std::vector varChildren(maxChildCount); + const auto res = AccessibleChildren( + pacc, + indexOfFirstChild, + maxChildCount, + varChildren.data(), + &maxChildCount + ); + if (res != S_OK) { + return std::make_pair( + std::vector(0), + res + ); + } + // shrink the vector in case less children were returned. + // so that varChildren.size() will equal actual filled size + varChildren.resize(maxChildCount); + // no need to shrink to fit, make_pair will copy the vector, using only the first varChildren.size() elements. + return std::make_pair(varChildren, S_OK); + } + catch (std::bad_array_new_length& e) { return std::make_pair( std::vector(0), - res + S_FALSE ); } - // shrink the vector in case less children were returned. - varChildren.resize(maxChildCount); // so that varChildren.size() will equal actual filled size - varChildren.shrink_to_fit(); // so that excess capacity isn't kept - return std::make_pair(varChildren, S_OK); } CComPtr HyperlinkGetter::next() { diff --git a/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp b/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp index ea4696232be..18fe515f830 100644 --- a/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp +++ b/nvdaHelper/vbufBackends/adobeAcrobat/adobeAcrobat.cpp @@ -623,7 +623,7 @@ AdobeAcrobatVBufStorage_controlFieldNode_t* AdobeAcrobatVBufBackend_t::fillVBuf( LOG_DEBUG(L"Fetch children with AccessibleChildren"); auto[varChildren, accChildRes] = getAccessibleChildren(pacc, 0, childCount); if(S_OK != accChildRes || varChildren.size() == 0) { - LOG_DEBUG(L"Failed to get AccessibleChildren, res: " << accChildRes); + LOG_DEBUG(L"Failed to get AccessibleChildren (count: " << childCount << L"), res: " << accChildRes); childCount=0; } LOG_DEBUG(L"got "<< varChildren.size() << L" children"); diff --git a/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp b/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp index 5ef58457b35..d143e3a50db 100755 --- a/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp +++ b/nvdaHelper/vbufBackends/gecko_ia2/gecko_ia2.cpp @@ -952,7 +952,7 @@ VBufStorage_fieldNode_t* GeckoVBufBackend_t::fillVBuf( // The object has no text, but we do want to render its children. auto [varChildren, accChildRes] = getAccessibleChildren(pacc, 0, childCount); if (S_OK != accChildRes || varChildren.size() == 0) { - LOG_DEBUG(L"AccessibleChildren failed, res: " << accChildRes); + LOG_ERROR(L"AccessibleChildren failed (count: " << childCount << L"), res: " << accChildRes); } LOG_DEBUG(L"got " << varChildren.size() << L" children"); From adfd21ae08a2a1aadd81b793a822ed7d91c51c4c Mon Sep 17 00:00:00 2001 From: Reef Turner Date: Sun, 16 Jan 2022 18:19:52 +0800 Subject: [PATCH 3/3] Remove unreferenced local variable --- nvdaHelper/common/ia2utils.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/nvdaHelper/common/ia2utils.cpp b/nvdaHelper/common/ia2utils.cpp index a69598b3592..3d2add7ef22 100644 --- a/nvdaHelper/common/ia2utils.cpp +++ b/nvdaHelper/common/ia2utils.cpp @@ -97,7 +97,7 @@ getAccessibleChildren(IAccessible* pacc, long indexOfFirstChild, long maxChildCo // no need to shrink to fit, make_pair will copy the vector, using only the first varChildren.size() elements. return std::make_pair(varChildren, S_OK); } - catch (std::bad_array_new_length& e) { + catch (std::bad_array_new_length&) { return std::make_pair( std::vector(0), S_FALSE