Adjust the order of clearing resource in CPDF_DocPageData::Clear

Adjust the order of clearing resource in CPDF_DocPageData::Clear

Images are basic resource and are referred or used by other objects in some cases. Images should be released after the objects who uses these objects. In this case, an image object is accessed in the process of CPDF_TilingPattern's destroy. Unlikely, this image has been destroyed before.

BUG=414046
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/26019d4a79c84843c710cd9505bd40e9da0ca4c6

[jun_fang, 2014-09-18 23:58:41]

jun_fang@foxitsoftware.com changed reviewers:
+ bo_xu@foxitsoftware.com, tsepez@chromium.org

[jun_fang, 2014-09-18 23:59:06]

Hi guys, please review this fix. Thanks!

[Tom Sepez, 2014-09-19 17:36:31]

Two nits, one question. Thanks!

https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:157: pos =
m_FontMap.GetStartPosition();
Nit: need a small comment here explaining that compound objects need to be
deleted before simple ones or some such.

https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:218: nCount = ptData->m_nCount;
nit: if ptObj->m_obj is NULL, we can short-circuit execution here with a
"continue" before even considering m_nCount. And you may not need the local
m_nCount since its only used once (in the if below) and ptData->m_nCount is just
fine to use there. I think the loop reads clearer that way.

https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:234: delete imageData->m_Obj;
do we need to check imageData->m_obj for NULLness here too?

[Tom Sepez, 2014-09-19 17:40:38]

https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:154: FX_DWORD	nCount;
In fact, we can remove the nCount local entirely, since it isn't adding any
value beyond directly using the left-hand-side of whatever you set it to.

[jun_fang, 2014-09-19 18:24:22]

On 2014/09/19 17:40:38, Tom Sepez wrote:
>
https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
> File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):
> 
>
https://codereview.chromium.org/582993002/diff/1/core/src/fpdfapi/fpdf_page/f...
> core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:154: FX_DWORD	nCount;
> In fact, we can remove the nCount local entirely, since it isn't adding any
> value beyond directly using the left-hand-side of whatever you set it to.

Hi Tom, please see the change in patch 2. I will add a comment to explain the
rule that a compound objects should be released before simple ones with any
other changes in patch 3.

[Tom Sepez, 2014-09-19 18:33:14]

Getting closer.  Thanks.

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

https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:161: if (!fontData ||
!fontData->m_Obj) {
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).

https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:187: if (!ipKey || !ipData) {
Don't think these can be NULL, can they?

https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:206:
m_IccProfileMap.RemoveKey(ipKey);
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?

https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:214: if (!ftData) {
same here.

https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:247: if (!imageData) {
same here.

[jun_fang, 2014-09-19 19:17:58]

On 2014/09/19 18:33:14, Tom Sepez wrote:
> Getting closer.  Thanks.
> 
>
https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
> File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):
> 
>
https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:161: if (!fontData ||
> !fontData->m_Obj) {
> 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).
> 
>
https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:187: if (!ipKey || !ipData) {
> Don't think these can be NULL, can they?
> 
>
https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:206:
> m_IccProfileMap.RemoveKey(ipKey);
> 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?
> 
>
https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:214: if (!ftData) {
> same here.
> 
>
https://codereview.chromium.org/582993002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:247: if (!imageData) {
> same here.

I agree that keys and data not NULL in most normal cases. I didn't check all
possibilities and only thought 
in a defensive way. It's fine for me to remove them if you think it's clearer
without them.

[Tom Sepez, 2014-09-19 20:20:04]

> It's fine for me to remove them if you think it's clearer
> without them.

Yeah, lets remove them if it's OK with you.  I think the cleanup here is
complicated enough as it is.

[jun_fang, 2014-09-19 21:18:06]

On 2014/09/19 20:20:04, Tom Sepez wrote:
> > It's fine for me to remove them if you think it's clearer
> > without them.
> 
> Yeah, lets remove them if it's OK with you.  I think the cleanup here is
> complicated enough as it is.

please check patch 3.

[Tom Sepez, 2014-09-19 21:37:43]

LGTM, with a few minor nits I'm almost ashamed to ask you to fix.

https://codereview.chromium.org/582993002/diff/40001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/582993002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:157: //Release objects saved in the
resource maps like font map and color space map.
nit: space between // and Release.

https://codereview.chromium.org/582993002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:158: //The compound objects shall
be released before simple ones .
nit: no space before period.

https://codereview.chromium.org/582993002/diff/40001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:181: //csData->m_Obj is deleted in
the function of ReleaseCS().
nit: ditto for space after //

[jun_fang, 2014-09-19 21:52:22]

Committed patchset #4 (id:60001) manually as 26019d4 (presubmit successful).