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_