Bug 108124 - CRASH: bad dynamic_cast! after redo operation
Summary: CRASH: bad dynamic_cast! after redo operation
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
Inherited From OOo
Hardware: All All
: highest critical
Assignee: Fyodor
URL:
Whiteboard: target:6.0.0 target:5.4.4 target:6.1....
Keywords: haveBacktrace
Depends on:
Blocks: Undo-Redo
  Show dependency treegraph
 
Reported: 2017-05-26 13:38 UTC by Telesto
Modified: 2021-01-26 12:27 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
backtrace (23.37 KB, text/plain)
2017-05-26 14:19 UTC, Xisco Faulí
Details
WinDBG Assertion failed in Master 6.0+ (10.20 KB, text/plain)
2017-07-11 09:59 UTC, Timur
Details
SwNodes dumpasxml (3.29 KB, text/xml)
2017-10-03 06:23 UTC, Fyodor
Details
SwUndoManager::m_xNodes dumped as xml before assertion (3.04 KB, text/xml)
2017-10-03 06:34 UTC, Fyodor
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Telesto 2017-05-26 13:38:10 UTC
Description:
Under specific conditions a crash occurs

Steps to Reproduce:
1. Open attachment 85053 [details] (bug 68839)
2. CRTL+A
3. CTRL+V (without deselecting first)
4. CTRL+Z
5. CTRL+Y - CRASH

Actual Results:  
Crash

Expected Results:
No Crash


Reproducible: Always

User Profile Reset: No

Additional Info:
Found in
Version: 5.5.0.0.alpha0+
Build ID: d57e6cd9dcc96112994ca2b14ac45896e86b26e5
CPU threads: 4; OS: Windows 6.19; UI render: GL; 
TinderBox: Win-x86@42, Branch:master, Time: 2017-05-18_22:43:07
Locale: nl-NL (nl_NL); Calc: CL

and in
Versie: 4.4.6.3 
Build ID: e8938fd3328e95dcf59dd64e7facd2c7d67c704d
Locale: nl_NL


User-Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
Comment 1 Xisco Faulí 2017-05-26 14:11:02 UTC
Confirmed in

Version: 5.5.0.0.alpha0+
Build ID: 7662b11cad6105d56fb9acc9c431c89d3b68dc89
CPU threads: 1; OS: Windows 6.1; UI render: default; 
TinderBox: Win-x86@39, Branch:master, Time: 2017-05-20_10:09:09
Locale: es-ES (es_ES); Calc: group

and

Version: 5.4.0.0.alpha1+
Build ID: 74d2e606fd3605fe0a585f596eaa215ae4e20d18
CPU Threads: 4; OS Version: Linux 4.8; UI Render: default; VCL: gtk3; 
Locale: en-US (ca_ES.UTF-8); Calc: group
Comment 2 Xisco Faulí 2017-05-26 14:13:14 UTC
Also reproduced in

- Version: 4.3.0.0.alpha1+
Build ID: c15927f20d4727c3b8de68497b6949e72f9e6e9e

- Version 4.1.0.0.alpha0+ (Build ID: efca6f15609322f62a35619619a6d5fe5c9bd5a)

and

LibreOffice 3.3.0 
OOO330m19 (Build:6)
tag libreoffice-3.3.0.4
Comment 3 Xisco Faulí 2017-05-26 14:19:56 UTC
Created attachment 133620 [details]
backtrace
Comment 4 Julien Nabet 2017-05-27 19:16:33 UTC
On pc Debian x86-64 with master sources updated 3 days ago, I could reproduce this.
This assert is triggered because it's a SwGrfNode
$4 = (SwNode &) @0x555558385210: {<BigPtrEntry> = {_vptr.BigPtrEntry = 0x7fffca649208 <vtable for SwGrfNode+208>, m_pBlock = 0x555557861a90, m_nOffset = 18}, 
  m_nNodeType = SwNodeType::Grf, m_nAFormatNumLvl = 0 '\000', m_bSetNumLSpace = false, m_bIgnoreDontExpand = false, static s_nSerial = 89, m_nSerial = 80, 
  m_pAnchoredFlys = std::unique_ptr<std::__debug::vector<SwFrameFormat*, std::allocator<SwFrameFormat*> >> containing 0x0, m_pStartOfSection = 0x55555bebad60}

Here's the test which fails:
   1507     // anchor only to paragraphs, or start nodes in case of RndStdIds::FLY_AT_FLY
   1508     // also allow table node, this is used when a table is selected and is converted to a frame by the UI
   1509     assert(!pPos
   1510             || ((RndStdIds::FLY_AT_FLY == nAnchorId) &&
   1511                     dynamic_cast<SwStartNode*>(&pPos->nNode.GetNode()))
   1512             || (RndStdIds::FLY_AT_PARA == nAnchorId && dynamic_cast<SwTableNode*>(&pPos->nNode.GetNode()))
   1513             || dynamic_cast<SwTextNode*>(&pPos->nNode.GetNode()));
(see http://opengrok.libreoffice.org/xref/core/sw/source/core/layout/atrfrm.cxx#1505)

Michael: noticing https://cgit.freedesktop.org/libreoffice/core/commit/?id=0ed73a0817ad0ff0107cb297208252c0afe3b4a9, thought you might have some idea here.
Comment 5 Timur 2017-07-11 09:46:37 UTC Comment hidden (obsolete)
Comment 6 Timur 2017-07-11 09:59:57 UTC
Created attachment 134585 [details]
WinDBG Assertion failed in Master 6.0+

I don't repro this, don't know why. Tested in Windows 7 64-bit.
1. Open attachment 85053 [details] 
2. CRTL+A  - SELECT ALL
3. CTRL+V (without deselecting first) - PASTE FROM CLIPBOARD OK IF NOT EMPTY; IF EMPTY, Assertion failed! IN master~2017-06-28_00.51.05_LibreOfficeDev_6.0.0.0.alpha0_Win_x86 - debug attached
4. CTRL+Z - UNDO
5. CTRL+Y - REDO, NO CRASH WITH 5.3.3.2 OR 5.4.0.b2
Comment 7 Timur 2017-07-11 10:16:39 UTC
Aforementioned assertion not reproduced in newer libo-master~2017-07-11_04.22.25_LibreOfficeDev_6.0.0.0.alpha0_Win_x86.
Comment 8 Fyodor 2017-09-28 03:03:04 UTC
This is easily reproducible with attached document and similar documents. I use dev build, from master brunch.
To reproduce with attached document:
1. Open document
2. Ctrl+A (sel All)
3. Ctrl+C (copy all, or you'll have nothing to insert on next step)
4. Ctrl+V
5. Ctrl+Z
6. Ctrl+Y

Or this can be reproduced on any new document.
1. Create new document
2. Insert -> Image (I used small images). By default it should be anchored to paragraph
3. Move image away from it default position
4. Insert another image (the same or another one)
5. Move second image away from it default position
6. Insert second paragraph (press Enter)

Now assertion is reproduced using procedure at the start.

I suspect that this is issue with Undo/Redo mechanism.
Comment 9 Xisco Faulí 2017-09-28 07:22:52 UTC
@Fyodor, do you plan to work on this?
Comment 10 Fyodor 2017-09-28 07:28:26 UTC
Yes, sure
Comment 11 Fyodor 2017-09-29 08:02:52 UTC
Assertion is failed in SwFormatAnchor::SetAnchor on line 
RndStdIds::FLY_AT_PARA == m_eAnchorId && dynamic_cast<SwTableNode*>(&pPos->nNode.GetNode()))

This happens due to bad dynamic_cast.

SwFormatAnchor::SetAnchor called several times and everytime pPos->nNode.GetNode() returns object of type "class SwTextNode". On assertion it returns object of "class SwGrfNode" (Got using typeid(pPos->nNode.GetNode()).name())

As this assertion happens after copy/paste actions I suspect that issue with copy/paste insterting some incorrect obeject in SwNodes array in SwDoc::m_pUndoManager.

Next week I'll try to investigate copy/paste process
Comment 12 Fyodor 2017-10-03 06:23:54 UTC
Created attachment 136718 [details]
SwNodes dumpasxml
Comment 13 Fyodor 2017-10-03 06:33:27 UTC
It seems that SwUndoManager::m_xUndoNodes is not used in Undo/Redo operations.
SwNodes::dumpAsXml shows empty document (see attachment SampleDoc.txt).

It contains SwDoc::m_pNodes dumped as xml and SwUndoManager::m_xUndoNodes also dumpoed as xml. Document itself consists of several paragraph and one image. Undo/Redo works fine, but SwUndoManager::m_xUndoNodes is empty...

But before assertion SwUndoManager::m_xUndoNodes contains some information and in my opinion it is malformed... (attachment UndoMgrNodesAfterUndo.txt)

I'm going to investigate SwUndo actions in SfxUndoAction::m_xData, as it seems that SwUndo actions really control Unod/Redo
Comment 14 Fyodor 2017-10-03 06:34:35 UTC
Created attachment 136719 [details]
SwUndoManager::m_xNodes dumped as xml before assertion
Comment 15 Fyodor 2017-10-10 04:32:10 UTC
During work on this issue I've noticed another thing.
When doing actions to reproduce this issue, you should

1. Open document
2. CTRL+A (select all)
3. CTRL+C
4. CTRL+V 

Here all document content should be inserted in place all previous document content, as on step 2 we've selected all. But in document with images we'll have four images (two in original document and two added). But if repeat steps above (again select all, copy and paste), the document will contain the same four images as before...

Images are added if before selecting and coping them, change their position (move some image to other place)...

When doing the same with text, all works correctly, text is replaced...
Comment 16 Fyodor 2017-10-20 05:16:50 UTC
For now, I've found following.

In SwUndoFlyBase::InsFly method after insertion saved SwFrameFormats to SpzFrameFormats (line 74 or so), SwDoc becomes malformed and SwDoc::dumpasxml crushes. (I use SwDoc::dumpasxml to get SwDoc contents).

Just before 

rFlyFormats.push_back( pFrameFormat );

SwDoc still OK and dumped successfully.

I'm going to understand if saved SwFrameFormats from Undo action are incorrect...
Comment 17 Fyodor 2017-10-24 02:32:34 UTC
I think I finally did it!

In original document we have 2 images (flys).
Then we do:
1. Open document
2. Ctrl + A (select all)
3. Ctrl + C (copy all to buffer)
4. Ctrl + V (paste all)
On this step we have document with four images. (I think this is incorrect, as we selected all and on insertion should overwrite all, but this is doesn't matter for the bug in question).
5. Ctrl + U (Undo last action - insertion)
During Undo LO deletes two flys out of four. This is done by following code
in file core\sw\source\core\undo\untblk.cxx, in method SwUndoInserts::UndoImpl, about line 190

    if (m_FlyUndos.size())
    {
        sal_uLong nTmp = rPam.GetPoint()->nNode.GetIndex();
        for (size_t n = m_FlyUndos.size(); 0 < n; --n)
        {
            m_FlyUndos[ n-1 ]->UndoImpl(rContext);
        }
        nNdDiff += nTmp - rPam.GetPoint()->nNode.GetIndex();
    }

The deletion done in reverse order. Firstly LO deletes last fly, than previous one.

6. Ctrl + Y (Redo last action - insert flys back).

Flys are inserted in method SwUndoInserts::RedoImpl, about line 288 by code

    for (size_t n = m_FlyUndos.size(); 0 < n; --n)
    {
        m_FlyUndos[ n-1 ]->RedoImpl(rContext);
    }

And also done in reverse order! As a result last fly inserted in incorrect position between sections, and LO crushed.

I changed the code above to insert flys in direct order and it works correctly.

    for (size_t n = 0; m_FlyUndos.size() > n; ++n)
    {
        m_FlyUndos[ n ]->RedoImpl(rContext);
    }

Now I'll check everything and submit patch.
Comment 18 Xisco Faulí 2017-10-24 06:54:18 UTC
Hello Fyodor,
Thanks for the effort you're putting on this issue.
Yes please, create a patch an submit it to gerrit so devs can review it ->
https://wiki.documentfoundation.org/Development/gerrit/SubmitPatch
Regards
Comment 19 Fyodor 2017-10-26 01:15:04 UTC
Submitted patch https://gerrit.libreoffice.org/#/c/43862/
Comment 20 Commit Notification 2017-11-03 13:33:20 UTC
Fyodor Yemelyanenko committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c263c25052ca7812ec8789b8825bca11da845054

tdf#108124 fix: crash during redo, when document contains images

It will be available in 6.0.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 21 Commit Notification 2017-11-03 20:39:33 UTC
Fyodor Yemelyanenko committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=39285d3621674097c415808084178faff6691664&h=libreoffice-5-4

tdf#108124 fix: crash during redo, when document contains images

It will be available in 5.4.4.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 22 Xisco Faulí 2017-11-05 22:43:50 UTC
Fix verified in

Version: 6.0.0.0.alpha1+
Build ID: 6070dec9ca9a15587a2aece81f9ae1ab5ac0f8c4
CPU threads: 4; OS: Linux 4.10; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group

Fyodor Yemelyanenko, close this issue as RESOLVED FIXED if your work is done here. Thanks for fixing it!!
Comment 23 Commit Notification 2018-01-22 19:47:08 UTC
Zdeněk Crhonek committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=5df0c80b8dc1106a8c28967226526b05b047bc09

uitest for tdf#108124

It will be available in 6.1.0.

The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
http://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 24 Commit Notification 2021-01-26 12:27:41 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/e0df00a1c3ec9340fca917dcad2a86d527e7d1dd

tdf#108124: sw: Move UItest to CppUnitTest

It will be available in 7.2.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.