Bug 117039 - Print Preview crashes on signed document
Summary: Print Preview crashes on signed document
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: LibreOffice (show other bugs)
Version:
(earliest affected)
5.4.6.2 release
Hardware: All All
: highest critical
Assignee: Katarina Behrens (CIB)
URL:
Whiteboard: target:6.1.0 target:6.0.5 target:5.4....
Keywords: bibisected, bisected, haveBacktrace, regression
Depends on:
Blocks: Print-Preview
  Show dependency treegraph
 
Reported: 2018-04-16 13:52 UTC by Ferry Toth
Modified: 2018-05-07 12:10 UTC (History)
4 users (show)

See Also:
Crash report or crash signature:


Attachments
gdb backtrace (14.15 MB, text/plain)
2018-04-16 16:04 UTC, Xisco Faulí
Details
bt part (19.33 KB, text/plain)
2018-04-17 20:52 UTC, Julien Nabet
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ferry Toth 2018-04-16 13:52:37 UTC
We tested this on Win7 / Win10 / Kubuntu 17.10, with LibO 5.4.6.2 and 6.0.3.2 both with Calc and Writer.

Clicking the Print Preview on a signed (odt / ods) crashes LibO. Removing the signature and then clicking Print Preview no crash.

May I note that as long time LibO users (starting from OO2.4) we a getting a bit worried about changes being made in areas of code that have worked well for a very long period, apparently without sufficient testing? Problems with signing documents started to appear around 5.1 or so.
Comment 1 Xisco Faulí 2018-04-16 15:28:22 UTC
I can reproduce it in

Version: 6.1.0.0.alpha0+
Build ID: b36ea44dcbdb862b0ac6e6cdaf27225b43dc0c7e
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group
Comment 2 Xisco Faulí 2018-04-16 16:04:25 UTC
Created attachment 141397 [details]
gdb backtrace
Comment 3 Xisco Faulí 2018-04-16 18:07:10 UTC
Regression introduced by:

author	Katarina Behrens <Katarina.Behrens@cib.de>	2017-05-31 18:26:07 +0200
committer	Samuel Mehrbrodt <Samuel.Mehrbrodt@cib.de>	2017-06-08 14:44:25 +0200
commit	2e11bbd288ec59d0ff3368cc5c5ef83e2f5133a2 (patch)
tree	9115b59dd5c354a8bf3148a59a01707e61780024
parent	0632208977a204195a4f5b9e727aed511ece075f (diff)
tdf#105566: Account for 'signature used to be ok but isn't anymore'

Bisected with: bibisect-linux64-6.0

Adding Cc: to Katarina Behrens

For the bisection, I used attachment 54849 [details] from bug 44182
Comment 4 Ferry Toth 2018-04-16 21:13:51 UTC
@Xisco, that was lightning fast, thanks!
Comment 5 Katarina Behrens (CIB) 2018-04-17 07:57:36 UTC
> May I note that as long time LibO users (starting from OO2.4) we a getting a
> bit worried about changes being made in areas of code that have worked well
> for a very long period, apparently without sufficient testing? 

Dude, I totally understand that the fact software has bugs is sometimes frustrating AF. 

But saying that something has been implemented "apparently without sufficient testing" and not having much of a clue how much testing is really happening [1] is not going to win you many friends. It's also not going to motivate people to swiftly deal with your bugreports although it might be tempting to believe otherwise.

No amount of testing ever is going to find all bugs and we all have to live with it.

[1] unit tests, API tests, UI tests, continuous integration running those tests 50+ times a day, crashtesting, fuzzing, army of QA people testing dailies, dedicated QA person paid by TDF, peer code review.
Comment 6 Julien Nabet 2018-04-17 20:52:42 UTC
Created attachment 141432 [details]
bt part

On pc Debian x86-64 with master sources updated today, I could reproduce this.
It seems a kind of infinite recursive loop.
I attached bt part showing it between sfx2/source/doc/objserv.cxx:1066 et sfx2/source/doc/objserv.cxx:1062
Comment 7 Ferry Toth 2018-04-17 21:50:06 UTC
@Katerina I know how much testing is going on. But LibO is a big product, big testing is necessary. I don't want to offend any one, I think all bugs happen "apparently without sufficient testing". The question is how can we prevent these things from happening.

What worries me that with each release bug reports explode. There was a time where there was a graph showing the number of open bugs... It's great that features get added and stuff gets improved. And I really appreciate all the work that goes into that. But there are people who actually *use* LibO and rely on existing functions to work. People who I convinced LibO is a better product than MS, who come to me to ask why it crashes when they click on the Print Preview button.

And there seem to be areas (FileOpen dialog, e-mail merge, signing, pdf export, and with recent version even formatting of old documents that break when opening) where bugs appear at the same speed as they get fixed. Which is good and bad at the same time. 

(PS I don't think you want me to be your dude, I'm 51)
Comment 8 Julien Nabet 2018-04-18 21:01:55 UTC Comment hidden (obsolete)
Comment 9 Julien Nabet 2018-04-19 18:08:54 UTC
(In reply to Julien Nabet from comment #8)
> I wonder if a mutex could help but there a lot of types of these in LO and I
> don't know how to choose :-(

It won't help. Indeed, I added this:
+fprintf(stderr, "%p instance\n", this); in SfxObjectShell::GetState_Impl, it's the same thread
Comment 10 Julien Nabet 2018-04-20 05:17:33 UTC Comment hidden (obsolete)
Comment 11 Julien Nabet 2018-04-20 05:23:16 UTC
(In reply to Julien Nabet from comment #10)
> I submitted a patch to review here:
> https://gerrit.libreoffice.org/#/c/53183/

Sorry, the patch is wrong, it resulted to just disable the call to remove and add info bar :-(
Comment 12 Julien Nabet 2018-04-21 06:10:44 UTC
Katarina: any update about this one? If not, what about reverting 2e11bbd288ec59d0ff3368cc5c5ef83e2f5133a2 for the moment?
Comment 13 Commit Notification 2018-04-30 08:40:28 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=43459bac67363f49aadd851e686d4a74b8ddc256

tdf#117039: update infobar instead of removing and re-adding it

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 14 Commit Notification 2018-04-30 16:45:23 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-6-0":

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

tdf#117039: update infobar instead of removing and re-adding it

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 15 Commit Notification 2018-04-30 18:50:57 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=65936059966f92e983ac0d3553ade9ff37884a4e&h=libreoffice-5-4

tdf#117039: update infobar instead of removing and re-adding it

It will be available in 5.4.8.

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 16 Ferry Toth 2018-04-30 19:33:24 UTC
@Katrina Thanks so much. I'll get my colleague to update his windows version. I'll test on Kubuntu.

Are there any code paths (other than print preview) that we should pay particular attention too?
Comment 17 Katarina Behrens (CIB) 2018-04-30 23:44:58 UTC
(In reply to Ferry Toth from comment #16)
> @Katrina Thanks so much. I'll get my colleague to update his windows
> version. I'll test on Kubuntu.
> 
> Are there any code paths (other than print preview) that we should pay
> particular attention too?

This isn't the first time this code path ran into infinite recursion, see this commit:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=d8faf3bf9a82e8f49340b5020ec4ee931cc2f3f4

and its revert b/c failed unit test:
https://cgit.freedesktop.org/libreoffice/core/commit/?id=b805e5705cfaafd8e54283840bec23afc789124a

and a different fix (unit tests green):
https://cgit.freedesktop.org/libreoffice/core/commit/?id=e744e9f4492d3013742fcdb6254cd76528870e9d

So ye this is an example of how humans were able to creatively outsmart unit test that obviously exercises the given functionality :|

Anything that alters the state or the visibility of info bar(s) such as: editing signed document, adding/deleting document signature, invalidating document signature, switching the document to read-only mode and back, going to print preview and back puts this code path to test.
Comment 18 Xisco Faulí 2018-05-03 13:59:44 UTC
Verified in

Version: 6.1.0.0.alpha1+
Build ID: f1579d3d6c5f5f3a651825e035b93bee7a4f43c6
CPU threads: 4; OS: Linux 4.13; UI render: default; VCL: gtk3; 
Locale: ca-ES (ca_ES.UTF-8); Calc: group

@Bubli, thanks for fixing this, should it be closed as RESOLVED FIXED ?
Comment 19 Katarina Behrens (CIB) 2018-05-03 18:38:44 UTC
> @Bubli, thanks for fixing this, should it be closed as RESOLVED FIXED ?

The issue as originally reported is certainly fixed. At least nobody complained so far
Comment 20 Ferry Toth 2018-05-04 10:39:49 UTC
Sorry for not responding earlier. We have a holiday here, so colleagues with Windows not available. I planned to test also on Kubuntu and version 5.4, but AFAIKT those are not yet available as daily build.
But I understand the urgency and tested myself in Win10 32bits in a Virtualbox. Fortunately I am very good in finding bugs.

It appears the bug is partly fixed. But I can still get a crash.

Here is what I tested:
libo-60~2018-05-04_04.45.57_LibreOfficeDev_6.0.5.0.0_Win_x86.msi
Win10 32bits (updates may be 3 months behind)

- open LibO (starts in safe mode)
- take unsigned document
- check print preview: OK
- check read only: OK
- check read only + preview: OK
- sign it: OK
- status bar shows document signed icon: OK
- title bar shows (signed): OK
- info bar appears with signed status: OK

- check print preview: OK
- check read only: OK
- check read only + preview: OK
- dismiss info bar (click x in right corner)
- check print preview: CRASH

Do we need to reopen this? Because I don't see how we can un'Fixed' it.
Comment 21 Xisco Faulí 2018-05-04 11:01:35 UTC
Hi Ferry,
Thank you very much for retesting it, I do confirm the crash when the infobar is removed, nice catch!
I will create a follow-up bug as the issue was introduced in another commit, e744e9f4492d3013742fcdb6254cd76528870e9d
Comment 22 Ferry Toth 2018-05-04 11:54:25 UTC
All I did was follow @Katerina's instructions.

I looks like this info bar is causing griefs heavier than our offenses (to paraphrase Shakespeare). If I could hide it with an option I would.
Comment 23 Commit Notification 2018-05-07 12:10:53 UTC
Katarina Behrens committed a patch related to this issue.
It has been pushed to "libreoffice-5-4-7":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=caf1019dd2b575e6dcfe46d05af5e444393d05ab&h=libreoffice-5-4-7

tdf#117039: update infobar instead of removing and re-adding it

It will be available in 5.4.7.

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.