Fix CPPGC Plugin warnings for PDFium
- Pure virtual Trace() not allowed.
- Superfluous Trace() removed.
- Converted Keys in maps to members.
- Made nested classes be GC'd rather than uniquely owned.
- Improved ContainerTrace() macros for these purposes.
Bug: chromium:1421576
Change-Id: Ided5190f193f0bed97c8268fbcf99f9b03b9c007
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/104591
Commit-Queue: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Nigi <nigi@chromium.org>
diff --git a/fxjs/gc/container_trace.h b/fxjs/gc/container_trace.h
index b7a5db8..dfe4a74 100644
--- a/fxjs/gc/container_trace.h
+++ b/fxjs/gc/container_trace.h
@@ -23,11 +23,29 @@
template <typename T, typename U, typename V = cppgc::Visitor>
void ContainerTrace(V* visitor,
+ const std::map<cppgc::Member<T>, U>& container) {
+ for (const auto& item : container) {
+ visitor->Trace(item.first);
+ }
+}
+
+template <typename T, typename U, typename V = cppgc::Visitor>
+void ContainerTrace(V* visitor,
const std::map<U, cppgc::Member<T>>& container) {
for (const auto& item : container)
visitor->Trace(item.second);
}
+template <typename T, typename U, typename V = cppgc::Visitor>
+void ContainerTrace(
+ V* visitor,
+ const std::map<cppgc::Member<U>, cppgc::Member<T>>& container) {
+ for (const auto& item : container) {
+ visitor->Trace(item.first);
+ visitor->Trace(item.second);
+ }
+}
+
template <typename T, typename V = cppgc::Visitor>
void ContainerTrace(V* visitor, const std::set<cppgc::Member<T>>& container) {
for (const auto& item : container)
diff --git a/fxjs/gc/container_trace_unittest.cpp b/fxjs/gc/container_trace_unittest.cpp
index f8f7769..9273afe 100644
--- a/fxjs/gc/container_trace_unittest.cpp
+++ b/fxjs/gc/container_trace_unittest.cpp
@@ -43,7 +43,16 @@
EXPECT_EQ(1, cv.call_count());
}
-TEST(ContainerTrace, ActualMapTrace) {
+TEST(ContainerTrace, ActualMapTraceFirst) {
+ std::map<cppgc::Member<Thing>, int> thing;
+ thing[nullptr] = 42;
+
+ CountingVisitor cv;
+ ContainerTrace(&cv, thing);
+ EXPECT_EQ(1, cv.call_count());
+}
+
+TEST(ContainerTrace, ActualMapTraceSecond) {
std::map<int, cppgc::Member<Thing>> thing;
thing[42] = nullptr;
@@ -52,6 +61,15 @@
EXPECT_EQ(1, cv.call_count());
}
+TEST(ContainerTrace, ActualMapTraceBoth) {
+ std::map<cppgc::Member<Thing>, cppgc::Member<Thing>> thing;
+ thing[nullptr] = nullptr;
+
+ CountingVisitor cv;
+ ContainerTrace(&cv, thing);
+ EXPECT_EQ(2, cv.call_count());
+}
+
TEST(ContainerTrace, ActualSetTrace) {
std::set<cppgc::Member<Thing>> thing;
thing.insert(nullptr);
diff --git a/fxjs/xfa/cfxjse_engine.h b/fxjs/xfa/cfxjse_engine.h
index 462cedf..c6a95c3 100644
--- a/fxjs/xfa/cfxjse_engine.h
+++ b/fxjs/xfa/cfxjse_engine.h
@@ -199,8 +199,10 @@
CXFA_Script::Type m_eScriptType = CXFA_Script::Type::Unknown;
// |m_mapObjectToValue| is what ensures the v8 object bound to a
// CJX_Object remains valid for the lifetime of the engine.
- std::map<CJX_Object*, v8::Global<v8::Object>> m_mapObjectToObject;
- std::map<CJX_Object*, std::unique_ptr<CFXJSE_Context>> m_mapVariableToContext;
+ std::map<cppgc::Persistent<CJX_Object>, v8::Global<v8::Object>>
+ m_mapObjectToObject;
+ std::map<cppgc::Persistent<CJX_Object>, std::unique_ptr<CFXJSE_Context>>
+ m_mapVariableToContext;
cppgc::Persistent<CXFA_Node> m_pTarget;
UnownedPtr<CXFA_EventParam> m_eventParam;
std::vector<cppgc::Persistent<CXFA_Node>> m_upObjectArray;
diff --git a/xfa/fwl/cfwl_app.cpp b/xfa/fwl/cfwl_app.cpp
index 39afbd6..250fcec 100644
--- a/xfa/fwl/cfwl_app.cpp
+++ b/xfa/fwl/cfwl_app.cpp
@@ -18,7 +18,8 @@
pAdapter->GetWidgetMgrAdapter(),
this)),
m_pNoteDriver(cppgc::MakeGarbageCollected<CFWL_NoteDriver>(
- pAdapter->GetHeap()->GetAllocationHandle())) {}
+ pAdapter->GetHeap()->GetAllocationHandle(),
+ this)) {}
CFWL_App::~CFWL_App() = default;
diff --git a/xfa/fwl/cfwl_notedriver.cpp b/xfa/fwl/cfwl_notedriver.cpp
index 35a110f..e6bcfe8 100644
--- a/xfa/fwl/cfwl_notedriver.cpp
+++ b/xfa/fwl/cfwl_notedriver.cpp
@@ -11,6 +11,7 @@
#include "build/build_config.h"
#include "core/fxcrt/fx_extension.h"
+#include "fxjs/gc/container_trace.h"
#include "xfa/fwl/cfwl_app.h"
#include "xfa/fwl/cfwl_event.h"
#include "xfa/fwl/cfwl_messagekey.h"
@@ -27,14 +28,13 @@
} // namespace
-CFWL_NoteDriver::CFWL_NoteDriver() = default;
+CFWL_NoteDriver::CFWL_NoteDriver(CFWL_App* pApp) : m_pApp(pApp) {}
CFWL_NoteDriver::~CFWL_NoteDriver() = default;
void CFWL_NoteDriver::Trace(cppgc::Visitor* visitor) const {
- for (const auto& item : m_eventTargets)
- item.second->Trace(visitor);
-
+ visitor->Trace(m_pApp);
+ ContainerTrace(visitor, m_eventTargets);
visitor->Trace(m_pHover);
visitor->Trace(m_pFocus);
visitor->Trace(m_pGrab);
@@ -54,9 +54,10 @@
key = g_next_listener_key++;
pListener->SetEventKey(key);
}
- if (!m_eventTargets[key])
- m_eventTargets[key] = std::make_unique<Target>(pListener);
-
+ if (!m_eventTargets[key]) {
+ m_eventTargets[key] = cppgc::MakeGarbageCollected<Target>(
+ m_pApp->GetHeap()->GetAllocationHandle(), pListener);
+ }
m_eventTargets[key]->SetEventSource(pEventSource);
}
diff --git a/xfa/fwl/cfwl_notedriver.h b/xfa/fwl/cfwl_notedriver.h
index 1f39ec1..00fd961 100644
--- a/xfa/fwl/cfwl_notedriver.h
+++ b/xfa/fwl/cfwl_notedriver.h
@@ -36,7 +36,7 @@
void SetGrab(CFWL_Widget* pGrab) { m_pGrab = pGrab; }
private:
- class Target {
+ class Target : public cppgc::GarbageCollected<Target> {
public:
explicit Target(CFWL_Widget* pListener);
~Target();
@@ -53,7 +53,7 @@
std::set<cppgc::Member<CFWL_Widget>> m_widgets;
};
- CFWL_NoteDriver();
+ explicit CFWL_NoteDriver(CFWL_App* pApp);
bool DispatchMessage(CFWL_Message* pMessage, CFWL_Widget* pMessageForm);
bool DoSetFocus(CFWL_Message* pMsg, CFWL_Widget* pMessageForm);
@@ -64,7 +64,8 @@
bool DoMouseEx(CFWL_Message* pMsg, CFWL_Widget* pMessageForm);
void MouseSecondary(CFWL_Message* pMsg);
- std::map<uint64_t, std::unique_ptr<Target>> m_eventTargets;
+ cppgc::Member<CFWL_App> m_pApp;
+ std::map<uint64_t, cppgc::Member<Target>> m_eventTargets;
cppgc::Member<CFWL_Widget> m_pHover;
cppgc::Member<CFWL_Widget> m_pFocus;
cppgc::Member<CFWL_Widget> m_pGrab;
diff --git a/xfa/fwl/cfwl_widgetmgr.cpp b/xfa/fwl/cfwl_widgetmgr.cpp
index 86c4938..b513415 100644
--- a/xfa/fwl/cfwl_widgetmgr.cpp
+++ b/xfa/fwl/cfwl_widgetmgr.cpp
@@ -7,6 +7,7 @@
#include "xfa/fwl/cfwl_widgetmgr.h"
#include "build/build_config.h"
+#include "fxjs/gc/container_trace.h"
#include "third_party/base/check.h"
#include "xfa/fwl/cfwl_app.h"
#include "xfa/fwl/cfwl_message.h"
@@ -25,8 +26,7 @@
void CFWL_WidgetMgr::Trace(cppgc::Visitor* visitor) const {
visitor->Trace(m_pApp);
visitor->Trace(m_pAdapter);
- for (auto& item : m_mapWidgetItem)
- visitor->Trace(item.second);
+ ContainerTrace(visitor, m_mapWidgetItem);
}
CFWL_Widget* CFWL_WidgetMgr::GetParentWidget(const CFWL_Widget* pWidget) const {
diff --git a/xfa/fwl/cfwl_widgetmgr.h b/xfa/fwl/cfwl_widgetmgr.h
index fe32047..8383f0f 100644
--- a/xfa/fwl/cfwl_widgetmgr.h
+++ b/xfa/fwl/cfwl_widgetmgr.h
@@ -94,7 +94,8 @@
cppgc::Member<AdapterIface> const m_pAdapter;
cppgc::Member<CFWL_App> const m_pApp;
- std::map<const CFWL_Widget*, cppgc::Member<Item>> m_mapWidgetItem;
+ std::map<cppgc::Member<const CFWL_Widget>, cppgc::Member<Item>>
+ m_mapWidgetItem;
};
#endif // XFA_FWL_CFWL_WIDGETMGR_H_
diff --git a/xfa/fxfa/cxfa_ffpageview.h b/xfa/fxfa/cxfa_ffpageview.h
index 9af01c0..32dd1d9 100644
--- a/xfa/fxfa/cxfa_ffpageview.h
+++ b/xfa/fxfa/cxfa_ffpageview.h
@@ -57,8 +57,6 @@
Mask<XFA_WidgetStatus> dwFilter);
~CXFA_FFPageWidgetIterator() override;
- void Trace(cppgc::Visitor* visitor) const {}
-
// CXFA_FFWidget::IteratorIface:
CXFA_FFWidget* MoveToFirst() override;
CXFA_FFWidget* MoveToLast() override;
diff --git a/xfa/fxfa/cxfa_textlayout.cpp b/xfa/fxfa/cxfa_textlayout.cpp
index a25c44c..10782c0 100644
--- a/xfa/fxfa/cxfa_textlayout.cpp
+++ b/xfa/fxfa/cxfa_textlayout.cpp
@@ -100,8 +100,7 @@
visitor->Trace(m_pTextProvider);
visitor->Trace(m_pTextDataNode);
visitor->Trace(m_pTextParser);
- if (m_pLoader)
- m_pLoader->Trace(visitor);
+ visitor->Trace(m_pLoader);
}
void CXFA_TextLayout::Unload() {
@@ -331,7 +330,8 @@
float CXFA_TextLayout::StartLayout(float fWidth) {
if (!m_pLoader)
- m_pLoader = std::make_unique<LoaderContext>();
+ m_pLoader = cppgc::MakeGarbageCollected<LoaderContext>(
+ m_pDoc->GetHeap()->GetAllocationHandle());
if (fWidth < 0 ||
(m_pLoader->fWidth > -1 && fabs(fWidth - m_pLoader->fWidth) > 0)) {
@@ -615,7 +615,7 @@
for (size_t i = 0; i < szBlockCount; ++i)
LayoutInternal(i);
m_pTabstopContext.reset();
- m_pLoader.reset();
+ m_pLoader.Clear();
}
std::vector<TextCharPos> char_pos(1);
diff --git a/xfa/fxfa/cxfa_textlayout.h b/xfa/fxfa/cxfa_textlayout.h
index 601b0f3..500a7af 100644
--- a/xfa/fxfa/cxfa_textlayout.h
+++ b/xfa/fxfa/cxfa_textlayout.h
@@ -98,7 +98,7 @@
float fHeight;
};
- struct LoaderContext {
+ struct LoaderContext : public cppgc::GarbageCollected<LoaderContext> {
LoaderContext();
~LoaderContext();
@@ -183,8 +183,8 @@
cppgc::Member<CXFA_TextProvider> const m_pTextProvider;
cppgc::Member<CXFA_Node> m_pTextDataNode;
cppgc::Member<CXFA_TextParser> m_pTextParser;
+ cppgc::Member<LoaderContext> m_pLoader;
std::unique_ptr<CFGAS_RTFBreak> m_pBreak;
- std::unique_ptr<LoaderContext> m_pLoader;
std::vector<std::unique_ptr<PieceLine>> m_pieceLines;
std::unique_ptr<CXFA_TextTabstopsContext> m_pTabstopContext;
};
diff --git a/xfa/fxfa/layout/cxfa_contentlayoutprocessor.cpp b/xfa/fxfa/layout/cxfa_contentlayoutprocessor.cpp
index fe1624a..1d827f3 100644
--- a/xfa/fxfa/layout/cxfa_contentlayoutprocessor.cpp
+++ b/xfa/fxfa/layout/cxfa_contentlayoutprocessor.cpp
@@ -652,6 +652,7 @@
visitor->Trace(m_pViewLayoutProcessor);
ContainerTrace(visitor, m_ArrayKeepItems);
ContainerTrace(visitor, m_PendingNodes);
+ ContainerTrace(visitor, m_PendingNodesCount);
}
CXFA_ContentLayoutItem* CXFA_ContentLayoutProcessor::CreateContentLayoutItem(
diff --git a/xfa/fxfa/layout/cxfa_contentlayoutprocessor.h b/xfa/fxfa/layout/cxfa_contentlayoutprocessor.h
index 5257555..fac5673 100644
--- a/xfa/fxfa/layout/cxfa_contentlayoutprocessor.h
+++ b/xfa/fxfa/layout/cxfa_contentlayoutprocessor.h
@@ -237,7 +237,7 @@
std::vector<float> m_rgSpecifiedColumnWidths;
std::vector<cppgc::Member<CXFA_ContentLayoutItem>> m_ArrayKeepItems;
std::list<cppgc::Member<CXFA_Node>> m_PendingNodes;
- std::map<CXFA_Node*, int32_t> m_PendingNodesCount;
+ std::map<cppgc::Member<CXFA_Node>, int32_t> m_PendingNodesCount;
cppgc::Member<CXFA_ContentLayoutProcessor> m_pCurChildPreprocessor;
};
diff --git a/xfa/fxfa/layout/cxfa_viewlayoutprocessor.cpp b/xfa/fxfa/layout/cxfa_viewlayoutprocessor.cpp
index 353b376..b56c377 100644
--- a/xfa/fxfa/layout/cxfa_viewlayoutprocessor.cpp
+++ b/xfa/fxfa/layout/cxfa_viewlayoutprocessor.cpp
@@ -359,13 +359,13 @@
visitor->Trace(m_pCurPageArea);
visitor->Trace(m_pPageSetRootLayoutItem);
visitor->Trace(m_pPageSetCurLayoutItem);
- for (const auto& record : m_ProposedViewRecords)
- visitor->Trace(*record);
+ ContainerTrace(visitor, m_ProposedViewRecords);
if (m_CurrentViewRecordIter != m_ProposedViewRecords.end())
visitor->Trace(*m_CurrentViewRecordIter);
ContainerTrace(visitor, m_PageArray);
+ ContainerTrace(visitor, m_pPageSetMap);
}
bool CXFA_ViewLayoutProcessor::InitLayoutPage(CXFA_Node* pFormNode) {
diff --git a/xfa/fxfa/layout/cxfa_viewlayoutprocessor.h b/xfa/fxfa/layout/cxfa_viewlayoutprocessor.h
index 1463827..e48c9df 100644
--- a/xfa/fxfa/layout/cxfa_viewlayoutprocessor.h
+++ b/xfa/fxfa/layout/cxfa_viewlayoutprocessor.h
@@ -193,7 +193,7 @@
int32_t m_nCurPageCount = 0;
XFA_AttributeValue m_ePageSetMode = XFA_AttributeValue::OrderedOccurrence;
bool m_bCreateOverFlowPage = false;
- std::map<CXFA_Node*, int32_t> m_pPageSetMap;
+ std::map<cppgc::Member<CXFA_Node>, int32_t> m_pPageSetMap;
std::vector<cppgc::Member<CXFA_ViewLayoutItem>> m_PageArray;
};
diff --git a/xfa/fxfa/parser/cxfa_nodelocale.cpp b/xfa/fxfa/parser/cxfa_nodelocale.cpp
index 9b8a84f..60a44de 100644
--- a/xfa/fxfa/parser/cxfa_nodelocale.cpp
+++ b/xfa/fxfa/parser/cxfa_nodelocale.cpp
@@ -43,6 +43,7 @@
CXFA_NodeLocale::~CXFA_NodeLocale() = default;
void CXFA_NodeLocale::Trace(cppgc::Visitor* visitor) const {
+ GCedLocaleIface::Trace(visitor);
visitor->Trace(m_pNode);
}
diff --git a/xfa/fxfa/parser/cxfa_xmllocale.cpp b/xfa/fxfa/parser/cxfa_xmllocale.cpp
index ebc6402..8395680 100644
--- a/xfa/fxfa/parser/cxfa_xmllocale.cpp
+++ b/xfa/fxfa/parser/cxfa_xmllocale.cpp
@@ -58,7 +58,9 @@
CXFA_XMLLocale::~CXFA_XMLLocale() = default;
-void CXFA_XMLLocale::Trace(cppgc::Visitor* visitor) const {}
+void CXFA_XMLLocale::Trace(cppgc::Visitor* visitor) const {
+ GCedLocaleIface::Trace(visitor);
+}
WideString CXFA_XMLLocale::GetName() const {
return locale_->GetAttribute(L"name");
diff --git a/xfa/fxfa/parser/gced_locale_iface.h b/xfa/fxfa/parser/gced_locale_iface.h
index a4d6d9e..7d4ca63 100644
--- a/xfa/fxfa/parser/gced_locale_iface.h
+++ b/xfa/fxfa/parser/gced_locale_iface.h
@@ -13,7 +13,7 @@
class GCedLocaleIface : public cppgc::GarbageCollected<GCedLocaleIface>,
public LocaleIface {
public:
- virtual void Trace(cppgc::Visitor* visitor) const = 0;
+ virtual void Trace(cppgc::Visitor* visitor) const {}
};
#endif // XFA_FXFA_PARSER_GCED_LOCALE_IFACE_H_
