CFX_UnownedPtr: check during assignment time as well.

In particular, doing m_pPtr = nullptr; in your dtor to evade this
check will not longer work.

Fix slight mis-ordering observeds in CFX_Font and CPDFXFA_Context.

Change-Id: I3e6137159430333b091364021283a54a13d916b5
Reviewed-on: https://pdfium-review.googlesource.com/5570
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>

4 files changed, 47 insertions, 9 deletions

diff --git a/core/fxcrt/cfx_unowned_ptr.h b/core/fxcrt/cfx_unowned_ptr.h
index 11f4e8e38b..bc58144335 100644
--- a/core/fxcrt/cfx_unowned_ptr.h
+++ b/core/fxcrt/cfx_unowned_ptr.h
@@ -26,17 +26,20 @@ class CFX_UnownedPtr {
   CFX_UnownedPtr(std::nullptr_t ptr) {}
 
 #if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
-  ~CFX_UnownedPtr() {
-    if (m_pObj)
-      reinterpret_cast<volatile uint8_t*>(m_pObj)[0];
-  }
+  ~CFX_UnownedPtr() { Probe(); }
 #endif
 
   CFX_UnownedPtr& operator=(T* that) {
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
+    Probe();
+#endif
     m_pObj = that;
     return *this;
   }
   CFX_UnownedPtr& operator=(const CFX_UnownedPtr& that) {
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
+    Probe();
+#endif
     if (*this != that)
       m_pObj = that.Get();
     return *this;
@@ -58,6 +61,13 @@ class CFX_UnownedPtr {
   T* operator->() const { return m_pObj; }
 
  private:
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
+  void Probe() {
+    if (m_pObj)
+      reinterpret_cast<volatile uint8_t*>(m_pObj)[0];
+  }
+#endif
+
   T* m_pObj = nullptr;
 };
 
diff --git a/core/fxcrt/cfx_unowned_ptr_unittest.cpp b/core/fxcrt/cfx_unowned_ptr_unittest.cpp
index 7d29faedf5..586c4bbb2e 100644
--- a/core/fxcrt/cfx_unowned_ptr_unittest.cpp
+++ b/core/fxcrt/cfx_unowned_ptr_unittest.cpp
@@ -25,6 +25,15 @@ void DeleteDangling() {
   delete ptr2;
 }
 
+void AssignDangling() {
+  Clink* ptr1 = new Clink();
+  Clink* ptr2 = new Clink();
+  ptr2->next_ = ptr1;
+  delete ptr1;
+  ptr2->next_ = nullptr;
+  delete ptr2;
+}
+
 }  // namespace
 
 TEST(fxcrt, UnownedPtrOk) {
@@ -43,7 +52,24 @@ TEST(fxcrt, UnownedPtrNotOk) {
 #endif
 }
 
-TEST(fxcrt, OperatorEQ) {
+TEST(fxcrt, UnownedAssignOk) {
+  Clink* ptr1 = new Clink();
+  Clink* ptr2 = new Clink();
+  ptr2->next_ = ptr1;
+  ptr2->next_ = nullptr;
+  delete ptr2;
+  delete ptr1;
+}
+
+TEST(fxcrt, UnownedAssignNotOk) {
+#if defined(MEMORY_TOOL_REPLACES_ALLOCATOR)
+  EXPECT_DEATH(AssignDangling(), "");
+#else
+  AssignDangling();
+#endif
+}
+
+TEST(fxcrt, UnownedOperatorEQ) {
   int foo;
   CFX_UnownedPtr<int> ptr1;
   EXPECT_TRUE(ptr1 == ptr1);
@@ -59,7 +85,7 @@ TEST(fxcrt, OperatorEQ) {
   EXPECT_TRUE(ptr1 == ptr3);
 }
 
-TEST(fxcrt, OperatorNE) {
+TEST(fxcrt, UnownedOperatorNE) {
   int foo;
   CFX_UnownedPtr<int> ptr1;
   EXPECT_FALSE(ptr1 != ptr1);
@@ -75,7 +101,7 @@ TEST(fxcrt, OperatorNE) {
   EXPECT_FALSE(ptr1 != ptr3);
 }
 
-TEST(fxcrt, OperatorLT) {
+TEST(fxcrt, UnownedOperatorLT) {
   int foos[2];
   CFX_UnownedPtr<int> ptr1(&foos[0]);
   CFX_UnownedPtr<int> ptr2(&foos[1]);
diff --git a/core/fxge/ge/cfx_font.cpp b/core/fxge/ge/cfx_font.cpp
index 18baed9d09..1a45fd3f5b 100644
--- a/core/fxge/ge/cfx_font.cpp
+++ b/core/fxge/ge/cfx_font.cpp
@@ -545,8 +545,9 @@ CFX_FaceCache* CFX_Font::GetFaceCache() const {
 void CFX_Font::ClearFaceCache() {
   if (!m_FaceCache)
     return;
-  CFX_GEModule::Get()->GetFontCache()->ReleaseCachedFace(this);
+
   m_FaceCache = nullptr;
+  CFX_GEModule::Get()->GetFontCache()->ReleaseCachedFace(this);
 }
 
 int CFX_Font::GetULPos() const {
diff --git a/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp b/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp
index 1b69ff45b9..1345bc8a08 100644
--- a/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp
+++ b/fpdfsdk/fpdfxfa/cpdfxfa_context.cpp
@@ -67,9 +67,10 @@ CPDFXFA_Context::~CPDFXFA_Context() {
 void CPDFXFA_Context::CloseXFADoc() {
   if (!m_pXFADoc)
     return;
+
+  m_pXFADocView = nullptr;
   m_pXFADoc->CloseDoc();
   m_pXFADoc.reset();
-  m_pXFADocView = nullptr;
 }
 
 void CPDFXFA_Context::SetFormFillEnv(