Bug 116977 - CRASH: LibreOffice crashes being closed with content in the clipboard
Summary: CRASH: LibreOffice crashes being closed with content in the clipboard
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha0+
Hardware: All All
: highest critical
Assignee: Armin Le Grand
URL:
Whiteboard: target:6.1.0
Keywords: bibisected, bisected, regression
: 116974 117111 117179 117181 117509 (view as bug list)
Depends on:
Blocks: AW080-Regressions
  Show dependency treegraph
 
Reported: 2018-04-12 16:39 UTC by Xisco Faulí
Modified: 2018-05-28 12:02 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Xisco Faulí 2018-04-12 16:39:15 UTC
Steps to reproduce:
1. Open attachment 141151 [details]
2. Select the table clicking in one of its corners
3. Cut the content
4. Close LibreOffice

-> Crash!

Reproduced in


Version: 6.1.0.0.alpha0+
Build ID: 24a57e2b854a1b8b3b8533ac72a6614ee29e374a
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group
Comment 1 Xisco Faulí 2018-04-12 16:42:15 UTC
Regression introduced by:

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-07 00:28:30 +0200
commit	dfefe448c41921f2f1e54d3f69b8b9e89031d055 (patch)
tree	1aace31054b5740e2faffcbc5de66a791be27f7d
parent	eba4d5b2b76cefde90cb3d6638c736f435023a45 (diff)
SOSAW080: Added first bunch of basic changes to helpers

Bisected with: bibisect-linux64-6.1

Adding Cc: to Armin Le Grand
Comment 2 Telesto 2018-04-12 17:23:42 UTC
*** Bug 116974 has been marked as a duplicate of this bug. ***
Comment 3 Xisco Faulí 2018-04-12 17:38:02 UTC
@Telesto, I'm not sure bug 116974 is a dupe of this one.
I can only reproduce it with GTk3...
Comment 4 Telesto 2018-04-12 18:05:09 UTC
(In reply to Xisco Faulí from comment #3)
> @Telesto, I'm not sure bug 116974 is a dupe of this one.
> I can only reproduce it with GTk3...

I get a crash following the steps of comment 0, so..
Version: 6.1.0.0.alpha0+
Build ID: 84e2614a75a615d6c8584b13a69b3368d2a12a3d
CPU threads: 4; OS: Windows 6.3; UI render: default; 
TinderBox: Win-x86@42, Branch:master, Time: 2018-04-12_09:11:38
Locale: nl-NL (nl_NL); Calc: CL
Comment 5 Caolán McNamara 2018-04-17 13:10:26 UTC
Yeah, this isn't gtk3 specific. Its just that gtk3 is the only linux backend that has the same save selection to system clipboard on exit feature as windows/mac
Comment 6 Armin Le Grand 2018-05-03 16:32:06 UTC
This one happens on Windows, checking crash...
Comment 7 Armin Le Grand 2018-05-03 17:52:56 UTC
Looks as if the Clipboard gets flushed (save content), that uses SdrExchangeView::GetMarkedObjModel(). That clones all marked SDrObj's to a new SdrModel. In this case, SvxTableController::GetMarkedObjModel() is used.
There, ::CloneRange creates the clone for SdrTableObj.

Problem: This *can* return nullptr what is *not* handled. But this did not happen before, thus I have to check in a version before my change what happened here.

Problem: SdrTableObj::CloneRange has an exit with nullptr (return) that 'delete's the SdrTableObj locally. It *should* use SdrObject::Free, ~SdrTableObj should be protected, too.

Problem: SdrExchangeView::GetMarkedObjModel() *has* to SdrObject::Clone to new target SdrModel in loop in else-path. Maybe better to check all ::Clone usages (by renaming and replacing maybe).

With most of these fixed I get in SdrModel cleanup a Broadcast for a still existing sdr::properties::CellProperties. That references it's SdrObject and SdrModel in AttributeProperties::Notify which are already destroyed. Theroretically the CellProperties should have been destroyed with the SdrObject already (?!?)

Thus: A mix of wrong usages and cleanup problems which surface by this change - that was one of it's purposes - sigh.
Comment 8 Armin Le Grand 2018-05-03 18:02:57 UTC
Preparing libreoffice-6-0 local build to compare what happens...
Comment 9 Armin Le Grand 2018-05-04 09:35:57 UTC
Renaming SdrObject::Clone -> SdrObject::CloneSdrObject to grab all places...
Comment 10 Armin Le Grand 2018-05-04 12:55:05 UTC
Getting solution for tdf#116879, also renamed SdrPage::Clone -> SdrPage::CloneSdrPage. Started checking ::CloneSdrObject usages - found that there are delicate places, so thinking about changing from optionally providing a target-SdrModel to always have to create one -> same as in constructor (anyways). Would make tjhings safer and future chnages less errror-prone if you have to think about traget-Model every time you clone an SdrObject.
Comment 11 Armin Le Grand 2018-05-04 13:46:09 UTC
Foubnd places where 'delete SdrObject' is/was used instead of SdrObject::Free(...), thinking about adding a debug-only mechanism to assert this...
Comment 12 Armin Le Grand 2018-05-04 14:08:18 UTC
Checking SDrObject hierarchy for destructors - these should all exist and be protected to force usage of SdrObject::Free
Comment 13 Armin Le Grand 2018-05-04 16:14:41 UTC
Changing all SdrObject::~ to protected finds quite some problems - fixing these. There are also std::unique_ptr involved which then need an own ObjectDeleter. There were some already, unified to using a central one from svdobj.hxx in svx (SdrObjectFreeOp).
Comment 14 Armin Le Grand 2018-05-04 17:12:09 UTC
Comparing now:

current version:
SvxTableController::GetMarkedObjModel is called as soon as CTRL-X is pressed. First run creates a pNewTableObj due to having a xTable in impl of rTableObj.
No 2nd run on closing the office when waiting too long.
When going faster, main thread is in CMtaOleClipboard::flushCLipboard() which triggers CmtaOleCLipboard::run() in an own task. Here ::GetMarkedObjModel cannot create a clone of rTableObj since rTableObj.mpImpl.mxTable is empty.

libreoffice-6-0:
Quite the same - 1st call when CTRL-X, all works. Not sure oif waiting long may also skip 2nd run.
2nd run also in 2nd task, but rTableObj.mpImpl.mxTable exists and clone is created without problems.

rTableObj.mpImpl.mxTable is in the not-yet-cloned object, so question is who is deleting it? Is it deleted? Checking if it is trhe same instance...

libreoffice-6-0: not the same instance in the 2nd run
current version: same

Thus the sdr::table::SdrTableObj is probably cloned/copied. Maybe the old version clones the mxTable while the noew one does not do this any linger. Checking this...
Comment 15 Armin Le Grand 2018-05-04 17:15:33 UTC
lo60 has SdrTableObj::Clone()
current has SdrTableObj::CloneSdrObject()

both use CloneHelper, both habe a SdrTableObj::operator= that should be used. This has a

    *mpImpl = *rObj.mpImpl;

at the end - that should copy the rTableObj.mpImpl.mxTable. Lets see...
Comment 16 Armin Le Grand 2018-05-04 17:46:32 UTC
neither SdrTableObj::Clone() nor SdrTableObj::CloneSdrObject() is used in old/new version.

rTableObj of the 2nd run is pNewTableObj of the 1st run -> the 1st clone does not work? Checking this...

The 1st copy of SdrTableObj is good, but it's mxTable gets disposed:
ARGH - cannot copy stack from the debugger. copy/paste is *blocked* system-wide when debugging clipboard stuff ;-(
  SdrTableObjImpl::disposing
  TableDesignFamily::dispose()
  SdStyleSheetPool::dispose()
  SdrModel::~SdrModel()
typed here most important stackframes...
Comment 17 Armin Le Grand 2018-05-04 17:52:26 UTC
Checked:  is *not* happening in libreoffice-6-0: This is the difference here. This may be the case doe to SDrModel not being set at that SDrObject derivation - will check that next...
Comment 18 Armin Le Grand 2018-05-07 08:26:21 UTC
SdrModel m1 gets created when doc is loaded, table t1 also.
m2 when CTRL-X, plus t2 (clone from m1/t1).
Closing LO destroys m1, this
- destroys t1 -> correct
- disposes t2 (impl part) over Style destruct/boradcast TableDesignFamily -> error!
- Flushing clipboard clones m2/t2 to m3/t3 where t3 fails due to t2 having no mxTable anymore.
Comment 19 Armin Le Grand 2018-05-07 10:00:03 UTC
This whole Table-Handling stuff in Draw/Impress is full of exceptional stuff compared with standard-SdrObjects (and always was) - argh! Thiw would need redesign (at least partial) anyways.
For now this is too much, will try to get it working. Missing seems just handling of Style-Stuff (of course Tables have their own Styles and own internal handling of these - argh).
Will try to evtl. avoid 'CloneRange' which is essentially another internal Clone mechanism, better sue the officlai Clone and try to make that work. This will require extending/workinOn the operator==, will see...
First, continue securing SdrModel stuff for SdrPages to get that out of the way...
Comment 20 Armin Le Grand 2018-05-07 11:00:13 UTC
Comitted the secure stuff for ::Clone method changes to gerrit (see https://gerrit.libreoffice.org/#/c/53933/). This got enough to already add stand-alone. Continuning (based on it) with the debugging/fix...
Comment 21 Armin Le Grand 2018-05-07 12:39:00 UTC
Decided to do the big cleanup: Only keep one possibility to clone the Object. This also gets rid of multiple code to copy/clone the XTable data. It gets rid of one special handling of GetMarkedObjModel. It gets rid of the extra-CloneRange stuff, too.
To get that working I had to move and add stuff to SdrTableObjImpl::operator= which will now do the main work (see there). It will now do the right thing depending on SdrModel as target (same or different).

removed SvxTableController::GetMarkedObjModel
removed SdrTableObj::CloneRange
fixed SdrTextObj::operator=
removed SdrTableObjImpl::SetModel
fixed/extended SdrTableObjImpl::operator=

Checked:
- The task itself
- copy/paste table to same doc
- copy/paste to 2nd doc
- load/save
- listen to TableStyle
- startup/shutdown of LO with loaded tables

Looks good, need to do more tests
Comment 22 Armin Le Grand 2018-05-07 12:43:50 UTC
- tested copy/paste to 2nd LO instance (lo-6-0)
- saved there
- end LO, restart, reload -> fine
- copy in LO-6-0, paste in current
 Looks good :-)
Comment 23 Armin Le Grand 2018-05-07 13:02:45 UTC
Lust wondering - when looking at the prev code, a multi-select with a TableObject in Draw/Impress *should* lead to problems -> only the TableObject (if first object) should go to clipboard, thus the others should be missing. It can also be that selecting a TableObject forces selection to a single object - but what happens with 'select all' then...?
Comment 24 Telesto 2018-05-07 15:29:20 UTC
(In reply to Armin Le Grand (CIB) from comment #22)
> - tested copy/paste to 2nd LO instance (lo-6-0)
> - saved there
> - end LO, restart, reload -> fine
> - copy in LO-6-0, paste in current
>  Looks good :-)

A testing suggestion:
1. Insert a table in Impress
2. Copy it/ paste it on the same/next sheet
3. Copy the paste/first table again
Comment 25 Armin Le Grand 2018-05-07 19:20:25 UTC
Re-thought about it: SdrTableObj::CloneRange or similar is used, there is functionality behind it:
- load bugdoc
- select e.g. two of the cells of the table
- CTRL-C
- new slide
- CTRL-V
-> only the selected cells get inserted.

Means: When cloning the TableObject, use the selection ;-)
Better do this where all the clones are creted - in SdrExchangeView::GetMarkedObjModel...
Comment 26 Commit Notification 2018-05-07 23:54:42 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=91b0d2122bdee361bf5412a42d48ff051159cbf2

tdf#116977 secured ::Clone methods

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 27 Armin Le Grand 2018-05-08 09:31:07 UTC
Wiull now use a more safe approach: Clone full SdrObject, then crop to selection. I already have a working operator== for SdrTableObject, so add functionality to reduce to evtl. selection after creating full clone.
Adapted SdrExchangeView::GetMarkedObjModel to use new method SvxTableController::GetMarkedSdrObjClone if it is a sdr::table::SdrTableObj. This will check the selection, vetl. create a full clone at call new method SdrTableObj::CropTableModelToSelection at it. This internally forwards to SdrTableObjImpl::CropTableModelToSelection which does the work, including all copy/reference/modify/listener stuff...
Comment 28 Armin Le Grand 2018-05-08 12:05:49 UTC
First thought was to create the new TableModel, copy data to it and then exchange mxTable and dispose old one. This does *not* work, even when all stuff looks nicely referenced and safe *because* Cell::create gets handed over the current SdrTableObj, hands it to ::Cell and there the local mxTable is initialized using rTableObj.getTable() (!). Due to This, the new created Cells in a new created TableModel based on given mpTableObj *will be disposed* when the old mxTable gets disposed - ARGH!
To avoid, change strategy: Remember old TableModel, reset mxTable immediately - this is the SdrTableObjImpl of the current SdrTableObj anyways. Luckily, this works as intended...
Comment 29 Armin Le Grand 2018-05-08 12:13:30 UTC
Actual fix - including partial clones - is now on gerrit (https://gerrit.libreoffice.org/#/c/53975/)
Comment 30 Armin Le Grand 2018-05-08 12:16:18 UTC
@Telesto: Thanks, the problem *should* occurr when multi-selecting a SdrTableObj with just some Cells selected and other objects. This solves due to being not possible in the UI, thus just was a possibility from having seen the code -> done, cannot happen. Anyways, with the fix, it would even work ;-)
Comment 31 Commit Notification 2018-05-09 15:41:48 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=015dd48ce60e64de517cb56a5ced89dd4feb4198

tdf#116977 Correctly handle copy-construct of SdrTableObj

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 32 Telesto 2018-05-10 07:46:39 UTC
*** Bug 117181 has been marked as a duplicate of this bug. ***
Comment 33 Telesto 2018-05-10 07:46:44 UTC
*** Bug 117509 has been marked as a duplicate of this bug. ***
Comment 34 Telesto 2018-05-10 07:48:08 UTC
*** Bug 117111 has been marked as a duplicate of this bug. ***
Comment 35 Buovjaga 2018-05-10 14:24:58 UTC
*** Bug 117179 has been marked as a duplicate of this bug. ***
Comment 36 Michael Meeks 2018-05-16 14:13:03 UTC
Is this now fixed ? if so - would be good to get it closed; Armin ? =)
Comment 37 Armin Le Grand 2018-05-18 09:21:24 UTC
@michael.meeks@collabora.com: Yes, fixed. I fixed it, but AFAIK I am not the one who should set it to 'fixed'...?
Comment 38 Armin Le Grand 2018-05-18 11:35:37 UTC
Checked in current version
Comment 39 Xisco Faulí 2018-05-28 12:00:41 UTC
(In reply to Armin Le Grand (CIB) from comment #37)
> @michael.meeks@collabora.com: Yes, fixed. I fixed it, but AFAIK I am not the
> one who should set it to 'fixed'...?

Hi Armin, Yes, you're the one who should set it to fix once you think your work is done. Then, the QA team should verify it's fixed!
Comment 40 Xisco Faulí 2018-05-28 12:02:25 UTC
Verified in

Version: 6.1.0.0.beta1+
Build ID: da49f4aeb8d5e9a7d2cba8855d911e7cc1d2f1e2
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group threaded