1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
|
From 1c08048b29e0d0106b47c8493b2ed8c2101668a7 Mon Sep 17 00:00:00 2001
From: Nick Liu <deltik@gmx.com>
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 <QDir>
#include <QMimeDatabase>
#include <QMimeType>
+#include <QTemporaryFile>
#include <core/form.h>
#include <core/page.h>
@@ -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<Okular::FormField *> &fields, const QString &name)
+{
+ for (Okular::FormField *ff : fields) {
+ if (ff->type() == Okular::FormField::FormButton && ff->name() == name) {
+ return static_cast<Okular::FormFieldButton *>(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<Okular::FormFieldButton *>() << m_checkBoxForms[0], QList<bool>() << 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<Okular::FormField *> 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<Okular::FormFieldButton *>() << checkBox, QList<bool>() << 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<Okular::FormField *> 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 <QVariant>
@@ -139,7 +140,7 @@ QList<Action *> 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<int, Action *> m_additionalActions;
QHash<int, Action *> 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<FormField *> &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
|
