Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491583002:
Fix receiving page, if it have not obj num. (Closed)
DescriptionFix receiving page, if it have not obj num.
In some PDF's the page may not have the obj num.
For example: testing\corpus\fx\other\jetman_std.pdf in pdfium repository.
And CPDF_Document::GetPage failed on second call for this page.
Restart the traversing of pages, to fix this
Also added test.
Committed: https://pdfium.googlesource.com/pdfium/+/c0a47773d6dadeb8a39a6ced4ebbb1795e2e411f
Patch Set 1 #Patch Set 2 : Fix comments and test name. #Patch Set 3 : Fix signed/unsigned comparison #
Total comments: 16
Patch Set 4 : Fix receiving page, if it have not obj num. #
Total comments: 4
Patch Set 5 : Fix review issues #Patch Set 6 : Fix review issues #Messages
Total messages: 43 (30 generated)
Description was changed from ========== Fix receiving page, if it have not obj num. In some PDF's the page may not have the obj num. For exaple: testing\corpus\fx\other\jetman_std.pdf in pdfium repository. And CPDF_Document::GetPage failed on second call for this page. Restart the traversing of pages, to fix this Also added test. ========== to ========== Fix receiving page, if it have not obj num. In some PDF's the page may not have the obj num. For exaple: testing\corpus\fx\other\jetman_std.pdf in pdfium repository. And CPDF_Document::GetPage failed on second call for this page. Restart the traversing of pages, to fix this Also added test. ==========
art-snake@yandex-team.ru changed reviewers: + brucedawson@chromium.org, dsinclair@chromium.org, thestig@chromium.org
The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== Fix receiving page, if it have not obj num. In some PDF's the page may not have the obj num. For exaple: testing\corpus\fx\other\jetman_std.pdf in pdfium repository. And CPDF_Document::GetPage failed on second call for this page. Restart the traversing of pages, to fix this Also added test. ========== to ========== Fix receiving page, if it have not obj num. In some PDF's the page may not have the obj num. For example: testing\corpus\fx\other\jetman_std.pdf in pdfium repository. And CPDF_Document::GetPage failed on second call for this page. Restart the traversing of pages, to fix this Also added test. ==========
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/...) linux on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/linux/builds/2728) mac_xfa_rel on master.tryserver.client.pdfium (JOB_FAILED, https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa_rel/bui...)
The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-pdfium-committers". Note that this has nothing to do with OWNERS files.
dsinclair@chromium.org changed reviewers: + npm@chromium.org - brucedawson@chromium.org
npm@ is this related to the stuff you were working on?
Yes, I would also recommend npm as a reviewer. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:497: // This can be happened, when page have not objNum. Rephrase: "This can happen when the page does not have an object number. On repeated calls to this function for the same page index, this condition causes TraversePDFPages() to incorrectly return nullptr." https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:500: // Example "testing\corpus\fx\other\jetman_std.pdf" I would prefer to use "/" for path separator, since that's what we use for include paths, and I'm biased against Windows. ;-) https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:505: nPagesToGo = iPage - m_iNextPageToTraverse + 1; nit: You already know |m_iNextPageToTraverse| is 0, so just omit it? https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:112: const auto page = document->GetPage(2); There's room on this line, so let's write const CPDF_Dictionary* because it's not obvious that |page| is a dictionary and not some other class that represents a page. At the very least, auto* for raw pointers.
https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:497: // This can be happened, when page have not objNum. On 2016/11/09 20:19:41, Lei Zhang (slow) wrote: > Rephrase: "This can happen when the page does not have an object number. On > repeated calls to this function for the same page index, this condition causes > TraversePDFPages() to incorrectly return nullptr." Done. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:500: // Example "testing\corpus\fx\other\jetman_std.pdf" On 2016/11/09 20:19:41, Lei Zhang (slow) wrote: > I would prefer to use "/" for path separator, since that's what we use for > include paths, and I'm biased against Windows. ;-) Done. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:505: nPagesToGo = iPage - m_iNextPageToTraverse + 1; On 2016/11/09 20:19:41, Lei Zhang (slow) wrote: > nit: You already know |m_iNextPageToTraverse| is 0, so just omit it? Done. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:112: const auto page = document->GetPage(2); On 2016/11/09 20:19:41, Lei Zhang (slow) wrote: > There's room on this line, so let's write const CPDF_Dictionary* because it's > not obvious that |page| is a dictionary and not some other class that represents > a page. At the very least, auto* for raw pointers. Done.
I mostly assumed this was not the case, as this is stated in the PDF spec. But it's nice to fix this. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:496: if (nPagesToGo <= 0) { This could also happen in cases where we just skip that page number. See places in TraversePDFPages where m_pTreeTraversal[level].second++ but m_PageList is not updated (no idea how common this is). https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:502: // TODO(art-snake): optimize this. To optimize, we could store the dict pointer when objnum is 0. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:503: m_pTreeTraversal.clear(); nit: use ResetTraversal() instead. https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:47: zeroToTwo->Add(CreateNumberedPage(2)); Maybe add an example instead of changing this one?
On 2016/11/09 20:32:56, npm wrote: > I mostly assumed this was not the case, as this is stated > in the PDF spec. But it's nice to fix this. FYI: Acrobat displays jetman_std.pdf, and hints at it being bad because when you close it, it offers to save the PDF even though the user did not make any explicit modifications. Libpoppler-based PDF readers refuse to open jetman_std.pdf.
On 2016/11/09 20:39:58, Lei Zhang (slow) wrote: > On 2016/11/09 20:32:56, npm wrote: > > I mostly assumed this was not the case, as this is stated > > in the PDF spec. But it's nice to fix this. > > FYI: > > Acrobat displays jetman_std.pdf, and hints at it being bad because when you > close it, it offers to save the PDF even though the user did not make any > explicit modifications. > > Libpoppler-based PDF readers refuse to open jetman_std.pdf. Yes. I tried to get rid of this test, but instead was told to add a corrected version :)
https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:496: if (nPagesToGo <= 0) { On 2016/11/09 20:32:56, npm wrote: > This could also happen in cases where we just skip that page number. > See places in TraversePDFPages where m_pTreeTraversal[level].second++ but > m_PageList is not updated (no idea how common this is). I see this before. I will check this case. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:502: // TODO(art-snake): optimize this. On 2016/11/09 20:32:56, npm wrote: > To optimize, we could store the dict pointer when objnum is 0. I should think about this. May be more better generate objNum for such pages. I'm not sure now. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:503: m_pTreeTraversal.clear(); On 2016/11/09 20:32:56, npm wrote: > nit: use ResetTraversal() instead. Done. https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:47: zeroToTwo->Add(CreateNumberedPage(2)); On 2016/11/09 20:32:56, npm wrote: > Maybe add an example instead of changing this one? I think it should be at this place. Because, the test GetPagesReverseOrder also was failed with page, which have zero objNum. And i want, that new document's tests also use this by default.
Looks good but I still think we should not use this case as our default test. https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:502: // TODO(art-snake): optimize this. On 2016/11/09 20:46:47, snake wrote: > On 2016/11/09 20:32:56, npm wrote: > > To optimize, we could store the dict pointer when objnum is 0. > > I should think about this. May be more better generate objNum for such pages. > I'm not sure now. AFAIK objNum is 0 for CPDF_Object when its direct reference.Maybe Tom has some idea about the best way to obtain/store something in this case. https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:47: zeroToTwo->Add(CreateNumberedPage(2)); On 2016/11/09 20:46:47, snake wrote: > On 2016/11/09 20:32:56, npm wrote: > > Maybe add an example instead of changing this one? > > I think it should be at this place. Because, the test GetPagesReverseOrder also > was failed with page, which have zero objNum. And i want, that new document's > tests also use this by default. The reason it would fail GetPagesReverseOrder is the same as for GetPageWithoutObjNumTwice. And I prefer to have a default test with a tree that conforms to PDF spec.
https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document.cpp (right): https://codereview.chromium.org/2491583002/diff/40001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document.cpp:502: // TODO(art-snake): optimize this. On 2016/11/09 21:55:03, npm wrote: > On 2016/11/09 20:46:47, snake wrote: > > On 2016/11/09 20:32:56, npm wrote: > > > To optimize, we could store the dict pointer when objnum is 0. > > > > I should think about this. May be more better generate objNum for such pages. > > I'm not sure now. > > AFAIK objNum is 0 for CPDF_Object when its direct reference.Maybe Tom has > some idea about the best way to obtain/store something in this case. Anyway, i will do this in other CL. https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... File core/fpdfapi/parser/cpdf_document_unittest.cpp (right): https://codereview.chromium.org/2491583002/diff/60001/core/fpdfapi/parser/cpd... core/fpdfapi/parser/cpdf_document_unittest.cpp:47: zeroToTwo->Add(CreateNumberedPage(2)); On 2016/11/09 21:55:03, npm wrote: > On 2016/11/09 20:46:47, snake wrote: > > On 2016/11/09 20:32:56, npm wrote: > > > Maybe add an example instead of changing this one? > > > > I think it should be at this place. Because, the test GetPagesReverseOrder > also > > was failed with page, which have zero objNum. And i want, that new document's > > tests also use this by default. > > The reason it would fail GetPagesReverseOrder is the same as for > GetPageWithoutObjNumTwice. And I prefer to have a default test with > a tree that conforms to PDF spec. Done.
lgtm
The CQ bit was checked by art-snake@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix receiving page, if it have not obj num. In some PDF's the page may not have the obj num. For example: testing\corpus\fx\other\jetman_std.pdf in pdfium repository. And CPDF_Document::GetPage failed on second call for this page. Restart the traversing of pages, to fix this Also added test. ========== to ========== Fix receiving page, if it have not obj num. In some PDF's the page may not have the obj num. For example: testing\corpus\fx\other\jetman_std.pdf in pdfium repository. And CPDF_Document::GetPage failed on second call for this page. Restart the traversing of pages, to fix this Also added test. Committed: https://pdfium.googlesource.com/pdfium/+/c0a47773d6dadeb8a39a6ced4ebbb1795e2e... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:40002) as https://pdfium.googlesource.com/pdfium/+/c0a47773d6dadeb8a39a6ced4ebbb1795e2e... |
