Bug 133630 - CRASH: Chart: Changing properties in different objects
Summary: CRASH: Chart: Changing properties in different objects
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Chart (show other bugs)
Version:
(earliest affected)
7.0.0.0.alpha1+
Hardware: All All
: highest critical
Assignee: Not Assigned
URL:
Whiteboard: target:7.1.0 target:7.0.4
Keywords: bibisected, bisected, haveBacktrace, regression
: 136898 138331 138819 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-06-03 10:56 UTC by Xisco Faulí
Modified: 2020-12-18 17:53 UTC (History)
14 users (show)

See Also:
Crash report or crash signature:


Attachments
screencast (1.21 MB, video/x-matroska)
2020-06-03 10:56 UTC, Xisco Faulí
Details
bt with debug symbols (gen) (9.61 KB, text/plain)
2020-06-03 11:28 UTC, Julien Nabet
Details
Call Stack (1.01 KB, text/plain)
2020-07-29 11:03 UTC, Regina Henschel
Details
sample document (7.42 KB, application/vnd.oasis.opendocument.spreadsheet)
2020-07-30 14:59 UTC, jun meguro
Details
patch preventing the crash (1.22 KB, patch)
2020-09-10 19:57 UTC, Leyan
Details
updated patch checking object type for the different side panels (4.59 KB, patch)
2020-09-17 20:19 UTC, Leyan
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xisco Faulí 2020-06-03 10:56:33 UTC
Created attachment 161562 [details]
screencast

This is a follow-up of bug 129428 and bug 129587

Steps to reproduce:
1. Open https://bugs.documentfoundation.org/attachment.cgi?id=156720
2. double click chart
3. select Y error bars
4. Format Y error bars
5. Unselect "Same value for both" or change any other property -> bug 129587 crashes here.
6. Click Ok
7. Select any other object ( e.g. the wall )
8. Change any property

-> Crashes

Version: 7.0.0.0.beta1+
Build ID: 2506d8221dd940dfd93d3d7c183430ba6ba3089d
CPU threads: 4; OS: Linux 4.19; UI render: default; VCL: x11
Locale: en-US (en_US.UTF-8); UI: en-US
Calc: threaded
Comment 1 Xisco Faulí 2020-06-03 10:57:36 UTC
Regression introduced by:

https://cgit.freedesktop.org/libreoffice/core/commit/?id=2d01ed9e8be543460e41e009fa992103a7c8d4c0

author	Muhammet Kara <muhammet.kara@collabora.com>	2019-11-25 21:55:31 +0300
committer	Muhammet Kara <muhammet.kara@collabora.com>	2019-11-26 15:39:56 +0100
commit	2d01ed9e8be543460e41e009fa992103a7c8d4c0 (patch)
tree	bd75130e564374647046f03ad4a334c1876d9dd0
parent	9b6b134b3e605236e90aca089867f08d659a6076 (diff)
tdf#94288: Show chart props sidebar on activation

Bisected with: bibisect-linux64-6.5

Adding Cc: to Muhammet Kara
Comment 2 Xisco Faulí 2020-06-03 10:58:32 UTC
@Michael, since you fixed bug 129428 and bug 129587, I thought you might be interested in this issue
Comment 3 Xisco Faulí 2020-06-03 11:02:30 UTC
Also reproducible with these steps:
1. Open attachment 149601 [details] from bug 123722
2. Double click on the chart
3. Click on any number on the X or Y axis
4. Right click on any of the data points - Data Range
5. Cancel the dialog

-> Crash. terminate called after throwing an instance of 'com::sun::star::lang::IndexOutOfBoundsException'
Comment 4 Julien Nabet 2020-06-03 11:28:02 UTC
Created attachment 161566 [details]
bt with debug symbols (gen)

On pc Debian x86-64 with master sources updated today + gen rendering, I got an assertion.
Comment 5 Julien Nabet 2020-06-03 11:29:24 UTC
(gdb) p aCID
$2 = "CID/D=0:CS=0:CT=0:Series=0"
Comment 6 Julien Nabet 2020-06-03 11:33:45 UTC
For the record, I got the assertion too after having reverted locally Muhammet Kara's patch, so perhaps the assertion is unrelated.
Comment 7 Michael Meeks 2020-06-03 12:52:30 UTC
Ah - I am interested of course =) but my fix was in fact a complete accident ;-) presumably it is a side-effect of lurking not actually destroying the sidebar panel widgets - but keeping them around: a vital optimization for mobile.

Is there a stack-trace ?
Comment 8 Julien Nabet 2020-06-03 13:03:37 UTC
(In reply to Michael Meeks from comment #7)
> Ah - I am interested of course =) but my fix was in fact a complete accident
> ;-) presumably it is a side-effect of lurking not actually destroying the
> sidebar panel widgets - but keeping them around: a vital optimization for
> mobile.
> 
> Is there a stack-trace ?

Don't know if it's related but https://bugs.documentfoundation.org/attachment.cgi?id=161566 contains a stacktrace with symbols.
Comment 9 Xisco Faulí 2020-07-10 13:13:20 UTC
*** Bug 134710 has been marked as a duplicate of this bug. ***
Comment 10 Regina Henschel 2020-07-29 11:03:12 UTC
Created attachment 163734 [details]
Call Stack

I get a crash, when I select a line of a data series and then change the line corner style in the sidebar. The attached call stack is from my debug build of LO 7.1

I get the crash too in Version: 7.0.0.2 (x64)
Build ID: c01aa64b6c3d89ebe5fe69c28c7adb24eb85249c
CPU threads: 8; OS: Windows 10.0 Build 18362; UI render: Skia/Vulkan; VCL: win
Locale: de-DE (en_US); UI: en-US
Calc: CL

with the error message window:

LibreOffice 7.0 - Fatal Error
/!\   -1
Comment 11 jun meguro 2020-07-30 14:59:12 UTC
Created attachment 163784 [details]
sample document
Comment 12 Leyan 2020-09-06 22:08:11 UTC
(In reply to Michael Meeks from comment #7)
> Ah - I am interested of course =) but my fix was in fact a complete accident
> ;-) presumably it is a side-effect of lurking not actually destroying the
> sidebar panel widgets - but keeping them around: a vital optimization for
> mobile.
> 
> Is there a stack-trace ?

I got the same kind of crash, with slightly different repro steps. I tried to understand what happened in the code, it seems that the ModifyListener of the different panels sometimes (always?) stay active once created, then they try to update the corresponding data, even if the currently selected object is not of the correct type. This causes issues with axes especially: either an assert in debug mode (complaining about the object type, the one in Comment 8) or an exception otherwise.

I managed to avoid the crash I saw by adding a sanity check inside ChartAxisPanel::updateData(), but what would be the correct way to solve this? Just more checks on object types at each updateData? Or a way to remove listeners without destroying the widgets?
Comment 13 Xisco Faulí 2020-09-10 09:06:05 UTC
*** Bug 136611 has been marked as a duplicate of this bug. ***
Comment 14 Julien Nabet 2020-09-10 09:12:58 UTC
I think we should put max priority here since:
- it's a regression
- it's a crash
- these are not corner cases
- it affect all envs
Comment 15 Michael Meeks 2020-09-10 14:46:29 UTC
Hi Leyan, I'm interested in your sanity-check as a patch. It's curious that the chart sidebars are particularly problematic in this regard - then again - if there is no chart selected they should behave quiescently (I would hope):

OUString getCID(const css::uno::Reference<css::frame::XModel>& xModel)
{
    css::uno::Reference<css::frame::XController> xController(xModel->getCurrentController());
    css::uno::Reference<css::view::XSelectionSupplier> xSelectionSupplier(xController, css::uno::UNO_QUERY);
    if (!xSelectionSupplier.is())
        return OUString();

    uno::Any aAny = xSelectionSupplier->getSelection();
    assert(aAny.hasValue());
    OUString aCID;
    aAny >>= aCID;
#if defined DBG_UTIL && !defined NDEBUG
    ObjectType eType = ObjectIdentifier::getObjectType(aCID);
    assert(eType == OBJECTTYPE_AXIS);
#endif

    return aCID;
}

could happily return an empty string instead of asserting in dbgutil mode - or (perhaps more helpfully) logging a warning instead of asserting.
Comment 16 Leyan 2020-09-10 19:55:58 UTC
Hi Michael,

With the attached patch, I no longer get the crash I was getting initially. I am not sure whether the original bug reporter had the exact same problem, there may be other issues with other types of panels.

Removing the assert you mentioned is the first step, but even without it, LO crashes when a ModifyListener calls ChartAxisPanel::updateData() while the currently selected object is not an axis. In the updateData, the getCID() function returns the CID of the selected object, but then ChartAxisPanel tries to use it through isLabelShown, getAxisForCID, getAxis, getMaximumAxisIndexByDimension and that's where an IndexOutOfBounds exception appears, because there is no dimension information in the CID.
Comment 17 Leyan 2020-09-10 19:57:14 UTC
Created attachment 165380 [details]
patch preventing the crash
Comment 18 Michael Meeks 2020-09-11 13:51:34 UTC
Patch looks plausible. I would SAL_WARN with a sensible domain if the object type is wrong.

Otherwise the 'return 'looks fine when it's not an axis. I would avoid the redundant extra braces - that module has a consistent no-extra-brace-for-one-line statements style.

Lets get it in =) thanks.
Comment 19 Leyan 2020-09-12 21:31:43 UTC
I put an updated version of the patch on gerrit: https://gerrit.libreoffice.org/c/core/+/102544

As it is the first time I go through this process, please tell me if something is wrong.
Comment 20 Regina Henschel 2020-09-12 23:16:09 UTC
Please improve your commit message. A reference to this bug is written as tdf#133630. It will automatically be replaced with a clickable link. Do not use https://.. for this purpose.

The first line should not say "A patch to solve the bug ...". The fact, that the patch will solve this bug is already given by the part "tdf#133630". But please describe what you have done to solve it.

Keep in mind, that the first line should have <50 characters. You should have got a warning when you upload your patch to Gerrit.
Comment 21 Xisco Faulí 2020-09-17 13:31:31 UTC
(In reply to Leyan from comment #19)
> I put an updated version of the patch on gerrit:
> https://gerrit.libreoffice.org/c/core/+/102544
> 
> As it is the first time I go through this process, please tell me if
> something is wrong.

Hi Leyan,
Thank you very much for you patch.
I applied it locally and the steps from the description still make LibreOffice to crash ( same steps as in the screencast ).
Could you please explain which steps your patch fix ?
Comment 22 Leyan 2020-09-17 20:19:14 UTC
Created attachment 165642 [details]
updated patch checking object type for the different side panels

Hi Xisco,

Sorry, I did not read the bug report carefully enough the first time. I came here after trying to solve a crash I got on my own, which causes the same message as the backtrace given in comment 4, so I thought it was the same. What I got was related to axes (for example open a chart with a secondary axis, and alternate between editing axes and chart series properties). So my patch only solved this case.

I reproduced the crash using your steps, it is indeed the same reason, but with error bars and chart series. The same kind of object type check can be used and should avoid the crash you get, please try the updated patch.
Comment 23 NISZ LibreOffice Team 2020-11-04 09:22:35 UTC
(In reply to Leyan from comment #22)
> Created attachment 165642 [details]
> updated patch checking object type for the different side panels
> 

Hi

In order for your patch to be incorporated into LibreOffice, it's also necessary for you to send a license statement to the developer mailing list at: https://lists.freedesktop.org/mailman/listinfo/libreoffice

as documented on the developer wiki:
https://wiki.documentfoundation.org/Development/GetInvolved#License_statement

Could you please do this?
Comment 24 Stefan Zurucker 2020-11-09 10:13:21 UTC
Even a possible patch is on its way, I want to to draw attention to the fact that bug 136898 seems to be another manifestation of this regression and the error logs in that post of further interest.
Comment 25 Leyan 2020-11-09 20:13:09 UTC
(In reply to NISZ LibreOffice Team from comment #23)

> 
> Hi
> 
> In order for your patch to be incorporated into LibreOffice, it's also
> necessary for you to send a license statement to the developer mailing list
> at: https://lists.freedesktop.org/mailman/listinfo/libreoffice
> 
> as documented on the developer wiki:
> https://wiki.documentfoundation.org/Development/GetInvolved#License_statement
> 
> Could you please do this?

Done here : https://lists.freedesktop.org/archives/libreoffice/2020-November/086237.html
Comment 27 Xisco Faulí 2020-11-10 10:31:01 UTC
*** Bug 136898 has been marked as a duplicate of this bug. ***
Comment 28 Commit Notification 2020-11-10 13:15:29 UTC
Ouyang Leyan committed a patch related to this issue.
It has been pushed to "master":

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

tdf#133630 Check object type when updating side panel data

It will be available in 7.1.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 29 Commit Notification 2020-11-10 16:06:21 UTC
Ouyang Leyan committed a patch related to this issue.
It has been pushed to "libreoffice-7-0":

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

tdf#133630 Check object type when updating side panel data

It will be available in 7.0.4.

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 2020-11-10 23:05:36 UTC
Xisco Fauli committed a patch related to this issue.
It has been pushed to "master":

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

tdf#133630: sc: Add UItest

It will be available in 7.1.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 31 Smyan Jaipuriyar 2020-11-19 11:36:04 UTC
essentially similar to what happened with this bug. Mainly graphical issues.
Comment 32 Buovjaga 2020-11-19 11:48:28 UTC
(In reply to Smyan Jaipuriyar from comment #31)
> essentially similar to what happened with this bug. Mainly graphical issues.

This bug is about a crash while bug 86321 is not.
Comment 33 Xisco Faulí 2020-11-19 13:55:41 UTC
I think we can close this issue now.
Comment 34 Xisco Faulí 2020-11-19 13:56:12 UTC
Ouyang Leyan, Andras Timar, thanks for fixing this issue
Comment 35 Xisco Faulí 2020-11-19 13:56:37 UTC
*** Bug 138331 has been marked as a duplicate of this bug. ***
Comment 36 Xisco Faulí 2020-12-18 17:53:28 UTC
*** Bug 138819 has been marked as a duplicate of this bug. ***