[PDF] Fix crash when double clicking at the end of a page
Double-click text selection at the end of a page is a literal edge case that PDFiumEngine::OnMultipleClick() does not handle correctly. This method creates a PDFiumRange where the character index is out of bounds. In older code, this would have triggered a DCHECK() failure, but not crash when DCHECKs are turned off. After https://crrev.com/1417508, the DCHECK() failure still happens. When DCHECKs are turned off, code execution eventually triggers a crash, because that CL assumed the character index is never out of bounds. Add a test case that triggers this crash, and fix the crash by calculating the character index correctly in OnMultipleClick(). Additionally: 1) Add another test case for double clicking on a blank page, to help provide more test coverage, and to show OnMultipleClick() does not need to handle pages with no text. 2) Upgrade a cheap DCHECK_GE() to CHECK_GE(). Bug: 401992988 Change-Id: I60e1a4afb7cbb956205ce8f304b15701cf762ce7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6347717 Reviewed-by: Andy Phan <andyphan@chromium.org> Commit-Queue: Lei Zhang <thestig@chromium.org> Cr-Commit-Position: refs/heads/main@{#1431938}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
ae6ebc4a8d
commit
ac8c360346
@ -1341,33 +1341,39 @@ void PDFiumEngine::OnSingleClick(int page_index, int char_index) {
|
||||
void PDFiumEngine::OnMultipleClick(int click_count,
|
||||
int page_index,
|
||||
int char_index) {
|
||||
DCHECK_GE(click_count, 2);
|
||||
bool is_double_click = click_count == 2;
|
||||
CHECK_GE(click_count, 2);
|
||||
const bool is_double_click = click_count == 2;
|
||||
|
||||
const int char_count = pages_[page_index]->GetCharCount();
|
||||
CHECK_GT(char_count, 0);
|
||||
|
||||
// It would be more efficient if the SDK could support finding a space, but
|
||||
// now it doesn't.
|
||||
int start_index = char_index;
|
||||
do {
|
||||
uint32_t cur = pages_[page_index]->GetCharUnicode(start_index);
|
||||
if (FindMultipleClickBoundary(is_double_click, cur))
|
||||
if (FindMultipleClickBoundary(is_double_click, cur)) {
|
||||
break;
|
||||
}
|
||||
} while (--start_index >= 0);
|
||||
if (start_index)
|
||||
if (start_index && start_index < char_count - 1) {
|
||||
start_index++;
|
||||
}
|
||||
|
||||
int end_index = char_index;
|
||||
const int total = pages_[page_index]->GetCharCount();
|
||||
for (; end_index < total; ++end_index) {
|
||||
for (; end_index < char_count; ++end_index) {
|
||||
uint32_t cur = pages_[page_index]->GetCharUnicode(end_index);
|
||||
if (FindMultipleClickBoundary(is_double_click, cur))
|
||||
if (FindMultipleClickBoundary(is_double_click, cur)) {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
selection_.push_back(PDFiumRange(pages_[page_index].get(), start_index,
|
||||
end_index - start_index));
|
||||
|
||||
if (handling_long_press_)
|
||||
if (handling_long_press_) {
|
||||
client_->NotifyTouchSelectionOccurred();
|
||||
}
|
||||
}
|
||||
|
||||
bool PDFiumEngine::OnLeftMouseDown(const blink::WebMouseEvent& event) {
|
||||
|
@ -1139,6 +1139,40 @@ TEST_P(PDFiumEngineTest, SelectTextAcrossEmptyPage) {
|
||||
EXPECT_EQ(kExpectedAllSelection, engine->GetSelectedText());
|
||||
}
|
||||
|
||||
TEST_P(PDFiumEngineTest, SelectTextWithDoubleClickOnEmptyPage) {
|
||||
NiceMock<MockTestClient> client;
|
||||
std::unique_ptr<PDFiumEngine> engine =
|
||||
InitializeEngine(&client, FILE_PATH_LITERAL("blank.pdf"));
|
||||
ASSERT_TRUE(engine);
|
||||
|
||||
// Plugin size chosen so all pages of the document are visible.
|
||||
engine->PluginSizeUpdated({1024, 4096});
|
||||
|
||||
constexpr gfx::PointF kPosition(100, 100);
|
||||
EXPECT_TRUE(engine->HandleInputEvent(MouseEventBuilder()
|
||||
.CreateLeftClickAtPosition(kPosition)
|
||||
.SetClickCount(2)
|
||||
.Build()));
|
||||
EXPECT_THAT(engine->GetSelectedText(), IsEmpty());
|
||||
}
|
||||
|
||||
TEST_P(PDFiumEngineTest, SelectTextWithDoubleClickAtEndOfPage) {
|
||||
NiceMock<MockTestClient> client;
|
||||
std::unique_ptr<PDFiumEngine> engine =
|
||||
InitializeEngine(&client, FILE_PATH_LITERAL("hello_world2.pdf"));
|
||||
ASSERT_TRUE(engine);
|
||||
|
||||
// Plugin size chosen so all pages of the document are visible.
|
||||
engine->PluginSizeUpdated({1024, 4096});
|
||||
|
||||
constexpr gfx::PointF kPosition(195, 130);
|
||||
EXPECT_TRUE(engine->HandleInputEvent(MouseEventBuilder()
|
||||
.CreateLeftClickAtPosition(kPosition)
|
||||
.SetClickCount(2)
|
||||
.Build()));
|
||||
EXPECT_THAT(engine->GetSelectedText(), IsEmpty());
|
||||
}
|
||||
|
||||
TEST_P(PDFiumEngineTest, DrawTextSelectionsHelloWorld) {
|
||||
constexpr int kPageIndex = 0;
|
||||
NiceMock<MockTestClient> client;
|
||||
|
Reference in New Issue
Block a user