Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2648013002:
Fix crash in PDF accessibility when PDF engine reports -1 as char count. (Closed)
DescriptionFix crash in PDF accessibility when PDF engine reports -1 as char count.
The PDF engine was returning -1 from engine_->GetCharCount(), and we were
trying to allocate that many bytes. See bug for repro.
BUG=648981
Review-Url: https://codereview.chromium.org/2648013002
Cr-Commit-Position: refs/heads/master@{#447132}
Committed: https://chromium.googlesource.com/chromium/src/+/d45c2e686cfea25c8e480cbb5604ef1bc5dfae0f
Patch Set 1 #Patch Set 2 : Add test #
Total comments: 2
Patch Set 3 : Add link to bug #Patch Set 4 : Move file to pdf_private #
Total comments: 1
Patch Set 5 : Reword comment #
Messages
Total messages: 29 (14 generated)
The CQ bit was checked by dmazzoni@chromium.org to run a CQ dry run
dmazzoni@chromium.org changed reviewers: + raymes@chromium.org
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: This issue passed the CQ dry run.
Thanks! lg but could you please add a regression test? It should be as simple as adding a PDF to ./chrome/test/data/pdf I think. Please just make sure it gets loaded when the PDFExtensionTest.Load runs.
The CQ bit was checked by dmazzoni@chromium.org
Test added!
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
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. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm https://codereview.chromium.org/2648013002/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2648013002/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:662: IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibilityCharCountCrash) { nit: please add a comment with a link to the bug
https://codereview.chromium.org/2648013002/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2648013002/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:662: IN_PROC_BROWSER_TEST_F(PDFExtensionTest, PdfAccessibilityCharCountCrash) { On 2017/01/24 23:05:47, raymes wrote: > nit: please add a comment with a link to the bug Done.
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2648013002/#ps40001 (title: "Add link to bug")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Please review this change to add the test file to pdf_private first: https://chromereviews.googleplex.com/559577013/ Then this should be ready to go.
lgtm
https://codereview.chromium.org/2648013002/diff/60001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2648013002/diff/60001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:663: // Test a particular PDF encountered in the wild that encountered a crash nit: encountered a crash->triggered a crash
The CQ bit was checked by dmazzoni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2648013002/#ps80001 (title: "Reword comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/01/30 18:58:04, raymes wrote: > https://codereview.chromium.org/2648013002/diff/60001/chrome/browser/pdf/pdf_... > File chrome/browser/pdf/pdf_extension_test.cc (right): > > https://codereview.chromium.org/2648013002/diff/60001/chrome/browser/pdf/pdf_... > chrome/browser/pdf/pdf_extension_test.cc:663: // Test a particular PDF > encountered in the wild that encountered a crash > nit: encountered a crash->triggered a crash Done
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1485819654160650,
"parent_rev": "69e1788a5ca7532e3bff055fb74738a0d0f95ffb", "commit_rev":
"d45c2e686cfea25c8e480cbb5604ef1bc5dfae0f"}
Message was sent while issue was closed.
Description was changed from ========== Fix crash in PDF accessibility when PDF engine reports -1 as char count. The PDF engine was returning -1 from engine_->GetCharCount(), and we were trying to allocate that many bytes. See bug for repro. BUG=648981 ========== to ========== Fix crash in PDF accessibility when PDF engine reports -1 as char count. The PDF engine was returning -1 from engine_->GetCharCount(), and we were trying to allocate that many bytes. See bug for repro. BUG=648981 Review-Url: https://codereview.chromium.org/2648013002 Cr-Commit-Position: refs/heads/master@{#447132} Committed: https://chromium.googlesource.com/chromium/src/+/d45c2e686cfea25c8e480cbb5604... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/d45c2e686cfea25c8e480cbb5604... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
