Call FPDFAvail_IsFormAvail() before creating form.

Call FPDFAvail_IsFormAvail() before creating form.

FPDFAvail_IsFormAvail() must be called before the PDF form is
initialized if new data was downloaded since the document was created.

BUG=572943
Committed: https://crrev.com/a69e93f6ac1ee1a6e3554ea61e0461a83c75081d
Cr-Commit-Position: refs/heads/master@{#367191}

[spelchat, 2015-12-30 00:58:53]

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

[Lei Zhang, 2015-12-30 01:09:21]

https://codereview.chromium.org/1556603002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1556603002/diff/1/pdf/pdfium/pdfium_engine.cc...
pdf/pdfium/pdfium_engine.cc:2603: if (!FPDFAvail_IsFormAvail(fpdf_availability_,
&download_hints_) &&
How about we call these 2 functions separately, and then compare their results?
It's probably better to do more checking for FPDFAvail_IsFormAvail() since it's
not returning a bool. I think you'd want to make sure it returned
PDF_FORM_AVAIL.

[spelchat, 2015-12-30 17:14:58]

https://codereview.chromium.org/1556603002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/1556603002/diff/1/pdf/pdfium/pdfium_engine.cc...
pdf/pdfium/pdfium_engine.cc:2603: if (!FPDFAvail_IsFormAvail(fpdf_availability_,
&download_hints_) &&
On 2015/12/30 01:09:21, Lei Zhang wrote:
> How about we call these 2 functions separately, and then compare their
results?
> It's probably better to do more checking for FPDFAvail_IsFormAvail() since
it's
> not returning a bool. I think you'd want to make sure it returned
> PDF_FORM_AVAIL.

PDF_FORM_NOTEXIST is OK too, it just means there is no form in the PDF.
PDF_FORM_ERROR shouldn't happen.

I've massaged the code a bit to make things clearer, !FPDFAvail_IsFormAvail()
was confusing.

[Lei Zhang, 2015-12-30 17:48:26]

lgtm

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

https://codereview.chromium.org/1556603002/diff/20001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2602: DCHECK(form_status != PDF_FORM_ERROR);
DCHECK_NE()

[spelchat, 2015-12-30 17:53:37]

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

https://codereview.chromium.org/1556603002/diff/20001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2602: DCHECK(form_status != PDF_FORM_ERROR);
On 2015/12/30 17:48:26, Lei Zhang wrote:
> DCHECK_NE()

Done.

[spelchat, 2015-12-30 17:53:41]

The CQ bit was checked by spelchat@chromium.org

[spelchat, 2015-12-30 17:53:42]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1556603002/#ps40001
(title: "Use DCHECK_NE instead of DCHECK")

[commit-bot: I haz the power, 2015-12-30 17:53:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1556603002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1556603002/40001

[spelchat, 2015-12-30 18:29:51]

The CQ bit was unchecked by spelchat@chromium.org

[spelchat, 2015-12-30 19:01:08]

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

https://codereview.chromium.org/1556603002/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2600: if (fpdf_availability_) {
This check and DCHECK_NE could be removed instead. This is more in-line with
what's done in the rest of the file, but it seems odd to me to call
PDFAvail_IsFormAvail() with invalid arguments and just ignore the error.

[Lei Zhang, 2015-12-30 19:58:19]

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

https://codereview.chromium.org/1556603002/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2600: if (fpdf_availability_) {
On 2015/12/30 19:01:08, spelchat wrote:
> This check and DCHECK_NE could be removed instead. This is more in-line with
> what's done in the rest of the file, but it seems odd to me to call
> PDFAvail_IsFormAvail() with invalid arguments and just ignore the error.

I think for this to happen, the error from FPDFAvail_IsDocAvail() in
LoadDocument() got ignored as well.

Looking at the possible call stacks, OnPartialDocumentLoaded() is the only place
that calls FPDFAvail_Create() and FPDFAvail_Create() can't fail. However,
sometimes LoadDocument() is called from OnPendingRequestComplete() or
OnDocumentComplete() instead. It's not obvious if OnPartialDocumentLoaded()
should have been called already, but clearly there are cases where it did not
get called, thus needing this check.

Should we just make sure FPDFAvail_Create() gets called instead?

[spelchat, 2015-12-30 20:14:00]

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

https://codereview.chromium.org/1556603002/diff/60001/pdf/pdfium/pdfium_engin...
pdf/pdfium/pdfium_engine.cc:2600: if (fpdf_availability_) {
On 2015/12/30 19:58:19, Lei Zhang wrote:
> On 2015/12/30 19:01:08, spelchat wrote:
> > This check and DCHECK_NE could be removed instead. This is more in-line with
> > what's done in the rest of the file, but it seems odd to me to call
> > PDFAvail_IsFormAvail() with invalid arguments and just ignore the error.
> 
> I think for this to happen, the error from FPDFAvail_IsDocAvail() in
> LoadDocument() got ignored as well.
> 
> Looking at the possible call stacks, OnPartialDocumentLoaded() is the only
place
> that calls FPDFAvail_Create() and FPDFAvail_Create() can't fail. However,
> sometimes LoadDocument() is called from OnPendingRequestComplete() or
> OnDocumentComplete() instead. It's not obvious if OnPartialDocumentLoaded()
> should have been called already, but clearly there are cases where it did not
> get called, thus needing this check.
> 
> Should we just make sure FPDFAvail_Create() gets called instead?

Yes all the FPDFAvail_* functions get called but return an error which is
ignored. That's what I meant when I mentioned it would be more in-line with the
rest of the file to just remove this check and DCHECK_NE. Thinking about it now
I do think that's preferable to what I have now (although I'm not a big fan of
calling those functions when it doesn't make sense...).

I think all those functions only make sense if we have partial data for the
document, which is why FPDFAvail_Create() is only called in
OnPartialDocumentLoaded(). If the document has been fully downloaded before
LoadDocument() is called, then FPDFAvail_Create() is not called and all the
FPDFAvail functions return early.

I don't know if there would be a problem in using the same path for both cases
(loading with partial data or loading once the whole document is loaded). I
suspect the FPDFAvail_* functions may fail on non-linearized PDFs.

[Lei Zhang, 2015-12-30 20:43:35]

I'm guessing the original logic was "if the document was fully loaded, even if
there is no form, initialize |form_| anyway."

How about we go back to patch set 3, minus the DCHECK_NE(), to fix the bug at
hand?

I'll file a bug for checking FPDFAvail_Foo() return values for errors / better
error handling and someone can deal with that separately. I agree it's weird
that these functions return an int but the code treats them almost as though
they were bools.

[spelchat, 2015-12-30 20:50:54]

The CQ bit was checked by spelchat@chromium.org

[spelchat, 2015-12-30 20:50:55]

The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1556603002/#ps80001
(title: "Remove DCHECK_NE")

[commit-bot: I haz the power, 2015-12-30 20:52:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1556603002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1556603002/80001

[Lei Zhang, 2015-12-30 21:01:10]

++lgtm

Thanks for fixing the bug and exploring the slightly crummy code!

[spelchat, 2015-12-30 21:03:11]

On 2015/12/30 21:01:10, Lei Zhang wrote:
> ++lgtm
> 
> Thanks for fixing the bug and exploring the slightly crummy code!

Thank *you*!

[commit-bot: I haz the power, 2015-12-30 22:07:01]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-12-30 22:08:01]

Description was changed from

==========
Call FPDFAvail_IsFormAvail() before creating form.

FPDFAvail_IsFormAvail() must be called before the PDF form is
initialized if new data was downloaded since the document was created.

BUG=572943
==========

to

==========
Call FPDFAvail_IsFormAvail() before creating form.

FPDFAvail_IsFormAvail() must be called before the PDF form is
initialized if new data was downloaded since the document was created.

BUG=572943

Committed: https://crrev.com/a69e93f6ac1ee1a6e3554ea61e0461a83c75081d
Cr-Commit-Position: refs/heads/master@{#367191}
==========

[commit-bot: I haz the power, 2015-12-30 22:08:02]

Patchset 5 (id:??) landed as
https://crrev.com/a69e93f6ac1ee1a6e3554ea61e0461a83c75081d
Cr-Commit-Position: refs/heads/master@{#367191}