Adjust the order of clearing resource in CPDF_DocPageData::Clear

core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp

Index: core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp
diff --git a/core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp b/core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp
index 77db4534fbd27f345ede1a73f9cb5ce3bbaea08e..93193d8428fa12d1f73441f06f540e94efd276f2 100644
--- a/core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp
+++ b/core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp
@@ -151,7 +151,6 @@ CPDF_DocPageData::~CPDF_DocPageData()
 void CPDF_DocPageData::Clear(FX_BOOL bForceRelease)
 {
     FX_POSITION pos;
-    FX_DWORD	nCount;
 
     m_bForceClear = bForceRelease;
     pos = m_FontMap.GetStartPosition();
@@ -159,31 +158,23 @@ void CPDF_DocPageData::Clear(FX_BOOL bForceRelease)
         CPDF_Dictionary* fontDict;
         CPDF_CountedObject<CPDF_Font*>* fontData;
         m_FontMap.GetNextAssoc(pos, fontDict, fontData);
-        nCount = fontData->m_nCount;
-        if (bForceRelease || nCount < 2) {
+        if (!fontData || !fontData->m_Obj) {

[Tom Sepez, 2014/09/19 18:33:13]

Can any of these return NULL fontData, etc.  The old code didn't check for it so
this seems like a needless complication (yeah, defensive programming but I think
its clearer without it).  

+            continue;
+        }
+        if (bForceRelease || fontData->m_nCount < 2) {
             delete fontData->m_Obj;
             fontData->m_Obj = NULL;
         }
     }
-    pos = m_ImageMap.GetStartPosition();
-    while (pos) {
-        FX_DWORD objNum;
-        CPDF_CountedObject<CPDF_Image*>* imageData;
-        m_ImageMap.GetNextAssoc(pos, objNum, imageData);
-        nCount = imageData->m_nCount;
-        if (bForceRelease || nCount < 2) {
-            delete imageData->m_Obj;
-            delete imageData;
-            m_ImageMap.RemoveKey(objNum);
-        }
-    }
     pos = m_ColorSpaceMap.GetStartPosition();
     while (pos) {
         CPDF_Object* csKey;
         CPDF_CountedObject<CPDF_ColorSpace*>* csData;
         m_ColorSpaceMap.GetNextAssoc(pos, csKey, csData);
-        nCount = csData->m_nCount;
-        if (bForceRelease || nCount < 2) {
+        if (!csData || !csData->m_Obj) {
+            continue;
+        }
+        if (bForceRelease || csData->m_nCount < 2) {
             csData->m_Obj->ReleaseCS();
             csData->m_Obj = NULL;
         }
@@ -193,8 +184,10 @@ void CPDF_DocPageData::Clear(FX_BOOL bForceRelease)
         CPDF_Stream* ipKey;
         CPDF_CountedObject<CPDF_IccProfile*>* ipData;
         m_IccProfileMap.GetNextAssoc(pos, ipKey, ipData);
-        nCount = ipData->m_nCount;
-        if (bForceRelease || nCount < 2) {
+        if (!ipKey || !ipData) {

[Tom Sepez, 2014/09/19 18:33:13]

Don't think these can be NULL, can they?

+            continue;
+        }
+        if (bForceRelease || ipData->m_nCount < 2) {
             FX_POSITION pos2 = m_HashProfileMap.GetStartPosition();
             while (pos2) {
                 CFX_ByteString bsKey;
@@ -205,7 +198,10 @@ void CPDF_DocPageData::Clear(FX_BOOL bForceRelease)
                     break;
                 }
             }
-            delete ipData->m_Obj;
+            if (ipData->m_Obj) {
+                delete ipData->m_Obj;
+                ipData->m_Obj = NULL;
+            }
             delete ipData;
             m_IccProfileMap.RemoveKey(ipKey);

[Tom Sepez, 2014/09/19 18:33:13]

nit: I always feel a little squeamish about deleting the object while there's
still a key pointing at it in a map.  Is there a reason we can't reverse these
two lines?

         }
@@ -215,11 +211,18 @@ void CPDF_DocPageData::Clear(FX_BOOL bForceRelease)
         CPDF_Stream* ftKey;
         CPDF_CountedObject<CPDF_StreamAcc*>* ftData;
         m_FontFileMap.GetNextAssoc(pos, ftKey, ftData);
-        nCount = ftData->m_nCount;
-        if (bForceRelease || nCount < 2) {
-            delete ftData->m_Obj;
+        if (!ftData) {

[Tom Sepez, 2014/09/19 18:33:13]

same here.

+            continue;
+        }
+        if (bForceRelease || ftData->m_nCount < 2) {
+            if (ftData->m_Obj) {
+                delete ftData->m_Obj;
+                ftData->m_Obj = NULL;
+            }
             delete ftData;
-            m_FontFileMap.RemoveKey(ftKey);
+            if (ftKey) {
+                m_FontFileMap.RemoveKey(ftKey);
+            }
         }
     }
     pos = m_PatternMap.GetStartPosition();
@@ -227,13 +230,32 @@ void CPDF_DocPageData::Clear(FX_BOOL bForceRelease)
         CPDF_Object* ptObj;
         CPDF_CountedObject<CPDF_Pattern*>* ptData;
         m_PatternMap.GetNextAssoc(pos, ptObj, ptData);
-        nCount = ptData->m_nCount;
-        if (bForceRelease || nCount < 2) {
+        if (!ptData || !ptData->m_Obj) {
+            continue;
+        }
+        if (bForceRelease || ptData->m_nCount < 2) {
             ptData->m_Obj->SetForceClear(bForceRelease);
             delete ptData->m_Obj;
             ptData->m_Obj = NULL;
         }
     }
+    pos = m_ImageMap.GetStartPosition();
+    while (pos) {
+        FX_DWORD objNum;
+        CPDF_CountedObject<CPDF_Image*>* imageData;
+        m_ImageMap.GetNextAssoc(pos, objNum, imageData);
+        if (!imageData) {

[Tom Sepez, 2014/09/19 18:33:13]

same here.

+            continue;
+        }
+        if (bForceRelease || imageData->m_nCount < 2) {
+            if (imageData->m_Obj) {
+                delete imageData->m_Obj;
+                imageData->m_Obj = NULL;
+            }
+            delete imageData;
+            m_ImageMap.RemoveKey(objNum);
+        }
+    }
 }
 CPDF_Font* CPDF_DocPageData::GetFont(CPDF_Dictionary* pFontDict, FX_BOOL findOnly)
 {