0

[PDF Ink Signatures] Fix crash when drawing a new stroke after undo

When a user draws a stroke, undoes it, and draws a new stroke,
PdfInkModule::ApplyUndoRedoDiscards() will reuse the stroke ID for the
new stroke. However, PDFiumEngine does not remove entries from its
`ink_stroke_objects_map_`, so there will already be an entry for the
first stroke. This fails the invariants and causes a crash.

Fix this by discarding entries in PDFiumEngine when necessary.

Fixed: 378427083
Change-Id: I77d8214d89c94f423cca2005a19a201d5147e4c4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6015244
Commit-Queue: Andy Phan <andyphan@chromium.org>
Reviewed-by: Alan Screen <awscreen@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1382585}
This commit is contained in:
Andy Phan
2024-11-13 21:24:47 +00:00
committed by Chromium LUCI CQ
parent 0bb6bfb67a
commit 50cd386507
10 changed files with 141 additions and 5 deletions

@ -1049,10 +1049,14 @@ void PdfInkModule::ApplyUndoRedoDiscards(
const InkStrokeId start_id = *discards.begin();
for (auto& [page_index, page_ink_strokes] : strokes_) {
// Find the first element in `page_ink_strokes` whose ID >= `start_id`.
auto it = base::ranges::lower_bound(
auto start = base::ranges::lower_bound(
page_ink_strokes, start_id, {},
[](const FinishedStrokeState& state) { return state.id; });
page_ink_strokes.erase(it, page_ink_strokes.end());
auto end = page_ink_strokes.end();
for (auto it = start; it < end; ++it) {
client_->DiscardStroke(page_index, it->id);
}
page_ink_strokes.erase(start, end);
}
// Check the pages with strokes and remove the ones that are now empty.

@ -39,6 +39,10 @@ class PdfInkModuleClient {
virtual ~PdfInkModuleClient() = default;
// Asks the client to discard the stroke identified by `id` on the page at
// `page_index`.
virtual void DiscardStroke(int page_index, InkStrokeId id) {}
// Gets the current page orientation.
virtual PageOrientation GetOrientation() const = 0;

@ -233,6 +233,11 @@ class FakeClient : public PdfInkModuleClient {
(int page_index, InkStrokeId id, bool active),
(override));
MOCK_METHOD(void,
DiscardStroke,
(int page_index, InkStrokeId id),
(override));
void UpdateThumbnail(int page_index) override {
updated_thumbnail_page_indices_.push_back(page_index);
}
@ -1639,8 +1644,15 @@ TEST_F(PdfInkModuleUndoRedoTest, UndoRedoBetweenDraws) {
ElementsAre(Pair(
0, ElementsAreArray(kInitial4StrokeMatchersSpan.first(2u)))));
constexpr int kPageIndex = 0;
EXPECT_CALL(client(), DiscardStroke(kPageIndex, InkStrokeId(2)));
EXPECT_CALL(client(), DiscardStroke(kPageIndex, InkStrokeId(3)));
ApplyStrokeWithMouseAtPoints(
kMouseDownPoint3, base::span_from_ref(kMouseMovePoint3), kMouseUpPoint3);
VerifyAndClearExpectations();
EXPECT_CALL(client(), DiscardStroke(_, _)).Times(0);
// The 2 strokes that were undone have been discarded, and the newly drawn
// stroke takes their place.
@ -1675,6 +1687,11 @@ TEST_F(PdfInkModuleUndoRedoTest, UndoRedoBetweenDraws) {
EXPECT_THAT(StrokeInputPositions(),
ElementsAre(Pair(0, ElementsAreArray(kNext3StrokeMatchersSpan))));
EXPECT_TRUE(VisibleStrokeInputPositions().empty());
VerifyAndClearExpectations();
EXPECT_CALL(client(), DiscardStroke(kPageIndex, InkStrokeId(0)));
EXPECT_CALL(client(), DiscardStroke(kPageIndex, InkStrokeId(1)));
EXPECT_CALL(client(), DiscardStroke(kPageIndex, InkStrokeId(2)));
ApplyStrokeWithMouseAtPoints(
kMouseDownPoint2, base::span_from_ref(kMouseMovePoint2), kMouseUpPoint2);

@ -126,6 +126,7 @@
#if BUILDFLAG(ENABLE_PDF_INK2)
#include "base/memory/raw_ref.h"
#include "pdf/pdf_ink_ids.h"
#include "pdf/pdf_ink_module.h"
#include "pdf/pdf_ink_module_client.h"
#include "third_party/skia/include/core/SkCanvas.h"
@ -363,6 +364,10 @@ class PdfViewWebPlugin::PdfInkModuleClientImpl : public PdfInkModuleClient {
plugin_->engine_->UpdateStrokeActive(page_index, id, active);
}
void DiscardStroke(int page_index, InkStrokeId id) override {
plugin_->engine_->DiscardStroke(page_index, id);
}
void UpdateThumbnail(int page_index) override {
plugin_->GenerateAndSendInkThumbnail(
page_index,

@ -2777,21 +2777,27 @@ TEST_F(PdfViewWebPluginInkTest, UpdateThumbnailWithNoStrokes) {
plugin_->ink_module_client_for_testing()->UpdateThumbnail(/*page_index=*/0);
}
TEST_F(PdfViewWebPluginInkTest, AddAndUpdateStroke) {
TEST_F(PdfViewWebPluginInkTest, AddUpdateDiscardStroke) {
const PdfInkBrush kBrush(PdfInkBrush::Type::kPen, SK_ColorRED, /*size=*/4.0f);
constexpr InkStrokeId kStrokeId(1);
constexpr int kPageIndex = 0;
EXPECT_CALL(*engine_ptr_, ApplyStroke(kPageIndex, kStrokeId, _));
EXPECT_CALL(*engine_ptr_, UpdateStrokeActive(kPageIndex, kStrokeId, false));
const ink::Stroke kStroke(kBrush.ink_brush());
plugin_->ink_module_client_for_testing()->StrokeAdded(kPageIndex, kStrokeId,
kStroke);
EXPECT_CALL(*engine_ptr_, UpdateStrokeActive(kPageIndex, kStrokeId, false));
plugin_->ink_module_client_for_testing()->UpdateStrokeActive(
kPageIndex, kStrokeId,
/*active=*/false);
EXPECT_CALL(*engine_ptr_, DiscardStroke(kPageIndex, kStrokeId));
plugin_->ink_module_client_for_testing()->DiscardStroke(kPageIndex,
kStrokeId);
}
TEST_F(PdfViewWebPluginInkTest, VisiblePageIndexFromPoint) {

@ -4424,6 +4424,22 @@ void PDFiumEngine::UpdateStrokeActive(int page_index,
ink_stroked_pages_needing_regeneration_.insert(page_index);
}
void PDFiumEngine::DiscardStroke(int page_index, InkStrokeId id) {
CHECK(PageIndexInBounds(page_index));
auto it = ink_stroke_objects_map_.find(id);
CHECK(it != ink_stroke_objects_map_.end());
for (FPDF_PAGEOBJECT page_object : it->second) {
bool result =
FPDFPage_RemoveObject(pages_[page_index]->GetPage(), page_object);
CHECK(result);
// After FPDFPage_RemoveObject(), ownership of `page_object` is transferred
// from the page to `this`, so free it.
FPDFPageObj_Destroy(page_object);
}
ink_stroke_objects_map_.erase(it);
}
std::map<InkModeledShapeId, ink::ModeledShape>
PDFiumEngine::LoadV2InkPathsForPage(int page_index) {
CHECK(PageIndexInBounds(page_index));

@ -386,6 +386,11 @@ class PDFiumEngine : public DocumentLoader::Client, public IFSDK_PAUSE {
// another call makes them active again. Virtual to support testing.
virtual void UpdateStrokeActive(int page_index, InkStrokeId id, bool active);
// Removes an existing stroke identified by `id`. The caller must pass the
// same consistent and valid `page_index/`id` pair as was provided to
// `ApplyStroke()`. Virtual to support testing.
virtual void DiscardStroke(int page_index, InkStrokeId id);
// Loads "V2" Ink paths from a page in the PDF identified by `page_index`. The
// `page_index` must be in bounds.
//

@ -2152,7 +2152,7 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeData) {
1);
// Perform equivalent of a "redo", to cause stroke to become active again.
// This causes the streok to be included in saved PDF data again.
// This causes the stroke to be included in saved PDF data again.
engine->UpdateStrokeActive(kPageIndex, kStrokeId2, /*active=*/true);
CheckPdfRendering(page.GetPage(), kPageSizeInPoints, kAppliedStroke2FilePath);
saved_pdf_data = engine->GetSaveData();
@ -2164,6 +2164,83 @@ TEST_P(PDFiumEngineInkDrawTest, StrokeData) {
2);
}
TEST_P(PDFiumEngineInkDrawTest, StrokeDiscardStroke) {
NiceMock<MockTestClient> client;
std::unique_ptr<PDFiumEngine> engine =
InitializeEngine(&client, FILE_PATH_LITERAL("blank.pdf"));
ASSERT_TRUE(engine);
int page_count = FPDF_GetPageCount(engine->doc());
ASSERT_EQ(page_count, 1);
// Original document drawn on has no stroke data.
EXPECT_EQ(GetPdfMarkObjCountForTesting(engine->doc(),
kInkAnnotationIdentifierKeyV2),
0);
std::vector<uint8_t> saved_pdf_data = engine->GetSaveData();
ASSERT_FALSE(saved_pdf_data.empty());
constexpr int kPageIndex = 0;
constexpr gfx::Size kPageSizeInPoints(200, 200);
const base::FilePath kBlankPngFilePath(FILE_PATH_LITERAL("blank.png"));
CheckPdfRendering(saved_pdf_data, kPageIndex, kPageSizeInPoints,
kBlankPngFilePath);
// Draw a stroke.
auto brush = std::make_unique<PdfInkBrush>(PdfInkBrush::Type::kPen,
SK_ColorRED, /*size=*/4.0f);
constexpr auto kInputs0 = std::to_array<PdfInkInputData>({
{{5.0f, 5.0f}, base::Seconds(0.0f)},
{{50.0f, 5.0f}, base::Seconds(0.1f)},
});
std::optional<ink::StrokeInputBatch> batch = CreateInkInputBatch(kInputs0);
ASSERT_TRUE(batch.has_value());
ink::Stroke stroke0(brush->ink_brush(), batch.value());
constexpr InkStrokeId kStrokeId(0);
engine->ApplyStroke(kPageIndex, kStrokeId, stroke0);
PDFiumPage& page = GetPDFiumPageForTest(*engine, kPageIndex);
// Verify the visibility of strokes for in-memory PDF.
const base::FilePath kAppliedStroke1FilePath(
GetInkTestDataFilePath("applied_stroke1.png"));
CheckPdfRendering(page.GetPage(), kPageSizeInPoints, kAppliedStroke1FilePath);
// Perform the equivalent of an "undo", to cause the stroke to be inactive.
engine->UpdateStrokeActive(kPageIndex, kStrokeId, /*active=*/false);
// The document should not have any stroke data.
saved_pdf_data = engine->GetSaveData();
ASSERT_FALSE(saved_pdf_data.empty());
CheckPdfRendering(saved_pdf_data, kPageIndex, kPageSizeInPoints,
kBlankPngFilePath);
EXPECT_EQ(GetPdfMarkObjCountForTesting(engine->doc(),
kInkAnnotationIdentifierKeyV2),
0);
EXPECT_EQ(FPDFPage_CountObjects(page.GetPage()), 1);
// Discard the stroke.
engine->DiscardStroke(kPageIndex, kStrokeId);
EXPECT_EQ(FPDFPage_CountObjects(page.GetPage()), 0);
// Draw a new stroke, reusing the same InkStrokeId. This can occur after an
// undo action.
constexpr auto kInputs1 = std::to_array<PdfInkInputData>({
{{75.0f, 5.0f}, base::Seconds(0.0f)},
{{75.0f, 60.0f}, base::Seconds(0.1f)},
});
batch = CreateInkInputBatch(kInputs1);
ASSERT_TRUE(batch.has_value());
ink::Stroke stroke1(brush->ink_brush(), batch.value());
engine->ApplyStroke(kPageIndex, kStrokeId, stroke1);
// Verify the visibility of strokes for in-memory PDF.
const base::FilePath kAppliedStroke3FilePath(
GetInkTestDataFilePath("applied_stroke3.png"));
CheckPdfRendering(page.GetPage(), kPageSizeInPoints, kAppliedStroke3FilePath);
EXPECT_EQ(FPDFPage_CountObjects(page.GetPage()), 1);
}
TEST_P(PDFiumEngineInkDrawTest, LoadedV2InkPathsAndUpdateShapeActive) {
NiceMock<MockTestClient> client;
std::unique_ptr<PDFiumEngine> engine =

Binary file not shown.

After

(image error) Size: 333 B

@ -107,6 +107,8 @@ class TestPDFiumEngine : public PDFiumEngine {
MOCK_METHOD(void, UpdateStrokeActive, (int, InkStrokeId, bool), (override));
MOCK_METHOD(void, DiscardStroke, (int, InkStrokeId), (override));
MOCK_METHOD((std::map<InkModeledShapeId, ink::ModeledShape>),
LoadV2InkPathsForPage,
(int),