Fix loading page using hint tables.

Fix loading page using hint tables.

When linearized document have hint table,
The FPDFAvail_IsPageAvail return true, but
FPDF_LoadPage return nullptr, for non first pages.

This happens, bacause document not use hint tables, to load page.

To fix this, I force save the page's ObjNum in document.

R=npm, dsinclair

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

[dsinclair, 2016-10-20 18:06:50]

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

[dsinclair, 2016-10-20 18:06:51]

Adding tsepez@ and thestig@ as they've looked into the hints table before I
believe.

[Tom Sepez, 2016-10-20 18:15:59]

On 2016/10/20 18:06:51, dsinclair wrote:
> Adding tsepez@ and thestig@ as they've looked into the hints table before I
> believe.

Thanks! 

Do you have a .PDF file that triggers this condition that we can use to test
this fix?
To trigger this, do we just load a non-first page in the first FPDF_LoadPage()
call?

Unfortunately, cpdf_document.cpp has been unstable over the last few days, we
will need to rebase your CL once the dust settles.

[snake, 2016-10-20 18:26:21]

On 2016/10/20 18:15:59, Tom Sepez wrote:
> On 2016/10/20 18:06:51, dsinclair wrote:
> > Adding tsepez@ and thestig@ as they've looked into the hints table before I
> > believe.
> 
> Thanks! 
> 
> Do you have a .PDF file that triggers this condition that we can use to test
> this fix?
> To trigger this, do we just load a non-first page in the first FPDF_LoadPage()
> call?
> 
> Unfortunately, cpdf_document.cpp has been unstable over the last few days, we
> will need to rebase your CL once the dust settles.

some times it is reprodusing on 

http://www.major-landrover.ru/upload/attachments/f/9/f96aab07dab04ae89c8a509e...
(but i have PDF plugin with some changes, which I want to commit to chromium. I
have found this, while test my changes.)

What problem with cpdf_document.cpp ?? Now i found, that it have incorrect
mapping with page and it is index. (problem in CPDF_Document::TraversePDFPages
method)

[Tom Sepez, 2016-10-20 18:32:16]

> What problem with cpdf_document.cpp ?? Now i found, that it have incorrect
> mapping with page and it is index. (problem in CPDF_Document::TraversePDFPages
> method)

Sorry, the cpdf_document issue is unrelated to your change.  
7403f8a2a removed the CPDF_Document::TraversePDFPages() function entirely,
and replaced it with the former implementation.  So your changes, which are
probably good, no longer line up with the latest version.

[snake, 2016-10-20 19:38:12]

On 2016/10/20 18:32:16, Tom Sepez wrote:
> > What problem with cpdf_document.cpp ?? Now i found, that it have incorrect
> > mapping with page and it is index. (problem in
CPDF_Document::TraversePDFPages
> > method)
> 
> Sorry, the cpdf_document issue is unrelated to your change.  
> 7403f8a2a removed the CPDF_Document::TraversePDFPages() function entirely,
> and replaced it with the former implementation.  So your changes, which are
> probably good, no longer line up with the latest version.

Now, No needs changes in cpdf_document, i just add test for it on this case.

[Tom Sepez, 2016-10-20 20:03:54]

lgtm

[snake, 2016-10-20 20:08:44]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 20:08:51]

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-10-20 20:12:20]

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

[commit-bot: I haz the power, 2016-10-20 20:12:20]

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

[snake, 2016-10-20 20:20:40]

The CQ bit was unchecked by art-snake@yandex-team.ru

[snake, 2016-10-20 20:20:44]

The CQ bit was checked by art-snake@yandex-team.ru

[snake, 2016-10-20 20:20:44]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org
Link to the patchset: https://codereview.chromium.org/2437773003/#ps80001
(title: "fix compilation on linux.")

[commit-bot: I haz the power, 2016-10-20 20:20:51]

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-10-20 20:29:47]

Description was changed from

==========
Fix loading page using hint tables.

When linearized document have hint table,
The FPDFAvail_IsPageAvail return true, but
FPDF_LoadPage return nullptr, for non first pages.

This happens, bacause document not use hint tables, to load page.

To fix this, I force save the page's ObjNum in document.

R=npm, dsinclair
==========

to

==========
Fix loading page using hint tables.

When linearized document have hint table,
The FPDFAvail_IsPageAvail return true, but
FPDF_LoadPage return nullptr, for non first pages.

This happens, bacause document not use hint tables, to load page.

To fix this, I force save the page's ObjNum in document.

R=npm, dsinclair

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

[commit-bot: I haz the power, 2016-10-20 20:29:48]

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

[Lei Zhang, 2016-10-20 21:18:43]

https://codereview.chromium.org/2437773003/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_document.cpp (right):

https://codereview.chromium.org/2437773003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_document.cpp:428: if
(CPDF_ModuleMgr::Get()->GetPageModule()) {
Is this to work around the crash in the unit test? Given that all the
GetPageModule() calls assume it won't return a nullptr, we could have changed
the unit test to act more like the real code, rather than changing the real code
to accommodate the unit test.

[Lei Zhang, 2016-10-20 21:43:36]

https://codereview.chromium.org/2437773003/diff/60001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_document.cpp (right):

https://codereview.chromium.org/2437773003/diff/60001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_document.cpp:428: if
(CPDF_ModuleMgr::Get()->GetPageModule()) {
On 2016/10/20 21:18:43, Lei Zhang wrote:
> Is this to work around the crash in the unit test? Given that all the
> GetPageModule() calls assume it won't return a nullptr, we could have changed
> the unit test to act more like the real code, rather than changing the real
code
> to accommodate the unit test.

See fpdfsdk/fpdfdoc_unittest.cpp for an example of how to do this the other way.

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_data_avail.cpp (right):

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_data_avail.cpp:1629: m_pDocument->SetPageObjNum(dwPage,
GetPage(dwPage)->GetObjNum());
Can GetPage() return a nullptr here?

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_data_avail.cpp:1768: if (!dwObjNum)
You should check |bPagePosGot| first, because if it returns false, then we
already know |dwObjNum| is not valid.

i.e. how about:

if (!bPagePosGot || !dwObjNum)
  return nullptr;

...  // continue with GetIndirectObject()

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_document_unittest.cpp (right):

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_document_unittest.cpp:25: std::unique_ptr<CPDF_Parser>
parser(new CPDF_Parser());
BTW, we have pdfium::MakeUnique, so please take advantage of that.

[npm, 2016-10-21 16:42:20]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/2442663005/ by npm@chromium.org.

The reason for reverting is: CPDF_DataAvail::IsPageAvail is causing crashes.

BUG=chromium:658168, chromium:658170.

[Lei Zhang, 2016-10-21 17:15:53]

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_data_avail.cpp (right):

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_data_avail.cpp:1629: m_pDocument->SetPageObjNum(dwPage,
GetPage(dwPage)->GetObjNum());
On 2016/10/20 21:43:36, Lei Zhang wrote:
> Can GetPage() return a nullptr here?

Based on the new crashes that caused this revert... yes, it can.

[snake, 2016-10-24 15:38:31]

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
File core/fpdfapi/parser/cpdf_data_avail.cpp (right):

https://codereview.chromium.org/2437773003/diff/80001/core/fpdfapi/parser/cpd...
core/fpdfapi/parser/cpdf_data_avail.cpp:1629: m_pDocument->SetPageObjNum(dwPage,
GetPage(dwPage)->GetObjNum());
On 2016/10/21 17:15:53, Lei Zhang wrote:
> On 2016/10/20 21:43:36, Lei Zhang wrote:
> > Can GetPage() return a nullptr here?
> 
> Based on the new crashes that caused this revert... yes, it can.

But should not, because the page should be available at this moment.