Bug 117475 - CRASH changing the master slide
Summary: CRASH changing the master slide
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Impress (show other bugs)
Version:
(earliest affected)
5.2 all versions
Hardware: All All
: highest critical
Assignee: Caolán McNamara
URL:
Whiteboard: target:6.1.0 target:6.0.5
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks: Master-Slide
  Show dependency treegraph
 
Reported: 2018-05-07 12:20 UTC by Xisco Faulí
Modified: 2018-05-28 11:06 UTC (History)
5 users (show)

See Also:
Crash report or crash signature: ["SdrPage::GetPageNum()"]


Attachments
gdb backtrace (30.11 KB, text/x-log)
2018-05-07 12:26 UTC, Xisco Faulí
Details
gdb + bt trace (7.75 KB, text/plain)
2018-05-07 13:09 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Xisco Faulí 2018-05-07 12:20:10 UTC
Steps to reproduce:
1. Open attachment 141932 [details] from bug 117358
2. From the sidebar change the background to color
3. Go to master slide view
4. Go back to normal view
5. Change the master slide
-> CRASH

Reproduced in

Version: 6.1.0.0.alpha1+
Build ID: 1e2afc9bd3062cfba6b65b45c17a08f298014239
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-05-07 12:26:26 UTC
Created attachment 141952 [details]
gdb backtrace
Comment 2 Julien Nabet 2018-05-07 13:09:23 UTC
Created attachment 141956 [details]
gdb + bt trace

On pc Debian x86-64 with master sources updated yesterday, I could reproduce this.

The file includes bt + logs from console like:
warn:legacy.tools:8762:8762:sd/source/ui/slidesorter/controller/SlsPageSelector.cxx:84: PageSelector::DeselectAllPages: the selected pages counter is not 0
warn:svx:8762:8762:svtools/source/control/valueacc.cxx:653: Calling disposed object. Throwing exception:
warn:svx:8762:8762:svtools/source/control/valueacc.cxx:653: Calling disposed object. Throwing exception:
warn:legacy.osl:8762:8762:sd/source/ui/accessibility/AccessibleSlideSorterView.cxx:759: OSL_ASSERT: nIndex>=0 && static_cast<sal_uInt32>(nIndex)<maPageObjects.size()
warn:sd.core:8762:8762:sd/source/core/PageListWatcher.cxx:95: ImpPageListWatcher::GetSdPage(PageKind::Standard): page number 65535 >= 1
Comment 3 Xisco Faulí 2018-05-07 13:40:11 UTC
Regression introduced by:

author	Yousuf Philips <philipz85@hotmail.com>	2016-06-08 18:03:26 +0400
committer	Yousuf Philips <philipz85@hotmail.com>	2016-06-09 19:34:05 +0000
commit	f0e3a36f6508dcfa0a3c672e15e15f3582e02110 (patch)
tree	eaf0089ab96a464bb47170ce35f37f3605762a52
parent	c8200675c7fd6550c78b20b7c87ebf03047bb6d4 (diff)
tdf#84909 Impress: Rearrange the standard toolbar

Bisected with: bibisect-linux-64-5.3 

Adding Cc: to Yousuf Philips

I was really surprise the bisect repo pointed me to this commit, however, I've reverted it locally, and it no longer crashes without this commit in.
Comment 4 Xisco Faulí 2018-05-07 13:41:00 UTC
@Tamás, I thought you might be interested in this issue...
Comment 5 Julien Nabet 2018-05-07 13:42:55 UTC
(In reply to Xisco Faulí from comment #3)
> ...
> I was really surprise the bisect repo pointed me to this commit, however,
> I've reverted it locally, and it no longer crashes without this commit in.

I wonder if this quoted commit may have revealed a bug instead of being the root cause.
Comment 6 Xisco Faulí 2018-05-07 13:44:31 UTC
Most likely
Comment 7 Maxim Monastirsky 2018-05-10 20:09:15 UTC
(In reply to Xisco Faulí from comment #3)
> I was really surprise the bisect repo pointed me to this commit, however,
> I've reverted it locally, and it no longer crashes without this commit in.
Actually it can be reproduced w/o this commit too, if before step 5 hide and then show again the Standard toolbar.

I tried to bisect this with the 5.2 repo, which resulted with:
last good - f688acfdae00ebdd891737e533d54368810185e1
first bad - 44326f8dfabd81bf8b5a5c741f29ae8c57b4a88e

That's a large range, as I had to skip a lot of commits which were crashing already at step 2. However, if we filter only the sd changes, it's not that much:

https://cgit.freedesktop.org/libreoffice/core/log/sd?qt=range&q=f688acfdae00ebdd891737e533d54368810185e1...44326f8dfabd81bf8b5a5c741f29ae8c57b4a88e
Comment 8 Maxim Monastirsky 2018-05-10 21:53:38 UTC
This is the real first bad commit:

commit db00223e6d3132eac9603e5dabd20cd03f599cb3
Author: Caolán McNamara <caolanm@redhat.com>
Date:   Wed May 18 12:10:26 2016 +0100

    Related: tdf#99523 select only the desired slides
    
    when selecting the same slides in the document as are
    selected in the slide pane, don't forget to unselect
    any slides already selected in the document.
    
    impress is nuts in carrying around two selection
    mechanisms.
    
    Change-Id: I97d744c1c57b68dc312a17a5cd5290e1b6ccf083

Adding Cc: to Caolán McNamara
Comment 9 Caolán McNamara 2018-05-11 10:21:16 UTC
It seems changing the color property triggers a "page (maybe) reordered" event. And reorder means the page is removed and reinserted in the slide sorter. So the slide selected property gets lost. You can see this visually in the slide pane, changing the orientation and the slide is still selected, change the color and the slide becomes unselected.
Comment 10 Commit Notification 2018-05-11 20:04:21 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=96998e0f4c35fb9c7d39e6bb3a31b194874b091c

Resolves: tdf#117475 page properties change triggers page reorder event

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 11 Commit Notification 2018-05-14 13:45:55 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-6-0":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=f2cc4285c51addcb3814222616778c18dd311fe0&h=libreoffice-6-0

Resolves: tdf#117475 page properties change triggers page reorder event

It will be available in 6.0.5.

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 12 Xisco Faulí 2018-05-28 11:06:08 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

@Caolán, thanks for fixing this!!