[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

4 files changed, 21 insertions, 12 deletions

diff --git a/xfa/fgas/font/fgas_stdfontmgr.cpp b/xfa/fgas/font/fgas_stdfontmgr.cpp
index ab7852e310..7e5cfbb796 100644
--- a/xfa/fgas/font/fgas_stdfontmgr.cpp
+++ b/xfa/fgas/font/fgas_stdfontmgr.cpp
@@ -1156,7 +1156,6 @@ void CFGAS_FontMgrImp::RemoveFont(CFGAS_GEFont* pEFont) {
 }
 
 void CFGAS_FontMgrImp::RegisterFace(FXFT_Face pFace,
-                                    CFX_FontDescriptors& Fonts,
                                     const CFX_WideString* pFaceName) {
   if ((pFace->face_flags & FT_FACE_FLAG_SCALABLE) == 0)
     return;
@@ -1188,7 +1187,7 @@ void CFGAS_FontMgrImp::RegisterFace(FXFT_Face pFace,
                 : CFX_WideString::FromLocal(FXFT_Get_Postscript_Name(pFace));
   pFont->m_nFaceIndex = pFace->face_index;
 
-  Fonts.Add(pFont.release());
+  m_InstalledFonts.Add(pFont.release());
 }
 
 void CFGAS_FontMgrImp::RegisterFaces(IFX_FileRead* pFontStream,
@@ -1202,7 +1201,7 @@ void CFGAS_FontMgrImp::RegisterFaces(IFX_FileRead* pFontStream,
     // All faces keep number of faces. It can be retrieved from any one face.
     if (num_faces == 0)
       num_faces = pFace->num_faces;
-    RegisterFace(pFace, m_InstalledFonts, pFaceName);
+    RegisterFace(pFace, pFaceName);
     if (FXFT_Get_Face_External_Stream(pFace))
       FXFT_Clear_Face_External_Stream(pFace);
     FXFT_Done_Face(pFace);
diff --git a/xfa/fgas/font/fgas_stdfontmgr.h b/xfa/fgas/font/fgas_stdfontmgr.h
index 65f260c54c..0506876b64 100644
--- a/xfa/fgas/font/fgas_stdfontmgr.h
+++ b/xfa/fgas/font/fgas_stdfontmgr.h
@@ -184,7 +184,6 @@ class CFGAS_FontMgrImp : public IFGAS_FontMgr {
 
  protected:
   void RegisterFace(FXFT_Face pFace,
-                    CFX_FontDescriptors& Fonts,
                     const CFX_WideString* pFaceName);
   void RegisterFaces(IFX_FileRead* pFontStream,
                      const CFX_WideString* pFaceName);
@@ -192,7 +191,6 @@ class CFGAS_FontMgrImp : public IFGAS_FontMgr {
   std::vector<uint16_t> GetCharsets(FXFT_Face pFace) const;
   void GetUSBCSB(FXFT_Face pFace, uint32_t* USB, uint32_t* CSB);
   uint32_t GetFlags(FXFT_Face pFace);
-  CFX_FontDescriptors m_InstalledFonts;
   FX_BOOL VerifyUnicode(CFX_FontDescriptor* pDesc, FX_WCHAR wcUnicode);
   FX_BOOL VerifyUnicode(CFGAS_GEFont* pFont, FX_WCHAR wcUnicode);
   int32_t IsPartName(const CFX_WideString& Name1, const CFX_WideString& Name2);
@@ -212,6 +210,7 @@ class CFGAS_FontMgrImp : public IFGAS_FontMgr {
                                  uint32_t index);
   IFX_FileRead* CreateFontStream(const CFX_ByteString& bsFaceName);
 
+  CFX_FontDescriptors m_InstalledFonts;
   CFX_MapPtrTemplate<uint32_t, CFX_FontDescriptorInfos*> m_Hash2CandidateList;
   CFX_MapPtrTemplate<uint32_t, CFX_ArrayTemplate<CFGAS_GEFont*>*> m_Hash2Fonts;
   CFX_MapPtrTemplate<CFGAS_GEFont*, IFX_FileRead*> m_IFXFont2FileRead;
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;