Bug 118199 - Draw/Impress crashes upon pasting a table from Writer (not on Windows)
Summary: Draw/Impress crashes upon pasting a table from Writer (not on Windows)
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Draw (show other bugs)
Version:
(earliest affected)
6.2.0.0.alpha0+
Hardware: All Linux (All)
: highest critical
Assignee: Armin Le Grand
URL:
Whiteboard: target:6.2.0 target:6.1.3
Keywords: bibisected, bisected, haveBacktrace, needUITest, regression
Depends on:
Blocks: AW080-Regressions
  Show dependency treegraph
 
Reported: 2018-06-16 18:41 UTC by Buovjaga
Modified: 2018-10-04 09:05 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
gdb backtrace (48.42 KB, text/plain)
2018-06-17 10:16 UTC, Xisco Faulí
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Buovjaga 2018-06-16 18:41:59 UTC
1. Create a table in Writer
2. Select the whole table, copy
3. Paste it in a new Draw document

Only on Linux, not on Win. Also happens with gtk3 backend. When I tried to get a backtrace, it did not crash. Crashes in safe mode as well.

Arch Linux 64-bit
Version: 6.2.0.0.alpha0+
Build ID: dc9ee533dc707cc10b99d537eaccc3ee5aa555fe
CPU threads: 8; OS: Linux 4.16; UI render: default; VCL: kde4; 
Locale: fi-FI (fi_FI.UTF-8); Calc: group threaded
Built on June 16th 2018
Comment 1 MM 2018-06-17 08:44:46 UTC
Confirmed on ubuntu 16.04 x64 with Version: 6.2.0.0.alpha0+
Build ID: c4c56de1b0e62ec866b519b2b24c5e805f0a86d3
CPU threads: 2; OS: Linux 4.4; UI render: default; VCL: gtk2; 
TinderBox: Linux-rpm_deb-x86_64@70-TDF, Branch:master, Time: 2018-06-09_00:23:35
Locale: en-US (en_US.UTF-8); Calc: threaded

Strangely enough, when recovering the docs, there is a table copied in draw.
Comment 2 raal 2018-06-17 09:45:54 UTC
This seems to have begun at the below commit.
Adding Cc: to Armin Le Grand ; Could you possibly take a look at this one?
Thanks

c91d690754f29061acf6749200df464d2f0c5c8b is the first bad commit
commit c91d690754f29061acf6749200df464d2f0c5c8b
Author: Jenkins Build User <tdf@pollux.tdf>
Date:   Fri Apr 6 23:11:09 2018 +0200

    source sha:6c14c27c75a03e2363f2b363ddf0a6f2f46cfa91

author	Armin Le Grand <Armin.Le.Grand@cib.de (CIB)>	2018-03-01 15:54:32 +0100
committer	Armin Le Grand <Armin.Le.Grand@cib.de>	2018-04-06 22:29:02 +0200
commit	6c14c27c75a03e2363f2b363ddf0a6f2f46cfa91 (patch)
tree	e66f50adb222dbc1490b4df2d3c63541dad999ac
parent	e1b247a843c5eb850fe0079819239d9883412d38 (diff)
SOSAW080: Added first bunch of basic changes to helpers
SOSAW080: Make SdrModel& prerequisite to SdrObjects
Comment 3 Xisco Faulí 2018-06-17 10:16:11 UTC
Created attachment 142814 [details]
gdb backtrace
Comment 4 Julien Nabet 2018-07-02 20:19:20 UTC
Noel: I haven’t tested it yet but think your commit "Pass OutLinerParaObject..." may help here (https://cgit.freedesktop.org/libreoffice/core/commit/?id=50c63e5c2f7962e8893e2d04b0e958209432f4c9)
Comment 5 Xisco Faulí 2018-07-02 23:31:16 UTC
I'm afraid it's still reproducible in

Version: 6.2.0.0.alpha0+
Build ID: 5fce97a58b8f764e35bf98128591c9a89537da05
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group threaded
Comment 6 Armin Le Grand 2018-07-03 09:59:03 UTC
Cannot check - when opening writer and try to create a table, I get a hang in debugger on SwWrongList::GetWrongPos where maList obviously is wrong/bad/not initialized. Updating master (hopefully fixed)
Comment 7 Armin Le Grand 2018-07-03 11:01:28 UTC
SdrText::~SdrText is called in Model cleanup after SdrTableObj::~SdrTableObj, so SdrText::mrObject is already dead, but used in Cell::~Cell by using Cell::dispose by using SetOutlinerParaObject.

Shutdown problem -> each Cell remembers the SdrObject it is based upon. Need a mechanism to safely discover that the SdrObject is already dead. Note: calling Cell::SetOutlinerParaObject in dispose() called from destructor is not good anyways - that's a virtual function.

Problem detected, but how to solve that...?

Compared with libreoffice-6-0: Difference is that mpOutlinerParaObject in SdrText is already destroyed in SdrText::~SdrText() and in SdrText::SetOutlinerParaObject a check

    if( mpOutlinerParaObject != pTextObject )

is used, so the critical access to the already destroyed SdrObject does not happen. A second diff is that mpOutlinerParaObject in master is a std::unique_ptr<OutlinerParaObject> now.

Thiinking about it...
Comment 8 Noel Grandin 2018-07-03 13:38:30 UTC
@Alg, something like:
   https://gerrit.libreoffice.org/#/c/56873/
?
which fixes it for me
Comment 9 Armin Le Grand 2018-07-03 16:26:00 UTC
Used
  ::tools::WeakReference< SdrTextObj > mxObject;
in class SdrText - that works, but seems complicated. Have to secure all mxObject usages then (and not sure that SdrTableObj gets deleted). Looking for something mmore simple.
Comment 10 Armin Le Grand 2018-07-03 16:32:07 UTC
@Noel - yes - something like that :)
But I would prefer to solve the basic problem - that the local SdrTextObj& mrObject in class SdrText is no longer valid. Other accesses to it will/may cause the same crash to reappear anytime...
Comment 11 Noel Grandin 2018-07-03 16:44:32 UTC
@Alg, at destruction time, we only need to care about what goes on inside the destructor, and it is only at destruction time that the mrObject reference becomes invalid, unless I am missing something?
Comment 12 Armin Le Grand 2018-07-03 17:38:53 UTC
@Noel: Cannot answer that :-) Don't know all your thoughts about it ;-)

Problem is that dispose() is executed multiple times on a Cell instance, last from the Undo-Stack cleanup when SdrModel goes down (btw why does the clipboard-model have an undo steck...?). Latest there, the SdrTableObj is *really* dead and it's mem freed. OTOH SdrTableObj cleans up principally nicely, already disposing as needed.
Thus - secure double dispose and do not access SdrTextObj when disposed. No bool member needed, mpProperties is safe for this. Also avoid resetting OutlinerParaObject when call is from edstructor - this is expensive and superfuous. And when at it - make dispose really private and add users as friends (surprise - there are other direct callers I could identify doing this).

What about https://gerrit.libreoffice.org/#/c/56896/ ..?
Comment 13 Commit Notification 2018-07-04 10:16:57 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

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

tdf#118199 avoid double dispose actions

It will be available in 6.2.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 14 Buovjaga 2018-07-04 11:05:05 UTC
Verified. Thanks.

Arch Linux 64-bit
Version: 6.2.0.0.alpha0+
Build ID: ea39c41fdf63191579d25f327db81db14862251c
CPU threads: 8; OS: Linux 4.17; UI render: default; VCL: gtk3; 
Locale: fi-FI (fi_FI.UTF-8); Calc: group threaded
Built on July 4th 2018
Comment 15 Xisco Faulí 2018-10-03 15:56:10 UTC
ahhh, reproduced in

Version: 6.1.3.0.0+
Build ID: 316281cf1d888a4bac9f1e40e92c66405480249c
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group threaded

Cherry-picked to 6.1 -> https://gerrit.libreoffice.org/#/c/61318/1
Comment 16 Commit Notification 2018-10-04 09:05:40 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "libreoffice-6-1":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=7239716e4307a348d818606340e34b4fc6ceb282&h=libreoffice-6-1

tdf#118199 avoid double dispose actions

It will be available in 6.1.3.

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.