Re-land "Assert that only 0-numbered objects are Released()"

This reverts commit b73c99335bfbd158ad16dd59c9c52396ffd2b54b.

TBR=thestig@chromium.org

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

6 files changed, 47 insertions, 36 deletions

diff --git a/core/fpdfapi/parser/cpdf_array.cpp b/core/fpdfapi/parser/cpdf_array.cpp
index 9a2dba0161..0d0c02f28d 100644
--- a/core/fpdfapi/parser/cpdf_array.cpp
+++ b/core/fpdfapi/parser/cpdf_array.cpp
@@ -23,7 +23,7 @@ CPDF_Array::~CPDF_Array() {
   // in case of cyclic references.
   m_ObjNum = kInvalidObjNum;
   for (auto& it : m_Objects) {
-    if (it)
+    if (it && it->GetObjNum() != kInvalidObjNum)
       it->Release();
   }
 }
diff --git a/core/fpdfapi/parser/cpdf_dictionary.cpp b/core/fpdfapi/parser/cpdf_dictionary.cpp
index 7ef5a53551..31f520af0d 100644
--- a/core/fpdfapi/parser/cpdf_dictionary.cpp
+++ b/core/fpdfapi/parser/cpdf_dictionary.cpp
@@ -30,7 +30,7 @@ CPDF_Dictionary::~CPDF_Dictionary() {
   // in case of cyclic references.
   m_ObjNum = kInvalidObjNum;
   for (const auto& it : m_Map) {
-    if (it.second)
+    if (it.second && it.second->GetObjNum() != kInvalidObjNum)
       it.second->Release();
   }
 }
diff --git a/core/fpdfapi/parser/cpdf_object.cpp b/core/fpdfapi/parser/cpdf_object.cpp
index 3bf0a953db..5c407336e2 100644
--- a/core/fpdfapi/parser/cpdf_object.cpp
+++ b/core/fpdfapi/parser/cpdf_object.cpp
@@ -38,9 +38,7 @@ CPDF_Object* CPDF_Object::CloneNonCyclic(
 }
 
 void CPDF_Object::Release() {
-  if (m_ObjNum)
-    return;
-
+  CHECK(!m_ObjNum);
   delete this;
 }
 
diff --git a/core/fpdfapi/parser/cpdf_object_unittest.cpp b/core/fpdfapi/parser/cpdf_object_unittest.cpp
index b2177afd8a..657f536c26 100644
--- a/core/fpdfapi/parser/cpdf_object_unittest.cpp
+++ b/core/fpdfapi/parser/cpdf_object_unittest.cpp
@@ -25,6 +25,7 @@ namespace {
 using ScopedArray = std::unique_ptr<CPDF_Array, ReleaseDeleter<CPDF_Array>>;
 using ScopedDict =
     std::unique_ptr<CPDF_Dictionary, ReleaseDeleter<CPDF_Dictionary>>;
+using ScopedStream = std::unique_ptr<CPDF_Stream, ReleaseDeleter<CPDF_Stream>>;
 
 void TestArrayAccessors(const CPDF_Array* arr,
                         size_t index,
@@ -95,8 +96,10 @@ class PDFObjectsTest : public testing::Test {
 
     // Indirect references to indirect objects.
     m_ObjHolder.reset(new CPDF_IndirectObjectHolder());
-    m_IndirectObjs = {boolean_true_obj, number_int_obj, str_spec_obj, name_obj,
-                      m_ArrayObj,       m_DictObj,      stream_obj};
+    m_IndirectObjs = {boolean_true_obj->Clone(), number_int_obj->Clone(),
+                      str_spec_obj->Clone(),     name_obj->Clone(),
+                      m_ArrayObj->Clone(),       m_DictObj->Clone(),
+                      stream_obj->Clone()};
     for (size_t i = 0; i < m_IndirectObjs.size(); ++i) {
       m_ObjHolder->AddIndirectObject(m_IndirectObjs[i]);
       m_RefObjs.emplace_back(new CPDF_Reference(
@@ -104,7 +107,7 @@ class PDFObjectsTest : public testing::Test {
     }
   }
 
-  bool Equal(CPDF_Object* obj1, CPDF_Object* obj2) {
+  bool Equal(const CPDF_Object* obj1, const CPDF_Object* obj2) {
     if (obj1 == obj2)
       return true;
     if (!obj1 || !obj2 || obj1->GetType() != obj2->GetType())
@@ -253,7 +256,7 @@ TEST_F(PDFObjectsTest, GetDict) {
   const CPDF_Dictionary* const indirect_obj_results[] = {
       nullptr, nullptr, nullptr, nullptr, nullptr, m_DictObj, m_StreamDictObj};
   for (size_t i = 0; i < m_RefObjs.size(); ++i)
-    EXPECT_EQ(indirect_obj_results[i], m_RefObjs[i]->GetDict());
+    EXPECT_TRUE(Equal(indirect_obj_results[i], m_RefObjs[i]->GetDict()));
 }
 
 TEST_F(PDFObjectsTest, GetArray) {
@@ -787,10 +790,9 @@ TEST(PDFDictionaryTest, CloneDirectObject) {
 
 TEST(PDFObjectTest, CloneCheckLoop) {
   {
-    // Create an object with a reference loop.
-    ScopedArray arr_obj(new CPDF_Array);
-    // Dictionary object.
+    // Create a dictionary/array pair with a reference loop.
     CPDF_Dictionary* dict_obj = new CPDF_Dictionary();
+    ScopedArray arr_obj(new CPDF_Array);
     dict_obj->SetFor("arr", arr_obj.get());
     arr_obj->InsertAt(0, dict_obj);
 
@@ -806,6 +808,22 @@ TEST(PDFObjectTest, CloneCheckLoop) {
     EXPECT_EQ(nullptr, cloned_dict->AsDictionary()->GetObjectFor("arr"));
   }
   {
+    // Create a dictionary/stream pair with a reference loop.
+    CPDF_Dictionary* dict_obj = new CPDF_Dictionary();
+    ScopedStream stream_obj(new CPDF_Stream(nullptr, 0, dict_obj));
+    dict_obj->SetFor("stream", stream_obj.get());
+
+    // Clone this object to see whether stack overflow will be triggered.
+    ScopedStream cloned_stream(stream_obj->Clone()->AsStream());
+    // Cloned object should be the same as the original.
+    ASSERT_TRUE(cloned_stream);
+    CPDF_Object* cloned_dict = cloned_stream->GetDict();
+    ASSERT_TRUE(cloned_dict);
+    ASSERT_TRUE(cloned_dict->IsDictionary());
+    // Recursively referenced object is not cloned.
+    EXPECT_EQ(nullptr, cloned_dict->AsDictionary()->GetObjectFor("stream"));
+  }
+  {
     CPDF_IndirectObjectHolder objects_holder;
     // Create an object with a reference loop.
     CPDF_Dictionary* dict_obj = new CPDF_Dictionary();
diff --git a/core/fpdfapi/parser/cpdf_parser.cpp b/core/fpdfapi/parser/cpdf_parser.cpp
index 17539101a1..f4cde0c198 100644
--- a/core/fpdfapi/parser/cpdf_parser.cpp
+++ b/core/fpdfapi/parser/cpdf_parser.cpp
@@ -956,36 +956,34 @@ FX_BOOL CPDF_Parser::RebuildCrossRef() {
 }
 
 FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) {
-  CPDF_Object* pObject = ParseIndirectObjectAt(m_pDocument, *pos, 0);
+  std::unique_ptr<CPDF_Object> pObject(
+      ParseIndirectObjectAt(m_pDocument, *pos, 0));
   if (!pObject)
     return FALSE;
 
+  CPDF_Object* pUnownedObject = pObject.get();
+
   if (m_pDocument) {
     CPDF_Dictionary* pRootDict = m_pDocument->GetRoot();
-    if (pRootDict && pRootDict->GetObjNum() == pObject->m_ObjNum) {
-      // If |pObject| has an objnum assigned then this will leak as Release()
-      // will early exit.
-      if (pObject->IsStream())
-        pObject->Release();
+    if (pRootDict && pRootDict->GetObjNum() == pObject->m_ObjNum)
       return FALSE;
-    }
-    if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration(pObject->m_ObjNum,
-                                                              pObject)) {
+    // Takes ownership of object (std::move someday).
+    uint32_t objnum = pObject->m_ObjNum;
+    if (!m_pDocument->ReplaceIndirectObjectIfHigherGeneration(
+            objnum, pObject.release())) {
       return FALSE;
     }
   }
 
-  CPDF_Stream* pStream = pObject->AsStream();
+  CPDF_Stream* pStream = pUnownedObject->AsStream();
   if (!pStream)
     return FALSE;
 
   CPDF_Dictionary* pDict = pStream->GetDict();
   *pos = pDict->GetIntegerFor("Prev");
   int32_t size = pDict->GetIntegerFor("Size");
-  if (size < 0) {
-    pStream->Release();
+  if (size < 0)
     return FALSE;
-  }
 
   CPDF_Dictionary* pNewTrailer = ToDictionary(pDict->Clone());
   if (bMainXRef) {
@@ -1017,10 +1015,8 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) {
     arrIndex.push_back(std::make_pair(0, size));
 
   pArray = pDict->GetArrayFor("W");
-  if (!pArray) {
-    pStream->Release();
+  if (!pArray)
     return FALSE;
-  }
 
   CFX_ArrayTemplate<uint32_t> WidthArray;
   FX_SAFE_UINT32 dwAccWidth = 0;
@@ -1029,10 +1025,8 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) {
     dwAccWidth += WidthArray[i];
   }
 
-  if (!dwAccWidth.IsValid() || WidthArray.GetSize() < 3) {
-    pStream->Release();
+  if (!dwAccWidth.IsValid() || WidthArray.GetSize() < 3)
     return FALSE;
-  }
 
   uint32_t totalWidth = dwAccWidth.ValueOrDie();
   CPDF_StreamAcc acc;
@@ -1092,17 +1086,14 @@ FX_BOOL CPDF_Parser::LoadCrossRefV5(FX_FILESIZE* pos, FX_BOOL bMainXRef) {
         if (type == 1) {
           m_SortedOffset.insert(offset);
         } else {
-          if (offset < 0 || !IsValidObjectNumber(offset)) {
-            pStream->Release();
+          if (offset < 0 || !IsValidObjectNumber(offset))
             return FALSE;
-          }
           m_ObjectInfo[offset].type = 255;
         }
       }
     }
     segindex += count;
   }
-  pStream->Release();
   return TRUE;
 }
 
diff --git a/core/fpdfapi/parser/cpdf_stream.cpp b/core/fpdfapi/parser/cpdf_stream.cpp
index c221edec68..935c80c5c8 100644
--- a/core/fpdfapi/parser/cpdf_stream.cpp
+++ b/core/fpdfapi/parser/cpdf_stream.cpp
@@ -17,7 +17,11 @@ CPDF_Stream::CPDF_Stream() {}
 CPDF_Stream::CPDF_Stream(uint8_t* pData, uint32_t size, CPDF_Dictionary* pDict)
     : m_pDict(pDict), m_dwSize(size), m_pDataBuf(pData) {}
 
-CPDF_Stream::~CPDF_Stream() {}
+CPDF_Stream::~CPDF_Stream() {
+  m_ObjNum = kInvalidObjNum;
+  if (m_pDict && m_pDict->GetObjNum() == kInvalidObjNum)
+    m_pDict.release();  // lowercase release, release ownership.
+}
 
 CPDF_Object::Type CPDF_Stream::GetType() const {
   return STREAM;