[XFA] Force destruction order of font managers.chromium/2838

The GEFont points to the font manager which creates it and tries to unregister
itself. Currently the GEFont can be created by the default mapper and then
stored in a different mapper. If the default mapper is destroyed first, when
the second mapper cleans up the font there will be a call to unregister on
the default mapper causing a use-after-free.

The long term fix is to fixup the GEFont so it points to the correct mapper
to unregister from. This CL forces the destruction order in CXFA_FFApp to
cleanup the non-default mapper first.

BUG=chromium:637546

Review-Url: https://codereview.chromium.org/2259823004

2 files changed, 18 insertions, 7 deletions

diff --git a/xfa/fxfa/app/xfa_fontmgr.cpp b/xfa/fxfa/app/xfa_fontmgr.cpp
index 5bb4ccb816..d191ce3537 100644
--- a/xfa/fxfa/app/xfa_fontmgr.cpp
+++ b/xfa/fxfa/app/xfa_fontmgr.cpp
@@ -1775,18 +1775,17 @@ CFGAS_GEFont* CXFA_DefFontMgr::GetFont(CXFA_FFDoc* hDoc,
         CFX_WideString wsReplace =
             CFX_WideString(pReplace, pNameText - pReplace);
         pFont = pFDEFontMgr->LoadFont(wsReplace.c_str(), dwStyle, wCodePage);
-        if (pFont) {
+        if (pFont)
           break;
-        }
+
         iLength--;
         pNameText++;
         pReplace = pNameText;
       }
     }
   }
-  if (pFont) {
+  if (pFont)
     m_CacheFonts.Add(pFont);
-  }
   return pFont;
 }
 
@@ -1804,9 +1803,8 @@ CFGAS_GEFont* CXFA_DefFontMgr::GetDefaultFont(
   }
 
   ASSERT(pFont);
-  if (pFont) {
+  if (pFont)
     m_CacheFonts.Add(pFont);
-  }
   return pFont;
 }
 struct XFA_PDFFONTNAME {
diff --git a/xfa/fxfa/include/xfa_ffapp.h b/xfa/fxfa/include/xfa_ffapp.h
index bc0d6dfb38..90bfcc0240 100644
--- a/xfa/fxfa/include/xfa_ffapp.h
+++ b/xfa/fxfa/include/xfa_ffapp.h
@@ -63,13 +63,26 @@ class CXFA_FFApp {
  protected:
   std::unique_ptr<CXFA_FFDocHandler> m_pDocHandler;
   IXFA_AppProvider* const m_pProvider;
+
+  // The fonts stored in the font manager may have been created by the default
+  // font manager. The GEFont::LoadFont call takes the manager as a param and
+  // stores it internally. When you destroy the GEFont it tries to unregister
+  // from the font manager and if the default font manager was destroyed first
+  // get get a use-after-free. The m_pFWLTheme can try to cleanup a GEFont
+  // when it frees, so make sure it gets cleaned up first. That requires
+  // m_pFWLApp to be cleaned up as well.
+  //
+  // TODO(dsinclair): The GEFont should have the FontMgr as the pointer instead
+  // of the DEFFontMgr so this goes away. Bug 561.
+  std::unique_ptr<IFGAS_FontMgr> m_pFDEFontMgr;
   std::unique_ptr<CXFA_FontMgr> m_pFontMgr;
+
 #if _FXM_PLATFORM_ != _FXM_PLATFORM_WINDOWS_
   std::unique_ptr<CFX_FontSourceEnum_File> m_pFontSource;
 #endif
   std::unique_ptr<CXFA_FWLAdapterWidgetMgr> m_pAdapterWidgetMgr;
   CFWL_WidgetMgrDelegate* m_pWidgetMgrDelegate;  // not owned.
-  std::unique_ptr<IFGAS_FontMgr> m_pFDEFontMgr;
+
   // |m_pFWLApp| has to be released first, then |m_pFWLTheme| since the former
   // may refers to theme manager and the latter refers to font manager.
   std::unique_ptr<CXFA_FWLTheme> m_pFWLTheme;