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
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
*** Bug 116974 has been marked as a duplicate of this bug. ***
@Telesto, I'm not sure bug 116974 is a dupe of this one. I can only reproduce it with GTk3...
(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
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
This one happens on Windows, checking crash...
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.
Preparing libreoffice-6-0 local build to compare what happens...
Renaming SdrObject::Clone -> SdrObject::CloneSdrObject to grab all places...
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.
Foubnd places where 'delete SdrObject' is/was used instead of SdrObject::Free(...), thinking about adding a debug-only mechanism to assert this...
Checking SDrObject hierarchy for destructors - these should all exist and be protected to force usage of SdrObject::Free
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).
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...
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...
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...
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...
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.
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...
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...
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
- 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 :-)
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...?
(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
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...
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.
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...
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...
Actual fix - including partial clones - is now on gerrit (https://gerrit.libreoffice.org/#/c/53975/)
@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 ;-)
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.
*** Bug 117181 has been marked as a duplicate of this bug. ***
*** Bug 117509 has been marked as a duplicate of this bug. ***
*** Bug 117111 has been marked as a duplicate of this bug. ***
*** Bug 117179 has been marked as a duplicate of this bug. ***
Is this now fixed ? if so - would be good to get it closed; Armin ? =)
@michael.meeks@collabora.com: Yes, fixed. I fixed it, but AFAIK I am not the one who should set it to 'fixed'...?
Checked in current version
(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!
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