Fix a leak with FPDFPageObj_NewImgeObj().

Fix a leak with FPDFPageObj_NewImgeObj().

BUG=pdfium:545
Committed: https://pdfium.googlesource.com/pdfium/+/dc359b03ab6a70ee52a91119ff6704cae92f4809

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

thestig@chromium.org changed reviewers:
+ dsinclair@chromium.org

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

New unit tests pass with LSAN locally.

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:28: delete m_pImage;
Not sure what else we can do here.

[Lei Zhang, 2016-08-01 22:08:09]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-01 22:08:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-01 22:10:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-01 22:10:04]

Dry run: Try jobs failed on following builders:
  linux_xfa_rel_gyp on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa_rel_g...)
  win on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/1495)

[Lei Zhang, 2016-08-01 22:21:17]

Bleh, red trybots. Will ping you after I work that out.

[Lei Zhang, 2016-08-04 08:41:32]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-04 08:41:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-04 08:58:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-04 08:58:17]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-08-04 13:48:20]

thestig@chromium.org changed reviewers:
+ weili@chromium.org

[Lei Zhang, 2016-08-04 13:48:21]

Ready for review.

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

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:28: delete m_pImage;
On 2016/08/01 22:08:03, Lei Zhang wrote:
> Not sure what else we can do here.

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.

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:442: bool
CPDF_DocPageData::HasImage(const CPDF_Object* pImageStream) const {
Why adding an extra function which does the same as the first half of
ReleaseImage()?

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/include/cpdf_image.h (right):

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/include/cpdf_image.h:34: void Release();
Nit: Release() can be removed. Callers can directly call 'delete' since they all
check whether it can be deleted.

[Lei Zhang, 2016-08-04 21:58:26]

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:28: delete m_pImage;
On 2016/08/04 18:00:39, Wei Li wrote:
> On 2016/08/01 22:08:03, Lei Zhang wrote:
> > Not sure what else we can do here.
> 
> 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. 

Does you comment apply to patch set 2 as well?

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:442: bool
CPDF_DocPageData::HasImage(const CPDF_Object* pImageStream) const {
On 2016/08/04 18:00:39, Wei Li wrote:
> Why adding an extra function which does the same as the first half of
> ReleaseImage()?

CPDF_ImageObject needs to know if CPDF_ImageObject is the owner of the
CPDF_Image, and only call ReleaseImage() if this returns true.

[Wei Li, 2016-08-04 23:36:40]

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:28: delete m_pImage;
On 2016/08/04 21:58:25, Lei Zhang wrote:
> On 2016/08/04 18:00:39, Wei Li wrote:
> > On 2016/08/01 22:08:03, Lei Zhang wrote:
> > > Not sure what else we can do here.
> > 
> > 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. 
> 
> Does you comment apply to patch set 2 as well?

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?

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2194393002/diff/20001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:442: bool
CPDF_DocPageData::HasImage(const CPDF_Object* pImageStream) const {
On 2016/08/04 21:58:25, Lei Zhang wrote:
> On 2016/08/04 18:00:39, Wei Li wrote:
> > Why adding an extra function which does the same as the first half of
> > ReleaseImage()?
> 
> CPDF_ImageObject needs to know if CPDF_ImageObject is the owner of the
> CPDF_Image, and only call ReleaseImage() if this returns true.

I think you may keep the change of patch 1 and let ReleaseImage() returns true
only when it is found in |m_ImageMap|. In this way, the same checking logic will
run only once.

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

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/1/core/fpdfapi/fpdf_page/cpdf...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:28: delete m_pImage;
On 2016/08/04 23:36:39, Wei Li wrote:
> On 2016/08/04 21:58:25, Lei Zhang wrote:
> > On 2016/08/04 18:00:39, Wei Li wrote:
> > > On 2016/08/01 22:08:03, Lei Zhang wrote:
> > > > Not sure what else we can do here.
> > > 
> > > 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. 
> > 
> > Does you comment apply to patch set 2 as well?
> 
> 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?

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().

[Lei Zhang, 2016-08-06 07:00:06]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 07:00:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-06 07:12:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 07:12:52]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-08-06 07:39:49]

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_image.cpp (left):

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_image.cpp:57: FX_BOOL
CPDF_Image::LoadImageF(CPDF_Stream* pStream, FX_BOOL bInline) {
Made into a ctor.

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_image.cpp:123: void
CPDF_Image::SetJpegImage(uint8_t* pData, uint32_t size) {
Dead code.

[Lei Zhang, 2016-08-09 18:46:40]

ping

[Wei Li, 2016-08-09 22:22:07]

sorry, didn't see the new update.

lgtm with a couple nits.

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:60: m_pImage = pImage.release();
std::move?

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:82: const CPDF_Object* pImageStream
= m_pImage->GetStream();
Nit: no need for |pImageStream|

[Lei Zhang, 2016-08-09 22:28:52]

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/cpdf_imageobject.cpp (right):

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:60: m_pImage = pImage.release();
On 2016/08/09 22:22:07, Wei Li wrote:
> std::move?

Can't. |m_pImage| is a raw pointer, because it can be used for unowned
CPDF_Images as well.

https://codereview.chromium.org/2194393002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/cpdf_imageobject.cpp:82: const CPDF_Object* pImageStream
= m_pImage->GetStream();
On 2016/08/09 22:22:07, Wei Li wrote:
> Nit: no need for |pImageStream|

Done.

[Lei Zhang, 2016-08-09 22:29:12]

The CQ bit was checked by thestig@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 22:29:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 22:44:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 22:44:05]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-08-09 22:46:01]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2016-08-09 22:46:01]

The patchset sent to the CQ was uploaded after l-g-t-m from weili@chromium.org
Link to the patchset: https://codereview.chromium.org/2194393002/#ps80001
(title: "nit")

[commit-bot: I haz the power, 2016-08-09 22:46:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 22:46:23]

Description was changed from

==========
Fix a leak with FPDFPageObj_NewImgeObj().

BUG=pdfium:545
==========

to

==========
Fix a leak with FPDFPageObj_NewImgeObj().

BUG=pdfium:545

Committed:
https://pdfium.googlesource.com/pdfium/+/dc359b03ab6a70ee52a91119ff6704cae92f...
==========

[commit-bot: I haz the power, 2016-08-09 22:46:24]

Committed patchset #5 (id:80001) as
https://pdfium.googlesource.com/pdfium/+/dc359b03ab6a70ee52a91119ff6704cae92f...