PDF: Always call FPDFAvail_IsDocAvail() when loading.

PDF: Always call FPDFAvail_IsDocAvail() when loading.

BUG=613704
Committed: https://crrev.com/0968faa3214a03bc763d15fd66943ff3bcaf75d5
Cr-Commit-Position: refs/heads/master@{#400286}

[Lei Zhang, 2016-05-23 23:24:52]

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

[Lei Zhang, 2016-05-23 23:24:52]

Internally in PDFium, if FPDFAvail_IsDocAvail() does not get called, then
CPDF_DataAvail::CheckDocStatus() does not get called. This leads us into a
situation where we never reach CPDF_DataAvail::CheckHintTables() to read the
hint table. Without a hint table, calling FPDFAvail_IsPageAvail() for 1000+
pages is very sloooooooow.

We can speed up FPDFAvail_IsPageAvail() for when a hint table is missing as a
separately issue, but the most common case for linearized PDFs is to have a
proper hint table.

[Wei Li, 2016-05-24 00:21:28]

https://codereview.chromium.org/2006793003/diff/20001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (left):

https://codereview.chromium.org/2006793003/diff/20001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2395: !FPDFAvail_IsDocAvail(fpdf_availability_,
&download_hints_)) {
Should these conditions be ORed instead?

[Lei Zhang, 2016-05-25 17:16:36]

I scrapped the approach in patch set 2 and I'm trying something different in
patch set 4.

https://codereview.chromium.org/2006793003/diff/20001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (left):

https://codereview.chromium.org/2006793003/diff/20001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2395: !FPDFAvail_IsDocAvail(fpdf_availability_,
&download_hints_)) {
On 2016/05/24 00:21:27, Wei Li wrote:
> Should these conditions be ORed instead?

No, we have to continue past this point to turn |doc_| non-NULL. If it's ORed,
then this if statement always evals to false. Basically it's "we are looking for
a reason to try to load the document" so only return early if no such reason
exists.

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:1052: DCHECK(fpdf_availability_);
I'm pretty sure OnPartialDocumentLoaded() has to be called before we can get to
this function.

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2442: int avail =
FPDFAvail_IsDocAvail(fpdf_availability_, &download_hints_);
I decided to (potentially) call this a second time just after loading the
document instead, like we do in samples/pdfium_test.cc.

[Wei Li, 2016-05-25 22:37:39]

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:1052: DCHECK(fpdf_availability_);
On 2016/05/25 17:16:36, Lei Zhang wrote:
> I'm pretty sure OnPartialDocumentLoaded() has to be called before we can get
to
> this function.

Acknowledged.

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2442: int avail =
FPDFAvail_IsDocAvail(fpdf_availability_, &download_hints_);
On 2016/05/25 17:16:36, Lei Zhang wrote:
> I decided to (potentially) call this a second time just after loading the
> document instead, like we do in samples/pdfium_test.cc.

Would this approach be fragile? In samples/pdfium_test.cc, it uses a loop to
wait until it becomes available. FPDFAvail_IsDocAvail calls
CPDF_DataAvail::CheckDocStatus which requires to be called at least three times
to read the hint table.

[Lei Zhang, 2016-06-14 17:53:56]

This one fell off the radar.

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2442: int avail =
FPDFAvail_IsDocAvail(fpdf_availability_, &download_hints_);
On 2016/05/25 22:37:38, Wei Li wrote:
> On 2016/05/25 17:16:36, Lei Zhang wrote:
> > I decided to (potentially) call this a second time just after loading the
> > document instead, like we do in samples/pdfium_test.cc.
> 
> Would this approach be fragile? In samples/pdfium_test.cc, it uses a loop to
> wait until it becomes available. FPDFAvail_IsDocAvail calls
> CPDF_DataAvail::CheckDocStatus which requires to be called at least three
times
> to read the hint table. 

Whether FPDFAvail_IsDocAvail() ends up reading the hint table depends on the
document. I think the DCHECK assertion may be a bit strong and I will remove it.
The goal here is not to make sure it returns PDF_DATA_AVAIL. (though it probably
should for well-formed documents) Instead, we just want to make sure
FPDFAvail_IsDocAvail() gets called at least once, so it has a chance to progress
through the CheckDocStatus() calls.

I tested with a modified pdfium_test where I don't run it in a loop and it
works.

[Wei Li, 2016-06-14 23:33:43]

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/2006793003/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2442: int avail =
FPDFAvail_IsDocAvail(fpdf_availability_, &download_hints_);
On 2016/06/14 17:53:56, Lei Zhang wrote:
> On 2016/05/25 22:37:38, Wei Li wrote:
> > On 2016/05/25 17:16:36, Lei Zhang wrote:
> > > I decided to (potentially) call this a second time just after loading the
> > > document instead, like we do in samples/pdfium_test.cc.
> > 
> > Would this approach be fragile? In samples/pdfium_test.cc, it uses a loop to
> > wait until it becomes available. FPDFAvail_IsDocAvail calls
> > CPDF_DataAvail::CheckDocStatus which requires to be called at least three
> times
> > to read the hint table. 
> 
> Whether FPDFAvail_IsDocAvail() ends up reading the hint table depends on the
> document. I think the DCHECK assertion may be a bit strong and I will remove
it.
> The goal here is not to make sure it returns PDF_DATA_AVAIL. (though it
probably
> should for well-formed documents) Instead, we just want to make sure
> FPDFAvail_IsDocAvail() gets called at least once, so it has a chance to
progress
> through the CheckDocStatus() calls.
> 
> I tested with a modified pdfium_test where I don't run it in a loop and it
> works.

I think the fix is in right direction. But we need to be extra careful here. If
I understand correctly, if the pdf file is not available at this moment, the
subsequent LoadDocument/FPDFAvail_IsDocAvail will not be called any more since
doc_ is set already.

[Lei Zhang, 2016-06-15 01:11:16]

On 2016/06/14 23:33:43, Wei Li wrote:
> I think the fix is in right direction. But we need to be extra careful here.
If
> I understand correctly, if the pdf file is not available at this moment, the
> subsequent LoadDocument/FPDFAvail_IsDocAvail will not be called any more since
> doc_ is set already.

It's probably not needed, but I'll call FPDFAvail_IsDocAvail() again in the
early return case in TryLoadingDoc() just to be on the safe side.

[Lei Zhang, 2016-06-16 20:13:56]

PTAL at patch set 7.

[Wei Li, 2016-06-16 21:32:15]

lgtm

[Lei Zhang, 2016-06-16 21:39:08]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2016-06-16 21:39:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2006793003/120001

[commit-bot: I haz the power, 2016-06-16 23:06:56]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-06-16 23:10:17]

Description was changed from

==========
PDF: Always call FPDFAvail_IsDocAvail() when loading.

BUG=613704
==========

to

==========
PDF: Always call FPDFAvail_IsDocAvail() when loading.

BUG=613704

Committed: https://crrev.com/0968faa3214a03bc763d15fd66943ff3bcaf75d5
Cr-Commit-Position: refs/heads/master@{#400286}
==========

[commit-bot: I haz the power, 2016-06-16 23:10:18]

Patchset 7 (id:??) landed as
https://crrev.com/0968faa3214a03bc763d15fd66943ff3bcaf75d5
Cr-Commit-Position: refs/heads/master@{#400286}