Bug 132714 - Crash when icon deleting table-row used for diagram generation.
Summary: Crash when icon deleting table-row used for diagram generation.
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.1.0.3 release
Hardware: All All
: high critical
Assignee: Not Assigned
URL:
Whiteboard: target:7.6.0 target:7.5.2 target:7.4....
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks: Chart Crash
  Show dependency treegraph
 
Reported: 2020-05-05 12:20 UTC by Sion
Modified: 2023-02-27 15:24 UTC (History)
9 users (show)

See Also:
Crash report or crash signature: ["SwTableBox::GetSttIdx()"]


Attachments
bt with debug symbols (14.05 KB, text/plain)
2021-03-27 21:34 UTC, Julien Nabet
Details
Valgrind trace (44.49 KB, application/x-bzip)
2021-03-28 15:35 UTC, Julien Nabet
Details
bt with debug symbols (7.34 KB, text/plain)
2021-11-26 16:43 UTC, Julien Nabet
Details
Valgrind trace (59.13 KB, text/x-log)
2021-11-26 17:00 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sion 2020-05-05 12:20:47 UTC
Description:
First, this is the bug report:
https://crashreport.libreoffice.org/stats/crash_details/f22c0a1f-e712-41b7-8d9a-0414f1101036

(I couldn't find any way to add recreation details to the automatic crash report (consider adding a field for user text before sending the automatic crash report, so I don't have to post several crash reports again)).

Here are recreation details:
#1: Make a table (in my case: X=3 & Y=7).
#2: Fill in some random numbers in the table.
#3: Select the table & go to: "Insert > Diagram".
#4: Generate a diagram from the data in the table.
#4.1: When done & the diagram works as expected.
#5: Go to the table & delete 1 row.
#6: Double click the diagram, so it updates the values. BOOOOM!!!
#7: Crash!

Additional data:
#1.1 In my case I used X-row-1 for axis-titles, set to heading-2
I also left the 2'nd row empty (because I tried to make the diagram not look weird (didn't work)).
#3.1: In my case I used a line diagram with data points.

Crash Cause assumption:
I assume that the diagram isn't updated properly & tries to read the last row that no longer exist & therefore returns some undefined- or null-value that isn't captured & handled as it should.

Hope this is helpful.
Good luck!

Steps to Reproduce:
see description.

Copy from description:

Here are recreation details:
#1: Make a table (in my case: X=3 & Y=7).
#2: Fill in some random numbers in the table.
#3: Select the table & go to: "Insert > Diagram".
#4: Generate a diagram from the data in the table.
#4.1: When done & the diagram works as expected.
#5: Go to the table & delete 1 row.
#6: Double click the diagram, so it updates the values. BOOOOM!!!
#7: Crash!

Additional data:
#1.1 In my case I used X-row-1 for axis-titles, set to heading-2
I also left the 2'nd row empty (because I tried to make the diagram not look weird (didn't work)).
#3.1: In my case I used a line diagram with data points.


Actual Results:
Crash to desktop & recreation of the document with possibly some loss of data.

Expected Results:
A working diagram.


Reproducible: Always


User Profile Reset: No



Additional Info:
LibreOffice version: 6.2.8.2 An older version?

see description.

Crash Cause assumption:
I assume that the diagram isn't updated properly & tries to read the last row that no longer exist & therefore returns some undefined- or null-value that isn't captured & handled as it should.
Comment 1 Telesto 2020-05-05 17:21:50 UTC
1. Open attachment 160394 [details]
2. Select A1 until C2
3. Right click above cell C2 (containing 2)
4. Delete rows
5. Double click the chart -> Crash

Not in
Versie: 4.4.7.2 
Build ID: f3153a8b245191196a4b6b9abd1d0da16eead600
Locale: nl_NL
Comment 2 Aron Budea 2020-05-09 02:59:32 UTC
Already crashes with Telesto's sample in oldest commit of bibisect-linux-64-5.2, but fine in latest commit of bibisect-50max. Needs to be bibisected in Windows.
Comment 3 Buovjaga 2020-05-11 14:40:16 UTC
(In reply to Aron Budea from comment #2)
> Already crashes with Telesto's sample in oldest commit of
> bibisect-linux-64-5.2, but fine in latest commit of bibisect-50max. Needs to
> be bibisected in Windows.

Not sure why on Windows? Did you test with 5.1 repo as well?

I was able to repro the crash exactly once on Windows, with master. Modification to Telesto's step 3 was that I did not click above the cell containing 2, but on it. However, I was then no longer able to repro with the same version or master of Win repos 5.2, 5.3, 5.4, 6.2, 6.3, 6.4, 6.5.

It seems either Telesto needs to bibisect or I need some help with reliability of crashing.

Version: 7.0.0.0.alpha0+ (x64)
Build ID: 00db5933ded1884b2ac453552badae20fa943478
CPU threads: 4; OS: Windows 10.0 Build 18362; UI render: default; VCL: win; 
Locale: fi-FI (fi_FI); UI-Language: en-US
Calc: threaded
Comment 4 Aron Budea 2020-05-12 04:23:07 UTC
(In reply to Buovjaga from comment #3)
> (In reply to Aron Budea from comment #2)
> > Already crashes with Telesto's sample in oldest commit of
> > bibisect-linux-64-5.2, but fine in latest commit of bibisect-50max. Needs to
> > be bibisected in Windows.
> 
> Not sure why on Windows? Did you test with 5.1 repo as well?
There's no 5.1 bibisect repo on Linux, yet.
What's strange though is that it doesn't crash with release versions until later... In Linux it's fine with 5.4.0.3, but crashes with 6.0.0.3. In Windows it's fine with 6.0.0.3, and crashes with 6.1.0.3.

Crash report:
- Win / 7.0 alpha1: https://crashreport.libreoffice.org/stats/crash_details/4de191f8-31c1-4508-9e64-e7e959847a52
- Win / 6.1.0.3: https://crashreport.libreoffice.org/stats/crash_details/0f78ca77-4a6f-4ea5-a711-090fad019d41
- Linux / 6.0.0.3: https://crashreport.libreoffice.org/stats/crash_details/0cb5ffd6-252d-4bf6-bc4c-651fd7790cc6

> I was able to repro the crash exactly once on Windows, with master.
> Modification to Telesto's step 3 was that I did not click above the cell
> containing 2, but on it. However, I was then no longer able to repro with
> the same version or master of Win repos 5.2, 5.3, 5.4, 6.2, 6.3, 6.4, 6.5.
Where you click in the table doesn't matter, you have a 3x2 cell selection ranging from 'a' to '2', the two affected rows will be deleted (in fact it's enough to select a range covering a single cell in each row, eg. 'a' and 'b').

There were a few times when I couldn't repro the crash, but generally I can.
Comment 5 Julien Nabet 2021-03-27 21:34:59 UTC
Created attachment 170788 [details]
bt with debug symbols

On pc Debian x86-64 with master sources updated today, I could reproduce this.
Comment 6 Julien Nabet 2021-03-28 15:35:37 UTC
Created attachment 170795 [details]
Valgrind trace

Here's a Valgrind trace retrieve on pc Debian x86-64 with master sources updated today (without enable-dbgutil).
Comment 7 Matt K 2021-05-18 03:49:39 UTC Comment hidden (obsolete)
Comment 8 Buovjaga 2021-05-18 05:31:15 UTC
(In reply to Matt K from comment #7)
> I don't see the option for "Insert > Diagram".  Is there another way to
> insert it?

Insert > Chart.

"Diagram" probably comes from free-form translation of whatever the UI language of the reporter is.
Comment 9 raal 2021-09-29 06:29:20 UTC
No crash with Version: 7.3.0.0.alpha0+ (x64) / LibreOffice Community
Build ID: 41ccb5c63c94e54b07707eca62bed761c87b0d10
CPU threads: 4; OS: Windows 10.0 Build 17763; UI render: Skia/Vulkan; VCL: win
Locale: cs-CZ (cs_CZ); UI: en-US
Calc: CL
Followed the steps in comment 1.
Please, can you retest it?
Comment 10 Aron Budea 2021-11-06 02:17:48 UTC
I can still repro in LO Version: 7.3.0.0.alpha0+ (ca74611acfef50280a2c1f785448d9a09cca5a0d) / Ubuntu. The commit is ~2 weeks old.
Comment 11 Julien Nabet 2021-11-26 16:43:12 UTC
Created attachment 176528 [details]
bt with debug symbols

On pc Debian x86-64 with master sources updated today + gen rendering, I got an assertion.
Comment 12 Julien Nabet 2021-11-26 17:00:31 UTC
Created attachment 176529 [details]
Valgrind trace

Here's an updated Valgrind trace (retrieved with gen rendering)
Comment 13 Julien Nabet 2021-11-27 12:36:32 UTC
I wanted to declare m_pStartNode in sw/inc/swtable.hxx like this:
std::unique_ptr<SwStartNode> m_pStartNode
instead of:
const SwStartNode * m_pStartNode

but I got stuck since I don't know which element must own the object and which one just wants to have some info from it.
Comment 14 Timur 2022-04-26 12:31:25 UTC
I reproduced steps from Comment 1 in 7.1. 
From 7.2 it is worse, crash already after rows select and delete (I used icon).
 e9ead9066c796b61750ac96729843daa8d848f2e is the first bad commit
    source eea37aa26932d06ed8e93d001862bf45175c4446
    prev 7fd980a607eead2c6cf6557c07a9c25cb5b1a5d4

author	Armin Le Grand (Allotropia) <Armin.Le.Grand@me.com>	2022-01-18
tdf#122995 Trigger Chart refresh directly in UpdateCharts for SW

CC Armin, please see.
Comment 15 Armin Le Grand 2022-05-13 09:43:19 UTC
Made a quick check. Seems to depend on how the row is deleted:
- Clicked in table field, context menu, Delete/Delete_rows -> OK
- Other ways (e.g. toolbar command) -> Crash

Definitely *not all ways* to delete a row lead to that crash.

I would have guessed that all ways to delete trigger the same (and that may be the case), so seems as if some side-effect (selection? current selection?) does cause the trouble (?)

If it crashes I get in SwTableBox::GetSttIdx() a bad ptr in m_pStartNode (in my case not nullptr, but 0x10000000000000 -> crash in call to m_pStartNode->GetIndex(), bad this-ptr).

Unfortunately I have no knowledge about SwTableBox and what m_pStartNode is used for, but seems as if SwStartNode* in m_pStartNode gets invalid but used (deleted?)
Comment 16 Matt K 2022-08-04 01:41:21 UTC
I bibisected the crash 3 times in the win64-7.0 repo and got 3 different results (following the bibisecting guide) and I couldn't determine if the changes were related or not.

A note about the repro: sometimes you have to double-click twice on the chart to see the crash
Comment 17 Noel Grandin 2022-08-11 09:40:04 UTC
So the problem(s) here is that writer does not appear to update charts when a table structure happens (or when a table is deleted).

The code that manages table structure changes (e.g rows added/deleted), will need to be updated to check if any charts depend on that table, and if so, force that chart to change it's pointers to table and cell nodes.
Comment 18 Noel Grandin 2022-08-11 12:52:03 UTC
Specifically,

SwChartDataSequence which has a field
  sw::UnoCursorPointer m_pTableCursor
which points at a
    SwUnoTableCursor
which extends
    SwTableCursor
which has a field
    SwSelBoxes m_SelectedBoxes
which is an array, each element of which has a field
    SwStartNode* m_pStartNode
which are pointing at the cells in the table.

When those cells are deleted, those pointers are now invalid, but we still try and dereference them.
Comment 19 Armin Le Grand 2022-10-20 10:17:17 UTC
I really don't know about what SW is doing there, including all the selection stuff. I can just say that it fails when in SwFEShell::DeleteRow one SwTableBox also being noted in "SwSelBoxes aBoxes;" gets accessed in SwTableBox::GetSttIdx.

This seems to have to do with that in SwFEShell::DeleteRow directly calling over ChartView::updateHard() ChartView::impl_updateView gets triggered. That again does trigger from Chart to SW using UNO API SwTableCursor::NewTableSelection() and SwTableCursor::ActualizeSelection. There it seems as if old and new selectin are somehow matched, where old selection seems to have invalid entries (plausible after a row was deleted)

I have no idea how that selection may be already updated/deleted in SwFEShell::DeleteRow *before* the call to EndAllActionAndCall() that triggers the chart repaint (and thus the callback from Chart to get the SW selection updated):

I tried in SwFEShell::DeleteRow to reset aBoxes (using aBoxes.clear()), but that did not help.

I tried to use TableCursorToCursor(), but false == IsTableMode(), so no m_pTableCursor that needs to be deleted (you can see that indeed the selection is removed when right-click to C2 to open the context menu).

I tried to use ClearMark(), but it does nothing - there really is no selection anymore before the call to EndAllActionAndCall().

I also tried in ChartView::updateHard() to call impl_updateView with true instead of false. That prevents the crash, but does not update the chart (since it means bCheckLockedCtrler and the controller is locked during that update).

So it seems not to be the selection, but *something* is not (yet) updated/in order in SW when Chart calls back to it using UNO API during chart refresh. Maybe the order inside EndAllActionAndCall should be checked...
Comment 20 Armin Le Grand 2022-10-20 12:04:37 UTC
Indeed in SwTableCursor::ActualizeSelection called after EndAllActionAndCall() in SwFEShell::DeleteRow *has* an invalid entry in m_SelectedBoxes which holds the 'old' entries. It is a SwTableBox that has a wrong SwStartNode* that is wrong/invalid after SwFEShell::DeleteRow did it's work.
SwStartNode is a SwNode and seems to be deleted (?). SwTableBox is a SwClient, so might be notified about that but has no SwClientNotify to notice that (?).
The other q is where does the SwTableCursor come from that holds that SwTableBox with the dying SwStartNode* ?
It's from a SwChartDataSequence that has a member m_pTableCursor. Seems as if that change in SW's Table-core does somehow not update SwChartDataSequence instances - there evtl. SwTableBox'es that point to SwStartNode's that get deleted need to be updated/deleted (?).
Comment 21 Armin Le Grand 2022-10-21 13:51:23 UTC
To see the problem, set a BP at sw/source/core/crsr/swcrsr.cxx:2545 which is after

2544:  SwTableBox const*const pPOld = m_SelectedBoxes[ nOld ];

In debugger check for pPOld->m_pStartNode to *see* the malformed pointer.
If I set this to 0x00 (two malformed ptrs come up here) the mechanism works.
-> POC that those ptrs are not correctly set after SwFEShell::DeleteRow
Comment 22 Armin Le Grand 2022-10-21 14:12:54 UTC
Inside SwFEShell::DeleteRow the call

        bRet = GetDoc()->DeleteRowCol( aBoxes );

gets three SwSelBoxes "aBoxes" which is a

    class SwSelBoxes : public o3tl::sorted_vector<SwTableBox*, CompareSwSelBoxes> {};

These get 'cleared' calling SwTableBox::RemoveFromTable() that resets

        m_pStartNode = nullptr; // clear it so this is only run once

there. Later, in EndAllActionAndCall(), these show up inSwTableCursor::ActualizeSelection. Sometimes one of these changed from 0x00 to a bad ptr, sometimes another SwTableBox* on pPOld is *wrong* by pointing to a (evtl deleted?) SwTableBox.

Seems as if SwFEShell::DeleteRow itself does not choose enough SwTableBox(es) to add to "aBoxes" to get 'cleaned up' ...?

It is strange anyways that half of the methods in SwTableBox are prepared to handle nullptr == m_pStartNode and half are not (?) so it *is* somehow expected, but not consequently...
Comment 23 Armin Le Grand 2022-10-21 14:55:49 UTC
I have now for test reverted eea37aa26932d06ed8e93d001862bf45175c4446 locally. It *seems* to work, but the same error happens. Revert leads to the chart not being updated, so the error is 'avoided'. If you revert and then activate the chart (e.g. double-click) the *same* error happens.

For that I claim this is not a regression in that sense. It's an error with the involved state of the SwTableBox incarnations after using SwFEShell::DeleteRow when a chart is connected.

To see that:
- revert eea37aa26932d06ed8e93d001862bf45175c4446
- do as in https://bugs.documentfoundation.org/show_bug.cgi?id=132714#c1
- activate chart
-> crash

My patch eea37aa26932d06ed8e93d001862bf45175c4446 just makes the last step unnecessary by really refreshing the chart (what is wanted). Reverting it will not fix the bug.

I hope to have found out enough about it in comments 19-22 to help, but I do not know enough about writer internals (especially SwTableBox and how it should be used) to get forward.
Comment 24 Armin Le Grand 2022-10-21 15:09:27 UTC
P.S.: Maybe that description in comment 23 can be used to do a new bibisect and find the real reason/cause...?
Comment 25 Commit Notification 2023-02-18 10:11:46 UTC
László Németh committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5b9855acc7fa6d1e4a5f53ff0bc47e1dd4729827

tdf#132714 sw: fix crash at table row deletion associated to a chart

It will be available in 7.6.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 26 László Németh 2023-02-18 10:13:04 UTC
Commit description:

tdf#132714 sw: fix crash at table row deletion associated to a chart

Using the table toolbar, deletion of the row of a text table
resulted crashing in ActualizeSelection(), if the text table
is associated to a chart, and the deleted row was the first
data row of the chart. Fix this by emptying the outdated table
selection cache m_SelectedBoxes in GetCellRangeName().

Note: since commit eea37aa26932d06ed8e93d001862bf45175c4446
"tdf#122995 Trigger Chart refresh directly in UpdateCharts for SW",
the crash occurred immediately, i.e. without double clicking on
the chart after the row deletion.

Note: uitest needed to get the crash, not uiwriter.
Comment 27 László Németh 2023-02-18 10:14:47 UTC
@Sion and all: thanks for the report and your analysis!

Started back-porting...
Comment 28 Commit Notification 2023-02-20 11:50:42 UTC
László Németh committed a patch related to this issue.
It has been pushed to "libreoffice-7-5":

https://git.libreoffice.org/core/commit/8c62a74561649ee06a4949931ae85559fa6ded5d

tdf#132714 sw: fix crash at table row deletion associated to a chart

It will be available in 7.5.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.
Comment 29 Commit Notification 2023-02-22 08:26:57 UTC
László Németh committed a patch related to this issue.
It has been pushed to "libreoffice-7-4":

https://git.libreoffice.org/core/commit/6c81e52f26f8bad39c741083fad6f21126ace53c

tdf#132714 sw: fix crash at table row deletion associated to a chart

It will be available in 7.4.7.

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 30 Commit Notification 2023-02-22 18:31:24 UTC
László Németh committed a patch related to this issue.
It has been pushed to "libreoffice-7-5-1":

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

tdf#132714 sw: fix crash at table row deletion associated to a chart

It will be available in 7.5.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 31 NISZ LibreOffice Team 2023-02-27 08:23:44 UTC
VERIFIED IN:
Version: 7.6.0.0.alpha0+ (X86_64) / LibreOffice Community
Build ID: 5a235634ca5761aa4b330ebf7e3a2083b7db1606
CPU threads: 8; OS: Windows 10.0 Build 19044; UI render: Skia/Vulkan; VCL: win
Locale: hu-HU (hu_HU); UI: hu-HU
Calc: CL threaded
Comment 32 Commit Notification 2023-02-27 09:01:33 UTC
László Németh committed a patch related to this issue.
It has been pushed to "libreoffice-7-4-6":

https://git.libreoffice.org/core/commit/8872ae3f6e86aca7f6ecc156caa3a1a8694deb6d

tdf#132714 sw: fix crash at table row deletion associated to a chart

It will be available in 7.4.6.

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.