From 1c08048b29e0d0106b47c8493b2ed8c2101668a7 Mon Sep 17 00:00:00 2001 From: Nick Liu Date: Sun, 30 Aug 2026 21:31:26 +0200 Subject: [PATCH] Fix dangling form field page pointers after saving `Document::swapBackingFile()` keeps the old Page objects alive and adopts the newly generated `PagePrivate` contents into them, deleting the temporary new `Page` objects afterwards. The freshly created form fields still pointed at those temporary pages, though, so any later `FormField::page()` call used freed memory. Specifically, toggling a form button after saving crashed in the `EditFormButtonsCommand` constructor (or recorded a garbage page number that crashed `refreshInternalPageReferences` on the next save). This change * makes form fields store the `PagePrivate`, the object the swap transplants into the surviving `Page`, so the fields stay valid without anything having to re-point them (`Annotation` already stores its page the same way), and * adds a test covering the toggle/save/toggle/save sequence. BUG: 477153 BUG: 505130 (cherry picked from commit 7fd85b3361def1860155806dc71c48d237411298) --- autotests/editformstest.cpp | 89 +++++++++++++++++++++++++++++++++++++ core/form.cpp | 3 +- core/form_p.h | 3 +- core/page.cpp | 2 +- 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/autotests/editformstest.cpp b/autotests/editformstest.cpp index f89f83fa6..be5782518 100644 --- a/autotests/editformstest.cpp +++ b/autotests/editformstest.cpp @@ -8,8 +8,10 @@ #include "../settings_core.h" #include "core/document.h" +#include #include #include +#include #include #include @@ -31,6 +33,7 @@ private Q_SLOTS: void testComboEditForm(); void testListSingleEdit(); void testListMultiEdit(); + void testEditAfterSwapBackingFile(); // helper methods void verifyRadioButtonStates(bool state1, bool state2, bool state3); @@ -414,5 +417,91 @@ void EditFormsTest::verifyTextForm(Okular::FormFieldText *form) QVERIFY(m_document->canRedo()); } +static Okular::FormFieldButton *findCheckBoxByName(const QList &fields, const QString &name) +{ + for (Okular::FormField *ff : fields) { + if (ff->type() == Okular::FormField::FormButton && ff->name() == name) { + return static_cast(ff); + } + } + return nullptr; +} + +void EditFormsTest::testEditAfterSwapBackingFile() +{ + // Check a checkbox, save, then uncheck it and save again. + // + // Regression test for: + // * https://bugs.kde.org/show_bug.cgi?id=477153 + // * https://bugs.kde.org/show_bug.cgi?id=505130 + const QString checkBoxName = m_checkBoxForms[0]->name(); + m_document->editFormButtons(0, QList() << m_checkBoxForms[0], QList() << true); + + QVERIFY(m_document->canSwapBackingFile()); + + QTemporaryFile saveFile1(QStringLiteral("%1/editformstestXXXXXX.pdf").arg(QDir::tempPath())); + QVERIFY(saveFile1.open()); + saveFile1.close(); + QString errorText; + QVERIFY(m_document->saveChanges(saveFile1.fileName(), &errorText)); + QVERIFY(errorText.isEmpty()); + QVERIFY(m_document->swapBackingFile(saveFile1.fileName(), QUrl::fromLocalFile(saveFile1.fileName()))); + + // The Page objects survive the swap, so the swapped-in form fields + // must point at them, not at the pages that were just deleted + const Okular::Page *page = m_document->page(0); + const QList newFields = page->formFields(); + QVERIFY(!newFields.isEmpty()); + for (const Okular::FormField *ff : newFields) { + QCOMPARE(ff->page(), page); + } + + // The swap also replaces the form fields, so look the checkbox up + // again and uncheck it. This used to crash in the + // EditFormButtonsCommand constructor + Okular::FormFieldButton *checkBox = findCheckBoxByName(newFields, checkBoxName); + QVERIFY(checkBox); + QVERIFY(checkBox->state()); + m_document->editFormButtons(0, QList() << checkBox, QList() << false); + QVERIFY(!checkBox->state()); + + // Saving again used to crash in refreshInternalPageReferences when + // the undo stack was refreshed with a garbage page number + QTemporaryFile saveFile2(QStringLiteral("%1/editformstestXXXXXX.pdf").arg(QDir::tempPath())); + QVERIFY(saveFile2.open()); + saveFile2.close(); + QVERIFY(m_document->saveChanges(saveFile2.fileName(), &errorText)); + QVERIFY(errorText.isEmpty()); + QVERIFY(m_document->swapBackingFile(saveFile2.fileName(), QUrl::fromLocalFile(saveFile2.fileName()))); + + const Okular::Page *pageAfterSecondSave = m_document->page(0); + const QList fieldsAfterSecondSave = pageAfterSecondSave->formFields(); + QVERIFY(!fieldsAfterSecondSave.isEmpty()); + for (const Okular::FormField *ff : fieldsAfterSecondSave) { + QCOMPARE(ff->page(), pageAfterSecondSave); + } + + // Both saves refreshed the undo stack to point at the newest form + // fields; make sure undo and redo still act on them + checkBox = findCheckBoxByName(fieldsAfterSecondSave, checkBoxName); + QVERIFY(checkBox); + QVERIFY(!checkBox->state()); + + // Undo the uncheck + m_document->undo(); + QVERIFY(checkBox->state()); + QVERIFY(m_document->canUndo()); + + // Undo the original check + m_document->undo(); + QVERIFY(!checkBox->state()); + QVERIFY(!m_document->canUndo()); + QVERIFY(m_document->canRedo()); + + // Redo the check + m_document->redo(); + QVERIFY(checkBox->state()); +} + QTEST_MAIN(EditFormsTest) #include "editformstest.moc" diff --git a/core/form.cpp b/core/form.cpp index 1a77893c2..0ec33f422 100644 --- a/core/form.cpp +++ b/core/form.cpp @@ -6,6 +6,7 @@ #include "form.h" #include "form_p.h" +#include "page_p.h" // qt includes #include @@ -139,7 +140,7 @@ QList FormField::additionalActions() const Page *FormField::page() const { Q_D(const FormField); - return d->m_page; + return d->m_page ? d->m_page->m_page : nullptr; } QString FormField::committedValue() const diff --git a/core/form_p.h b/core/form_p.h index ad36c9c1d..7b95c170a 100644 --- a/core/form_p.h +++ b/core/form_p.h @@ -15,6 +15,7 @@ namespace Okular { class Action; class FormField; +class PagePrivate; class FormFieldPrivate { @@ -35,7 +36,7 @@ public: Action *m_activateAction; QHash m_additionalActions; QHash m_additionalAnnotActions; - Page *m_page = nullptr; + PagePrivate *m_page = nullptr; QString m_committedValue; QString m_committedFormattedValue; diff --git a/core/page.cpp b/core/page.cpp index c4783c2f5..b9ffabfdd 100644 --- a/core/page.cpp +++ b/core/page.cpp @@ -776,7 +776,7 @@ void Page::setFormFields(const QList &fields) d->formfields = fields; for (FormField *ff : std::as_const(d->formfields)) { ff->d_ptr->setDefault(); - ff->d_ptr->m_page = this; + ff->d_ptr->m_page = d; } } -- GitLab