Fix Regression: Incomplete file loading is seen for multi page pdf files.

Fix Regression: Incomplete file loading is seen for multi page pdf files.

This was introduced at PDFium revision 12a9940. There was a subtle logic
change for null |parray|.

BUG=415438
R=jun_fang@foxitsoftware.com

Committed: https://pdfium.googlesource.com/pdfium/+/e0399c76ba1412926a3d55f6328a095c23467951

[Tom Sepez, 2014-09-18 17:48:00]

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

[Tom Sepez, 2014-09-18 17:48:11]

Jun, please take a look.

[jun_fang, 2014-09-18 18:00:22]

https://codereview.chromium.org/579363002/diff/20001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/579363002/diff/20001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1048: nSegs =
arrIndex.size();
arrIndex may be empty here. If it's empty, we should add the default value (0,
size) into arrIndex.

[Tom Sepez, 2014-09-18 18:02:34]

https://codereview.chromium.org/579363002/diff/20001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1048: nSegs =
> arrIndex.size();
> arrIndex may be empty here. If it's empty, we should add the default value (0,
> size) into arrIndex.

Nope.  That's what introduced the regression.  If you look at the change in
12a9940, prior to that the code had the behaviour I now implement and no issue.

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

On 2014/09/18 18:02:34, Tom Sepez wrote:
>
https://codereview.chromium.org/579363002/diff/20001/core/src/fpdfapi/fpdf_pa...
> > core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1048: nSegs =
> > arrIndex.size();
> > arrIndex may be empty here. If it's empty, we should add the default value
(0,
> > size) into arrIndex.
> 
> Nope.  That's what introduced the regression.  If you look at the change in
> 12a9940, prior to that the code had the behaviour I now implement and no
issue.

Before the change of 12a9940, arrIndex almost has at least one item unless
|pArray->GetCount() == 0|.
In the change of 12a9940, we check more conditions to add items into arrIndex.
It's possible that 
arrIndex is empty but pArray->GetCount() is not 0. In this case, I added a
default value (0, size)
into arrIndex. The root cause of failing the regression test is that I missed to
add the default value 
when |pArray == NULL| rather than adding a default value for empty arrIndex.
right?

[Tom Sepez, 2014-09-18 18:41:58]

> I missed to add the default value when |pArray == NULL| rather than adding
> a default value for empty arrIndex. right?
Right. Gotcha.  See PS#3.

Since vector::size() is cheap, we're able to get rid of the separate nSegs
counter, too. This is cleaner since it prevents the possibility of a mismatch
between sizes.

[Tom Sepez, 2014-09-18 18:48:24]

Re PS#4: minor nit, but in the old days, C++ couldn't handle >> at the end of a
template invocation, because it got confused with the >> operator.  So the
convention is no space at front since the opening <'s are already separated, but
space in the back.

[jun_fang, 2014-09-18 18:54:35]

On 2014/09/18 18:48:24, Tom Sepez wrote:
> Re PS#4: minor nit, but in the old days, C++ couldn't handle >> at the end of
a
> template invocation, because it got confused with the >> operator.  So the
> convention is no space at front since the opening <'s are already separated,
but
> space in the back.

LGTM. Thanks!

[Tom Sepez, 2014-09-18 18:59:10]

Re PS#5: more tinkering, sorry.  A quick check for any botches on that one and
I'll land it.

[Tom Sepez, 2014-09-18 19:00:34]

PS#6 fixes a botch.

[jun_fang, 2014-09-18 19:02:50]

https://codereview.chromium.org/579363002/diff/80001/core/src/fpdfapi/fpdf_pa...
File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):

https://codereview.chromium.org/579363002/diff/80001/core/src/fpdfapi/fpdf_pa...
core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1036: CPDF_Object* pCountObj
= pArray->GetElement(i * 2 + 1);
Nit: doesn't align with the previous line.

[jun_fang, 2014-09-18 19:04:37]

On 2014/09/18 19:02:50, jun_fang wrote:
>
https://codereview.chromium.org/579363002/diff/80001/core/src/fpdfapi/fpdf_pa...
> File core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp (right):
> 
>
https://codereview.chromium.org/579363002/diff/80001/core/src/fpdfapi/fpdf_pa...
> core/src/fpdfapi/fpdf_parser/fpdf_parser_parser.cpp:1036: CPDF_Object*
pCountObj
> = pArray->GetElement(i * 2 + 1);
> Nit: doesn't align with the previous line.

LGTM for patch 6. It's much clearer than before. Thanks!

[Tom Sepez, 2014-09-18 19:12:01]

Committed patchset #7 (id:120001) manually as e0399c7 (presubmit successful).