Fix a leak with FPDFPageObj_NewImgeObj().

core/fpdfapi/fpdf_page/cpdf_imageobject.cpp

 // Copyright 2016 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "core/fpdfapi/fpdf_page/include/cpdf_imageobject.h"
 
 #include "core/fpdfapi/fpdf_page/include/cpdf_image.h"
 #include "core/fpdfapi/fpdf_page/pageint.h"
 #include "core/fpdfapi/fpdf_parser/include/cpdf_document.h"
 
 CPDF_ImageObject::CPDF_ImageObject() : m_pImage(nullptr) {}
 
 CPDF_ImageObject::~CPDF_ImageObject() {
-  if (!m_pImage) {
+  if (!m_pImage)
+    return;
+
+  if (m_pImage->CanRelease()) {
+    m_pImage->Release();
     return;
   }
-  if (m_pImage->IsInline() ||
-      (m_pImage->GetStream() && m_pImage->GetStream()->GetObjNum() == 0)) {
-    delete m_pImage;
-  } else {
-    m_pImage->GetDocument()->GetPageData()->ReleaseImage(m_pImage->GetStream());
-  }
+
+  auto* pPageData = m_pImage->GetDocument()->GetPageData();
+  if (pPageData->ReleaseImage(m_pImage->GetStream()))
+    return;
+
+  delete m_pImage;

[Lei Zhang, 2016/08/01 22:08:03]

Not sure what else we can do here.

[Wei Li, 2016/08/04 18:00:39]

I suspect this might not be the right thing to do. If ReleaseImage() failed or
m_pImage can not be released, this means the image is still referenced by some
other object. This is one of the tricky situation that release order matters. 

[Lei Zhang, 2016/08/04 21:58:25]

Does you comment apply to patch set 2 as well?

[Wei Li, 2016/08/04 23:36:39]

Yes, I think both patches share similar logic. Could you be more explicit about
which situation this 'delete' tries to cover? Is it "m_pImage->CanRelease()"
failed and "pPageData has no such image"? I expect the image is either stored
inside this ImageObject (CanRelease() should be true) or in the pageData's map.
Should it be deleted from its owner side?

[Lei Zhang, 2016/08/05 18:55:22]

So CPDF_ImageObject::m_pImage gets assigned from 6 places:
1) CPDF_PageContentGenerator::ProcessImage: Owned by CPDF_DocPageData
2) CPDF_ImageObject::Clone: Either owned by CPDF_DocPageData, or owned by
CPDF_ImageObject
3) CPDF_StreamContentParser::AddImage: Owned by CPDF_DocPageData
4) CPDF_StreamContentParser::AddImage, again: Owned by CPDF_DocPageData
5) CPDF_StreamContentParser::AddImage, thrice: Owned by CPDF_ImageObject
6) FPDFPageObj_NewImgeObj: Owned by CPDF_ImageObject

Another theory is that we have Release() because
CPDF_DocPageData::ReleaseImage() returns a void.

If we change CPDF_DocPageData::ReleaseImage() to return a bool, CPDF_ImageObject
can get rid of its Release() and just delete after checking with ReleaseImage().

 }
 
 CPDF_ImageObject* CPDF_ImageObject::Clone() const {
   CPDF_ImageObject* obj = new CPDF_ImageObject;
   obj->CopyData(this);
 
   obj->m_pImage = m_pImage->Clone();
   obj->m_Matrix = m_Matrix;
   return obj;
 }
@@ skipping 17 matching lines @@
 
 const CPDF_ImageObject* CPDF_ImageObject::AsImage() const {
   return this;
 }
 
 void CPDF_ImageObject::CalcBoundingBox() {
   m_Left = m_Bottom = 0;
   m_Right = m_Top = 1.0f;
   m_Matrix.TransformRect(m_Left, m_Right, m_Top, m_Bottom);
 }