Fix an embedder test with leaked page object

Fix an embedder test with leaked page object

The public API FPDFPage_New() incorrectly said to use FPDFPage_Delete()
instead of FPDF_ClosePage() to free the new page. This led to a page object
leak in an embedder test. Correct the public API description
as well as its usage in the embedder test.

BUG=pdfium:242
Committed: https://pdfium.googlesource.com/pdfium/+/9b777deb00fb50dba37ccc1ee69767c6e04a3ee4

[Wei Li, 2016-08-19 05:33:55]

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

[commit-bot: I haz the power, 2016-08-19 05:33:57]

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-19 05:58:06]

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

[commit-bot: I haz the power, 2016-08-19 05:58:06]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2016-08-19 17:13:51]

Patchset #1 (id:1) has been deleted

[Wei Li, 2016-08-19 17:15:30]

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

[commit-bot: I haz the power, 2016-08-19 17:15:35]

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-19 17:34:03]

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

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

Dry run: This issue passed the CQ dry run.

[Wei Li, 2016-08-19 19:48:30]

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

[Wei Li, 2016-08-19 19:48:31]

pls review, thanks

[Tom Sepez, 2016-08-19 20:24:48]

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fpdfedit_embedd...
File fpdfsdk/fpdfedit_embeddertest.cpp (right):

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fpdfedit_embedd...
fpdfsdk/fpdfedit_embeddertest.cpp:69: // FPDFPage_New() doesn't add any new page
into document if the page index is
This comment should be in a public/ header file.  How does an embedder know if
the page was in range and hence which function to use?  Sounds like Delete()
should be smarter and just handle all cases.

[Tom Sepez, 2016-08-19 20:37:47]

Ok, here's a proposal.  In public/fpdf_edit.h, we add a comment to FPDFPage_New
that the actual index of the page may not be the same as the argument, since it
it limited to the current document length + 1.

Then replace the comment "The page should be deleted with |FPDFPage_Delete| when
finished." with the page should be closed with CPDF_ClosePage when finished as
with any other page in the document.

Then, I think the code above is right.

[Tom Sepez, 2016-08-19 20:41:18]

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fsdk_baseform_e...
File fpdfsdk/fsdk_baseform_embeddertest.cpp (right):

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fsdk_baseform_e...
fpdfsdk/fsdk_baseform_embeddertest.cpp:45: CBA_AnnotIterator iter(
I don't understand why these should change.  UnderLying is an implementation
detail and shouldn't be used in tests.

[Tom Sepez, 2016-08-19 21:02:16]

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fsdk_baseform_e...
File fpdfsdk/fsdk_baseform_embeddertest.cpp (right):

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fsdk_baseform_e...
fpdfsdk/fsdk_baseform_embeddertest.cpp:45: CBA_AnnotIterator iter(
On 2016/08/19 20:41:18, Tom Sepez wrote:
> I don't understand why these should change.  UnderLying is an implementation
> detail and shouldn't be used in tests.

Is this where the comment about ref-counts kicks in?  If so, we should keep the
result of getpageview in a local and release it later on,too.

[Wei Li, 2016-08-19 22:21:48]

Description was changed from

==========
Fix embedder tests with leaked objects

Both cases leak page objects. One needs to use proper function to
delete the page, the other needs to unload the pages as well as
using the method which doesn't increase the ref-count of the page.

BUG=pdfium:242
==========

to

==========
Fix an embedder test with leaked page object

The public API FPDFPage_New() incorrectly said to use FPDFPage_Delete()
instead of FPDF_ClosePage() to free the new page. This led to a page object leak
in an embedder
test. Correct the public API description as well as its usage in the embedder
test.

BUG=pdfium:242
==========

[Wei Li, 2016-08-19 22:22:31]

Description was changed from

==========
Fix an embedder test with leaked page object

The public API FPDFPage_New() incorrectly said to use FPDFPage_Delete()
instead of FPDF_ClosePage() to free the new page. This led to a page object leak
in an embedder
test. Correct the public API description as well as its usage in the embedder
test.

BUG=pdfium:242
==========

to

==========
Fix an embedder test with leaked page object

The public API FPDFPage_New() incorrectly said to use FPDFPage_Delete()
instead of FPDF_ClosePage() to free the new page. This led to a page object
leak in an embedder test. Correct the public API description
as well as its usage in the embedder test.

BUG=pdfium:242
==========

[Wei Li, 2016-08-19 22:25:27]

thanks for all the suggestions. Split these two cases now for easier review and
understanding.

ptal, thanks

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fpdfedit_embedd...
File fpdfsdk/fpdfedit_embeddertest.cpp (right):

https://codereview.chromium.org/2260683003/diff/20001/fpdfsdk/fpdfedit_embedd...
fpdfsdk/fpdfedit_embeddertest.cpp:69: // FPDFPage_New() doesn't add any new page
into document if the page index is
On 2016/08/19 20:24:48, Tom Sepez wrote:
> This comment should be in a public/ header file.  How does an embedder know if
> the page was in range and hence which function to use?  Sounds like Delete()
> should be smarter and just handle all cases.

Done.

[Wei Li, 2016-08-19 22:25:41]

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

[commit-bot: I haz the power, 2016-08-19 22:25:54]

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

[Tom Sepez, 2016-08-19 22:27:16]

lgtm

Perfect

[commit-bot: I haz the power, 2016-08-19 23:00:24]

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

[commit-bot: I haz the power, 2016-08-19 23:00:25]

Dry run: This issue passed the CQ dry run.

[Wei Li, 2016-08-19 23:19:24]

The CQ bit was checked by weili@chromium.org

[commit-bot: I haz the power, 2016-08-19 23:19:31]

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-19 23:19:48]

Description was changed from

==========
Fix an embedder test with leaked page object

The public API FPDFPage_New() incorrectly said to use FPDFPage_Delete()
instead of FPDF_ClosePage() to free the new page. This led to a page object
leak in an embedder test. Correct the public API description
as well as its usage in the embedder test.

BUG=pdfium:242
==========

to

==========
Fix an embedder test with leaked page object

The public API FPDFPage_New() incorrectly said to use FPDFPage_Delete()
instead of FPDF_ClosePage() to free the new page. This led to a page object
leak in an embedder test. Correct the public API description
as well as its usage in the embedder test.

BUG=pdfium:242

Committed:
https://pdfium.googlesource.com/pdfium/+/9b777deb00fb50dba37ccc1ee69767c6e04a...
==========

[commit-bot: I haz the power, 2016-08-19 23:19:49]

Committed patchset #4 (id:70001) as
https://pdfium.googlesource.com/pdfium/+/9b777deb00fb50dba37ccc1ee69767c6e04a...