Fixup MSan embeddertests

The embeddertests were closing the document before the formfill environment.
This caused a use-after-free as we try to use the document during formfill
destruction.

This Cl fixes the destruction order in the embedder tests. As well, a few guards
are put in place to keep the system from crashing if the wrong destruction
order is called.

R=tsepez@chromium.org

Review URL: https://codereview.chromium.org/2398063002 .

4 files changed, 8 insertions, 9 deletions

diff --git a/fpdfsdk/cpdfsdk_document.h b/fpdfsdk/cpdfsdk_document.h
index 3da229cc67..ee140d82ce 100644
--- a/fpdfsdk/cpdfsdk_document.h
+++ b/fpdfsdk/cpdfsdk_document.h
@@ -54,6 +54,7 @@ class CPDFSDK_Document : public CFX_Observable<CPDFSDK_Document> {
 #ifdef PDF_ENABLE_XFA
   // Gets the XFA document directly (XFA-only).
   CPDFXFA_Document* GetXFADocument() const { return m_pDoc; }
+  void ResetXFADocument() { m_pDoc = nullptr; }
 
   int GetPageViewCount() const { return m_pageMap.size(); }
 #endif  // PDF_ENABLE_XFA
diff --git a/fpdfsdk/fpdfformfill.cpp b/fpdfsdk/fpdfformfill.cpp
index 8be9d5f0b7..fd083c9dcd 100644
--- a/fpdfsdk/fpdfformfill.cpp
+++ b/fpdfsdk/fpdfformfill.cpp
@@ -277,7 +277,10 @@ FPDFDOC_ExitFormFillEnvironment(FPDF_FORMHANDLE hHandle) {
   // XFA document.
   if (CPDFSDK_Document* pSDKDoc = pEnv->GetSDKDocument()) {
     pSDKDoc->ClearAllFocusedAnnots();
-    pSDKDoc->GetXFADocument()->SetSDKDoc(nullptr);
+    // If the document was closed first, it's possible the XFA document
+    // is now a nullptr.
+    if (pSDKDoc->GetXFADocument())
+      pSDKDoc->GetXFADocument()->SetSDKDoc(nullptr);
   }
 #endif  // PDF_ENABLE_XFA
 
diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp
index bd7f931c59..90a762ddf3 100644
--- a/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp
+++ b/fpdfsdk/fpdfxfa/cpdfxfa_document.cpp
@@ -45,6 +45,9 @@ CPDFXFA_Document::~CPDFXFA_Document() {
 
   if (m_pSDKDoc) {
     m_pSDKDoc->ClearAllFocusedAnnots();
+    // Once we're deleted the SDKDocument will point at a bad underlying
+    // doc so we need to reset it ...
+    m_pSDKDoc->ResetXFADocument();
     m_pSDKDoc = nullptr;
   }
 
diff --git a/testing/embedder_test.cpp b/testing/embedder_test.cpp
index 1ce0f36a45..c23b5c8018 100644
--- a/testing/embedder_test.cpp
+++ b/testing/embedder_test.cpp
@@ -99,16 +99,8 @@ void EmbedderTest::SetUp() {
 void EmbedderTest::TearDown() {
   if (document_) {
     FORM_DoDocumentAAction(form_handle_, FPDFDOC_AACTION_WC);
-#ifdef PDF_ENABLE_XFA
-    // Note: The shut down order here is the reverse of the non-XFA branch
-    // order. Need to work out if this is required, and if it is, the lifetimes
-    // of objects owned by |doc| that |form| reference.
-    FPDF_CloseDocument(document_);
-    FPDFDOC_ExitFormFillEnvironment(form_handle_);
-#else   // PDF_ENABLE_XFA
     FPDFDOC_ExitFormFillEnvironment(form_handle_);
     FPDF_CloseDocument(document_);
-#endif  // PDF_ENABLE_XFA
   }
 
   FPDFAvail_Destroy(avail_);