sw/qa/extras/rtfexport/data/tdf162342.rtf | 7 ++
sw/qa/extras/rtfexport/rtfexport8.cxx | 64 +++++++++++++++++++
sw/source/writerfilter/dmapper/DomainMapper_Impl.cxx | 34 ++++++++--
3 files changed, 101 insertions(+), 4 deletions(-)
New commits:
commit 7877bfc1356ca38d294772e9d88c429b3c613a04
Author: Mike Kaganski <mike.kagan...@collabora.com>
AuthorDate: Mon Jul 28 01:29:18 2025 +0500
Commit: Miklos Vajna <vmik...@collabora.com>
CommitDate: Mon Jul 28 13:26:44 2025 +0200
tdf#162342: workaround out-of-order context stack pop
Turns out, the context stack is populated in one order, but emptied in
another. Commit e63a75e00dcd797b0bd061add3c72dcb7b353007 (tdf#158586
writerfilter: RTF import: fix \page \sect \skbnone, 2024-02-01) added
yet another case, which resulted in m_pTopContext being empty at the
moment when section properties should have been applied (see "needed
for page properties" comment in DomainMapper::sprmWithProps; that code
wasn't reached because of (!rContext) check above).
But that isn't the only case; I tried to assert in non-RTF cases, and
it fired in `make check`, too.
So here is an UGLY HACK. I repair the stack when it's popped out of
order. This pleads for a proper fix, but the effort will be far from
trivial.
Change-Id: I1a5c646188f3da4842d103b8edeb11b801fea830
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/188450
Reviewed-by: Michael Stahl <michael.st...@collabora.com>
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoff...@gmail.com>
diff --git a/sw/qa/extras/rtfexport/data/tdf162342.rtf
b/sw/qa/extras/rtfexport/data/tdf162342.rtf
new file mode 100644
index 000000000000..e04fdc6aeb96
--- /dev/null
+++ b/sw/qa/extras/rtfexport/data/tdf162342.rtf
@@ -0,0 +1,7 @@
+{ tf1
+\margl720 \margr360 \margt720 \margb720 \paperw6120 \paperh7920
+First
+\page
+Second
+\page
+Third}
\ No newline at end of file
diff --git a/sw/qa/extras/rtfexport/rtfexport8.cxx
b/sw/qa/extras/rtfexport/rtfexport8.cxx
index 3337ce0b38d9..5fbfb0128fd4 100644
--- a/sw/qa/extras/rtfexport/rtfexport8.cxx
+++ b/sw/qa/extras/rtfexport/rtfexport8.cxx
@@ -1040,6 +1040,70 @@ CPPUNIT_TEST_FIXTURE(Test, testTdf167569_2)
}
}
+CPPUNIT_TEST_FIXTURE(Test, testTdf162342)
+{
+ // Given an RTF with some page settings, and two page breaks:
+ createSwDoc("tdf162342.rtf");
+
+ {
+ xmlDocUniquePtr pLayout = parseLayoutDump();
+ assertXPath(pLayout, "//page['pass 1']", 3);
+
+ // Without the fix, this failed with
+ // - Expected: 6120
+ // - Actual : 12240
+ // i.e., the page geometry was ignored.
+ assertXPath(pLayout, "//page[1]['pass 1']/infos/bounds", "width",
u"6120");
+ assertXPath(pLayout, "//page[1]['pass 1']/infos/bounds", "height",
u"7937"); // Not 7920?
+ assertXPath(pLayout, "//page[1]['pass 1']/infos/prtBounds", "left",
u"720");
+ assertXPath(pLayout, "//page[1]['pass 1']/infos/prtBounds", "top",
u"720");
+ assertXPath(pLayout, "//page[1]['pass 1']/infos/prtBounds", "width",
u"5040");
+ assertXPath(pLayout, "//page[1]['pass 1']/infos/prtBounds", "height",
u"6497");
+
+ assertXPath(pLayout, "//page[2]['pass 1']/infos/bounds", "width",
u"6120");
+ assertXPath(pLayout, "//page[2]['pass 1']/infos/bounds", "height",
u"7937");
+ assertXPath(pLayout, "//page[2]['pass 1']/infos/prtBounds", "left",
u"720");
+ assertXPath(pLayout, "//page[2]['pass 1']/infos/prtBounds", "top",
u"720");
+ assertXPath(pLayout, "//page[2]['pass 1']/infos/prtBounds", "width",
u"5040");
+ assertXPath(pLayout, "//page[2]['pass 1']/infos/prtBounds", "height",
u"6497");
+
+ assertXPath(pLayout, "//page[3]['pass 1']/infos/bounds", "width",
u"6120");
+ assertXPath(pLayout, "//page[3]['pass 1']/infos/bounds", "height",
u"7937");
+ assertXPath(pLayout, "//page[3]['pass 1']/infos/prtBounds", "left",
u"720");
+ assertXPath(pLayout, "//page[3]['pass 1']/infos/prtBounds", "top",
u"720");
+ assertXPath(pLayout, "//page[3]['pass 1']/infos/prtBounds", "width",
u"5040");
+ assertXPath(pLayout, "//page[3]['pass 1']/infos/prtBounds", "height",
u"6497");
+ }
+
+ saveAndReload(mpFilter);
+
+ {
+ xmlDocUniquePtr pLayout = parseLayoutDump();
+ assertXPath(pLayout, "//page['pass 2']", 3);
+
+ assertXPath(pLayout, "//page[1]['pass 2']/infos/bounds", "width",
u"6120");
+ assertXPath(pLayout, "//page[1]['pass 2']/infos/bounds", "height",
u"7937");
+ assertXPath(pLayout, "//page[1]['pass 2']/infos/prtBounds", "left",
u"720");
+ assertXPath(pLayout, "//page[1]['pass 2']/infos/prtBounds", "top",
u"720");
+ assertXPath(pLayout, "//page[1]['pass 2']/infos/prtBounds", "width",
u"5040");
+ assertXPath(pLayout, "//page[1]['pass 2']/infos/prtBounds", "height",
u"6497");
+
+ assertXPath(pLayout, "//page[2]['pass 2']/infos/bounds", "width",
u"6120");
+ assertXPath(pLayout, "//page[2]['pass 2']/infos/bounds", "height",
u"7937");
+ assertXPath(pLayout, "//page[2]['pass 2']/infos/prtBounds", "left",
u"720");
+ assertXPath(pLayout, "//page[2]['pass 2']/infos/prtBounds", "top",
u"720");
+ assertXPath(pLayout, "//page[2]['pass 2']/infos/prtBounds", "width",
u"5040");
+ assertXPath(pLayout, "//page[2]['pass 2']/infos/prtBounds", "height",
u"6497");
+
+ assertXPath(pLayout, "//page[3]['pass 2']/infos/bounds", "width",
u"6120");
+ assertXPath(pLayout, "//page[3]['pass 2']/infos/bounds", "height",
u"7937");
+ assertXPath(pLayout, "//page[3]['pass 2']/infos/prtBounds", "left",
u"720");
+ assertXPath(pLayout, "//page[3]['pass 2']/infos/prtBounds", "top",
u"720");
+ assertXPath(pLayout, "//page[3]['pass 2']/infos/prtBounds", "width",
u"5040");
+ assertXPath(pLayout, "//page[3]['pass 2']/infos/prtBounds", "height",
u"6497");
+ }
+}
+
} // end of anonymous namespace
CPPUNIT_PLUGIN_IMPLEMENT();
diff --git a/sw/source/writerfilter/dmapper/DomainMapper_Impl.cxx
b/sw/source/writerfilter/dmapper/DomainMapper_Impl.cxx
index fc9e14f6a22b..93bac6b106d2 100644
--- a/sw/source/writerfilter/dmapper/DomainMapper_Impl.cxx
+++ b/sw/source/writerfilter/dmapper/DomainMapper_Impl.cxx
@@ -1368,13 +1368,39 @@ void DomainMapper_Impl::PopProperties(ContextType
eId)
}
m_aPropertyStacks[eId].pop();
+ {
+ // !!!FIXME!!!
+ // We sometimes pop out of order. Just try to comment out this fixing
block, keeping the
+ // assert; and run `make check`. Sigh! In that case, popping from
m_aContextStack would
+ // make it (and m_pTopContext) out-of-sync. This must be an abort; but
let's try to fix
+ // the stack instead, as a temporary (?) measure. I don't replace the
stack with a more
+ // easy-to-manipulate container: I don't optimize for invalid case.
+ // !!!FIXME!!!
+ SAL_WARN_IF(m_aContextStack.top() != eId, "writerfilter.dmapper", "pop
out of order");
+ std::stack<ContextType> tmp;
+ while (m_aContextStack.top() != eId)
+ {
+ tmp.push(m_aContextStack.top());
+ m_aContextStack.pop();
+ }
+ m_aContextStack.pop();
+ while (!tmp.empty())
+ {
+ m_aContextStack.push(tmp.top());
+ tmp.pop();
+ }
+ m_aContextStack.push(eId); // to pop below
+ }
+ assert(m_aContextStack.top() == eId);
m_aContextStack.pop();
- if(!m_aContextStack.empty() &&
!m_aPropertyStacks[m_aContextStack.top()].empty())
-
- m_pTopContext = m_aPropertyStacks[m_aContextStack.top()].top();
+ if (!m_aContextStack.empty() &&
!m_aPropertyStacks[m_aContextStack.top()].empty())
+ {
+ m_pTopContext = m_aPropertyStacks[m_aContextStack.top()].top();
+ }
else
{
- // OSL_ENSURE(eId == CONTEXT_SECTION, "this should happen at a section
context end");
+ SAL_WARN_IF(eId != CONTEXT_SECTION, "writerfilter.dmapper",
+ "this should happen at a section context end");
m_pTopContext.clear();
}
}
