Bug 127481 - EDITING: Crash in sdr::table::Cell::GetItemSet() (on double-click in 1x1 table within duplicated slide)
Summary: EDITING: Crash in sdr::table::Cell::GetItemSet() (on double-click in 1x1 tabl...
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
6.1.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard: target:6.5.0 target:6.4.0.1 target:6....
Keywords: bibisected, bisected, haveBacktrace, needUITest, regression
Depends on:
Blocks: AW080-Regressions
  Show dependency treegraph
 
Reported: 2019-09-10 17:05 UTC by Stepas Toliautas
Modified: 2019-12-21 09:08 UTC (History)
5 users (show)

See Also:
Crash report or crash signature: ["sdr::table::Cell::GetItemSet()"]


Attachments
bt Windows (windbg) (50.27 KB, text/plain)
2019-09-11 07:40 UTC, Julien Nabet
Details
bt with debug symbols (8.25 KB, text/plain)
2019-09-17 18:54 UTC, Julien Nabet
Details
Valgrind trace (37.33 KB, text/x-log)
2019-09-17 19:09 UTC, Julien Nabet
Details
Test case run log with custom debug output (3.89 KB, text/plain)
2019-12-15 14:24 UTC, Stepas Toliautas
Details
Stack traces of each cell construction/ disposal during test case (66.74 KB, text/plain)
2019-12-15 14:26 UTC, Stepas Toliautas
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Stepas Toliautas 2019-09-10 17:05:58 UTC
This bug was filed from the crash reporting server and is br-947d5fbe-199f-476e-a3b4-d87a9cb0ae34.
=========================================
Impress consistently crashes upon trying to put cursor in a table that exists in a recently duplicated slide.

Steps to quickly reproduce:
1. Open Impress application.
2. Cancel template dialog. [irrelevant]
3. Create new slide (via right-click) to get an area that allows table creation.
4. Select Table from the menu at area centerpoint.
5. Set 1x1 cell counts, confirm. [unknown if size is relevant]
6. Duplicate slide with table (I do it via right-click menu).
7. Select table in new slide (by single-click), then double-click inside it.
Result: LO crashes.

Remarks:
* Double-clicking within tables in original slide or any existing slides works, only the duplicated slide is affected.
* Crash occurs trying to edit table contents even after saving (Ctrl-S), BUT if file is closed and reopened after saving, bug does not reproduce.
* All versions from 6.1.0 to 6.3.1 are affected.
* Two different computers (built ca. 2008 and 2012) are affected, albeit running almost identical OS setup.
* Safe mode is affected as well.
Comment 1 MM 2019-09-10 19:25:27 UTC
Confirmed on windows 7 x64 with Version: 6.3.1.2 (x64)
Build ID: b79626edf0065ac373bd1df5c28bd630b4424273
CPU threads: 3; OS: Windows 6.1; UI render: default; VCL: win

https://crashreport.libreoffice.org/stats/crash_details/c2de299b-804e-4bef-8bf1-9efed8b6075c
Comment 2 Julien Nabet 2019-09-11 07:40:20 UTC
Created attachment 154096 [details]
bt Windows (windbg)

On Win10 with master sources updated yesterday, I could reproduce this.
Comment 3 Xisco Faulí 2019-09-17 15:09:40 UTC
regression from

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
Comment 4 Julien Nabet 2019-09-17 18:54:12 UTC
Created attachment 154237 [details]
bt with debug symbols

On pc Debian x86-64 with master sources updated today, I could reproduce this.
Comment 5 Julien Nabet 2019-09-17 19:09:35 UTC
Created attachment 154238 [details]
Valgrind trace
Comment 6 Xisco Faulí 2019-09-18 09:14:59 UTC
Just for the record, it only happens with a 1x1 table, quite a corner case

@Thorsten, I thought you might be interested in this crash...
Comment 7 Stepas Toliautas 2019-12-14 15:20:22 UTC
Not to be rude, but since I'm using single-cell tables within Impress slides all the time (as a container for other textual information than statement lists -- usually code listings), it means that I'm stuck with LO 6.0.7 unless this bug is fixed or I re-learn Impress (tables in Impress 6.0 are slow and stupid anyway, but I do not know of any other Impress layout element that would be usable in those cases). So if anyone can point out approximately what needs fixing (not just where in the code it breaks), I might try patching it myself.
Comment 8 Regina Henschel 2019-12-14 16:01:37 UTC
(In reply to Stepas Toliautas from comment #7)
> ... but I do not know of any other Impress layout
> element that would be usable in those cases).

What advantage has a single-cell table compared with a text box shape or with a custom-shape rectangle with text or with a legacy rectangle with text?
Comment 9 Julien Nabet 2019-12-14 16:06:12 UTC
(In reply to Stepas Toliautas from comment #7)
> ...
> So if anyone can point out
> approximately what needs fixing (not just where in the code it breaks), I
> might try patching it myself.

Great if you may propose a fix!
Here's a link where to start:
https://wiki.documentfoundation.org/Development/GetInvolved

The first thing is to retrieve code and to be able to build it.
Second thing is to submit license statement.
Comment 10 Stepas Toliautas 2019-12-14 19:27:51 UTC
(In reply to Regina Henschel from comment #8)

I responded to this comment privately, to keep the discussion off of bug-report.
Comment 11 Thorsten Behrens (CIB) 2019-12-14 19:46:28 UTC
There's a band-aid here: https://gerrit.libreoffice.org/#/c/85148/
Comment 12 Stepas Toliautas 2019-12-15 00:58:21 UTC
(while I'm waiting for an endless build to end...)
Cell also has GetObjectItemSet that does exactly the same thing as GetItemSet (except for not having second const qualifier), but is null-pointer-guarded and emits warning specific to the Cell. As far as I can see, that message is first of two things that differ between SdrText and Cell implementations of these two functions.
The second thing being that Cell gets its "ObjectItemSet" through TextProperties, while SdrText accesses its parent, AttributeProperties. Actually, Cell.mpProperties member is of type CellProperties (child of TextProperties) that inside the Clone function has warning "does not work yet!" and calls CellProperties __with a nullptr as its last argument (which is supposed to be 'this' of the cell-in-construction)__, but still overrides upstream Clone with it. As the crash occurs on newly-duplicated slide (that one reasonably expects to involve some object cloning), I'm tempted to set a break in there and see what happens.
Comment 13 Julien Nabet 2019-12-15 09:07:59 UTC
(In reply to Stepas Toliautas from comment #12)
> (while I'm waiting for an endless build to end...)
Yes it's an Office suite, not a 1k lines program. If you enable debug build, it's even longer.
Comment 14 Stepas Toliautas 2019-12-15 09:56:43 UTC
It's OK, please don't get offended. Build took 4 hours, no problems except for some warnings. Single-file recompiles are actually OK, within 10 seconds.
Please write me an email if you know where remarks to build process developers can be submitted, I have a couple.
Now I'm looking at double- and triple-disposes of at least 5 cells in a file that "has" only 2 tables with 1 cell each. Thorsten's change works because it avoids using code from cell.cxx; simply checking for nullptr in Cell::GetItemSet from that file works too. I'll look into cell.cxx for a bit.
Comment 15 Stepas Toliautas 2019-12-15 14:24:41 UTC
Created attachment 156596 [details]
Test case run log with custom debug output
Comment 16 Stepas Toliautas 2019-12-15 14:26:07 UTC
Created attachment 156597 [details]
Stack traces of each cell construction/ disposal during test case
Comment 17 Stepas Toliautas 2019-12-15 14:41:27 UTC
OK, here's what I know so far:

* After table creation dialog, one table and one cell is created.
* On page duplication (SdDrawDocument::DuplicatePage), 
    - in SdrObject::CloneHelper<sdr::table::SdrTableObj>, via 'MakeNewObject', 
        - TableModel with single row and single cell is created (2 cells now).
    But then,
    - SdrObject::CloneHelper<sdr::table::SdrTableObj> tries to fill newly created object with data from '*this' using 'operator='.
        - TableModel::dispose and Cell::dispose are called along the way, making just-created cell unusable.
        - TableModel::dispose also activates TableRow::dispose, which calls Cell::dispose AGAIN. (This double-dispose is checked against as part of earlier bugfix, tdf#118199.)
    - SdrObject::CloneHelper<sdr::table::SdrTableObj>'s 'operator=' then
        - creates another TableModel with single row and single cell. Since Cell 2 is only 'disposed of', but not deleted, there are now 3 pointers/ references to dynamically-allocated cells.
    - However, reference to the Cell 2 is kept for future use instead of Cell 3, which later causes the crash that was initially reported here.
    - Either checking for mpProperties != nullptr or not using Cell::GetItemSet disables/ avoids the crash, but reference assignment is not corrected.
* Additionally, on table focus (single click) drag copy of the table is temporarily created and then deleted, with the same cell-duplication and double-dispose effects as described above. This process is repeated on each focus-unfocus cycle.
* At the end of the program (I choose not to save file), all 3 initial Cell references are accounted for and deleted, but with multiple dispose calls along the way.

As I feel really not qualified to make decisions on copy/move semantics, object disposal and memory management conventions of LO, I would be grateful if someone more knowledgeable would interject here and try to sort out which parts of this design are really indisputably wrong and which ones should be worked around.

Again, either nullptr check or Thorsten's proposal seem to be enough to stop Impress from crashing. But the ghost-duplicates will remain.
Comment 18 Commit Notification 2019-12-15 20:09:24 UTC
Thorsten Behrens committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/4ddd76a7324c8b035e60a12ac06cf08339f6e366

tdf#127481 don't override SdrText::GetItemSet() for cells

It will be available in 6.5.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.
Comment 19 Commit Notification 2019-12-16 15:06:59 UTC
Thorsten Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

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

tdf#127481 don't override SdrText::GetItemSet() for cells

It will be available in 6.4.0.1.

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.
Comment 20 Xisco Faulí 2019-12-16 17:52:55 UTC
Verified in

Version: 6.5.0.0.alpha0+
Build ID: 3e33a11d8a553a99bd5f23940a65c301924198fb
CPU threads: 4; OS: Linux 4.15; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); UI-Language: en-US
Calc: threaded

@Thosten, thanks for fixing this issue!
Comment 21 Commit Notification 2019-12-17 12:47:56 UTC
Thorsten Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-6-3":

https://git.libreoffice.org/core/commit/65eed7dec08f7ae35906a329e8011a40e194399e

tdf#127481 don't override SdrText::GetItemSet() for cells

It will be available in 6.3.5.

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.
Comment 22 Commit Notification 2019-12-18 16:49:50 UTC
Stepas Toliautas committed a patch related to this issue.
It has been pushed to "master":

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

tdf#127481: reset active cell reference during table copy

It will be available in 6.5.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.
Comment 23 Commit Notification 2019-12-21 09:08:20 UTC
Stepas Toliautas committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

https://git.libreoffice.org/core/commit/3079c028592bc5123e9fd96c5852dc89a87595bd

tdf#127481: reset active cell reference during table copy

It will be available in 6.4.0.2.

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.