Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2960753002:
Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). (Closed)
DescriptionFix bug where Chromium crashes in PDFiumEngine::OnMouseUp().
Chromium previously crashed when a user right clicked outside of a form
text area, and then left clicked in the form text area. This was due to
an out of bounds error. The fix is checking that |last_page_mouse_down_|
is not -1 before trying to set the PDF plugin's selected text.
BUG=59266, 737529
Review-Url: https://codereview.chromium.org/2960753002
Cr-Commit-Position: refs/heads/master@{#483133}
Committed: https://chromium.googlesource.com/chromium/src/+/ea6109f1b3034ec6557598194c72c2c3533a7aa5
Patch Set 1 #
Total comments: 2
Messages
Total messages: 23 (12 generated)
Description was changed from ========== Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). Chromium previously crashed when a user right clicked outside of a form text area, and then left clicked in the form text area. This was due to an out of bounds error. The fix is checking that |last_page_mouse_down_| is not -1 before trying to set the PDF plugin's selected text. BUG=59266 ========== to ========== Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). Chromium previously crashed when a user right clicked outside of a form text area, and then left clicked in the form text area. This was due to an out of bounds error. The fix is checking that |last_page_mouse_down_| is not -1 before trying to set the PDF plugin's selected text. BUG=59266 ==========
drgage@google.com changed reviewers: + dsinclair@chromium.org, thestig@chromium.org
Hi Lei and Dan, In this CL, I have a fix to prevent Chromium from crashing due to an out of bounds error. Thanks for reviewing. Best, Diana
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2960753002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2960753002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:1860: if (area == PDFiumPage::FORM_TEXT_AREA && last_page_mouse_down_ != -1) This will definitely fix the crash, but another question that arises is whether we should be using |last_page_mouse_down_|, or |page_index|. From looking at the code: - |last_page_mouse_down_| is really "last_page_left_mouse_button_down_". - On a multi-page PDF, if the left mouse down occurs on page 2, the mouse button is held and dragged to page 1, and then released on page 1, then |last_page_mouse_down_| is 1 and |page_index| is 0. (Both variables are 0-based)
https://codereview.chromium.org/2960753002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2960753002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:1860: if (area == PDFiumPage::FORM_TEXT_AREA && last_page_mouse_down_ != -1) On 2017/06/27 01:26:51, Lei Zhang wrote: > This will definitely fix the crash, but another question that arises is whether > we should be using |last_page_mouse_down_|, or |page_index|. > > From looking at the code: > - |last_page_mouse_down_| is really "last_page_left_mouse_button_down_". > - On a multi-page PDF, if the left mouse down occurs on page 2, the mouse button > is held and dragged to page 1, and then released on page 1, then > |last_page_mouse_down_| is 1 and |page_index| is 0. (Both variables are 0-based) FORM_GetSelectedText returns the text from a text field or combo box which can't go over page boundaries. I'd hope that would mean last_page_mouse_down_ and page_index_ should match, otherwise, I'm not sure what that would mean?
On 2017/06/27 15:24:22, dsinclair wrote: > FORM_GetSelectedText returns the text from a text field or combo box which can't > go over page boundaries. I'd hope that would mean last_page_mouse_down_ and > page_index_ should match, otherwise, I'm not sure what that would mean? It would be a drag & drop, but let's not go there.
On 2017/06/27 20:27:20, Lei Zhang wrote: > On 2017/06/27 15:24:22, dsinclair wrote: > > FORM_GetSelectedText returns the text from a text field or combo box which > can't > > go over page boundaries. I'd hope that would mean last_page_mouse_down_ and > > page_index_ should match, otherwise, I'm not sure what that would mean? > > It would be a drag & drop, but let's not go there. I have been playing around with this on this multi-page PDF with forms: https://www.irs.gov/pub/irs-pdf/fw4.pdf In the case where the user types text into a form text field on one page, and then drags the mouse to the next page and releases the mouse within a normal text area, the selection on the first page remains. However, if the user releases the mouse on the second page within another form text field, the selection on the first page is cleared and the typing prompt appears in the second page's form text field. For this reason, I think it makes sense to keep it the way it is and use |last_page_mouse_down_| instead of |page_index|. Either the text on the first page is still highlighted, or nothing is highlighted depending on where the mouse is released on the second page. Because the first page's selected text is still visually highlighted in the first case I mentioned, I think the user should still be able to copy that text. What do you think?
Description was changed from ========== Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). Chromium previously crashed when a user right clicked outside of a form text area, and then left clicked in the form text area. This was due to an out of bounds error. The fix is checking that |last_page_mouse_down_| is not -1 before trying to set the PDF plugin's selected text. BUG=59266 ========== to ========== Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). Chromium previously crashed when a user right clicked outside of a form text area, and then left clicked in the form text area. This was due to an out of bounds error. The fix is checking that |last_page_mouse_down_| is not -1 before trying to set the PDF plugin's selected text. BUG=59266,737529 ==========
The selection is still wonky, but let's do this for the sake of fixing the crash. I added 737529 to the BUG= line as well.
lgtm
The CQ bit was checked by drgage@google.com
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by drgage@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1498681166086130, "parent_rev":
"2fad84b7f174ec2ba14a29b60f1e0d1d70654341", "commit_rev":
"ea6109f1b3034ec6557598194c72c2c3533a7aa5"}
Message was sent while issue was closed.
Description was changed from ========== Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). Chromium previously crashed when a user right clicked outside of a form text area, and then left clicked in the form text area. This was due to an out of bounds error. The fix is checking that |last_page_mouse_down_| is not -1 before trying to set the PDF plugin's selected text. BUG=59266,737529 ========== to ========== Fix bug where Chromium crashes in PDFiumEngine::OnMouseUp(). Chromium previously crashed when a user right clicked outside of a form text area, and then left clicked in the form text area. This was due to an out of bounds error. The fix is checking that |last_page_mouse_down_| is not -1 before trying to set the PDF plugin's selected text. BUG=59266,737529 Review-Url: https://codereview.chromium.org/2960753002 Cr-Commit-Position: refs/heads/master@{#483133} Committed: https://chromium.googlesource.com/chromium/src/+/ea6109f1b3034ec6557598194c72... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/ea6109f1b3034ec6557598194c72... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
