Bug 113647 - Crash when clicking 2 times on "New Theme..." in Gallery on detached SideBar ( steps in comment 4 ) ( not gtk3)
Summary: Crash when clicking 2 times on "New Theme..." in Gallery on detached SideBar ...
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
5.3.0.0.alpha1+
Hardware: All All
: highest critical
Assignee: Caolán McNamara
URL:
Whiteboard: target:6.0.0 target:5.4.4
Keywords: bibisected, bisected, regression
Depends on:
Blocks:
 
Reported: 2017-11-04 13:56 UTC by Emil Tanev
Modified: 2017-12-01 01:14 UTC (History)
4 users (show)

See Also:
Crash report or crash signature: ["vcl::Window::EnableInput(bool,bool)"]


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Emil Tanev 2017-11-04 13:56:07 UTC
Open Writer
Undock the property side panel 
Open Paragraph > More numbering
Open two windows
Click any of the ident buttons a few times
Back in the More Numbering windows, click OK in the first, then the in second
Issue: crashes

https://www.screencast.com/t/daCQgtViSK

* Multiple windows happens with all custom settings windows
** Happens if you undock the main toolbar as well (e.g. Add new style)

Version: 6.0.0.0.alpha1+ (x64)
Build ID: d30522e46ca884e9bc74af21711d9537e8118859
CPU threads: 8; OS: Windows 10.0; UI render: default; 
TinderBox: Win-x86_64@42, Branch:master, Time: 2017-10-30_00:25:37
Locale: ja-JP (ja_JP); Calc: CL
Comment 1 Emil Tanev 2017-11-04 14:03:01 UTC
Here is another crash scenario - New Theme

https://www.screencast.com/t/LinOmuTEnY
Comment 2 Dieter Praas 2017-11-04 16:43:12 UTC
I tried to reproduce it with the steps from the bug report, but I had no crash.

Version: 6.0.0.0.alpha1 (x64)
Build ID: c1d1f859b268f650143d48f294999cda0fa57350
CPU threads: 4; OS: Windows 10.0; UI render: default; 
Locale: de-DE (de_DE); Calc: group
Comment 3 Mike Kaganski 2017-11-04 18:50:15 UTC
Reproducible with 

Version: 6.0.0.0.alpha1+ (x64)
Build ID: b03fe77699b1ad30a9441bd9b283f25579ac261e
CPU threads: 4; OS: Windows 10.0; UI render: GL; 
Locale: en-US (ru_RU); Calc: CL

and

Version: 5.4.2.2 (x64)
Build ID: 22b09f6418e8c2d508a9eaf86b2399209b0990f4
CPU threads: 4; OS: Windows 6.19; UI render: GL; 
Locale: ru-RU (ru_RU); Calc: CL

Code pointers for an interested developer:

The crash happens because Dialog::ImplStartExecuteModal() tries to access previous active modal dialog's member, but the previous active dialog is already disposed of at that time (and ImplGetSVData()->maWinData.mpLastExecuteDlg still points to that already disposed dialog).

This happens (in case of gallery crash) in GalleryBrowser1::ImplGalleryThemeProperties, where mpThemePropertiesDialog (referencing the previous gallery dialog) is reassigned to a newly created dialog, thus disposing previous one.

The problems:

1. It shouldn't be possible to operate on detached tools, like with attached; so the reassignment wouldn't happen;
2. In that case, if we first checked for existence of the dialog, and possibly just returned the already existing instance, it would work. Or maybe add an assert that would express the invariant (that the mpThemePropertiesDialog must be empty before assignment).
Comment 4 Mike Kaganski 2017-11-05 01:59:07 UTC
The steps to reproduce comment 1 (just in case the screencast gets no longer available):

1. In Writer, undock side panel, and switch to Gallery pane.
2. Click "New Theme..." (pane's topmost button)
3. Click "New Theme..." second time.
Comment 5 Xisco Faulí 2017-11-06 16:23:20 UTC
I think there are two different crashes here.
I can't reproduce the one in the description, but I can the one in comment 1, which is related to bug 105017.
I would suggest to have this ticket for the crash in 'New Theme' and open a new one for the crash in the description
Comment 6 Xisco Faulí 2017-11-06 16:37:01 UTC
Actually, the commits fixing bug 105017, a076a062ceb02bd34a460819db61dcbfbca5c8d7 and cd9d8315141c3070f43e145ed4ee390e837eb73f, fixed the issue in GTK3 but not in the other platforms.

First it started crashing after:

author	Noel Grandin <noel@peralex.com>	2016-09-21 12:48:15 (GMT)
committer	Noel Grandin <noel.grandin@collabora.co.uk>	2016-10-27 06:08:30 (GMT)
commit	eca5ea9f79181d45cd7fbabe2313617d3025818a (patch)
tree	10b5837fe04212349825742b38f5a37be9ce7009
parent	bbd44f8f89839b5abb4ec6c7ea195431de5b2f48 (diff)
make the AbstractDialog stuff extend from VclReferenceBase
Because some stuff wants to multiple-inherit from VclAbstractDialog and
OutputDevice-subclasses, and we'd prefer to keep all the lifetime
management through a single smart pointer class (VclPtr)

The change in msgbox.cxx and window.cxx is to workaround a bug in
VS2013 to do with virtual inheritance and delegating constructors.

and then it changed to two clicks after:

author	Noel Grandin <noel.grandin@collabora.co.uk>	2016-11-10 10:53:02 (GMT)
committer	Noel Grandin <noel.grandin@collabora.co.uk>	2016-11-11 06:55:41 (GMT)
commit	78b4a1fb01af9ad3b3395a22f6e396be914b553e (patch)
tree	846fdaea907a70fdc274a1e76642ed5e06622c0d
parent	071e23fee07b92b8f07800cda3ca7e66afe818ae (diff)
update vclwidget loplugin to find ref-dropping assigment
Look for places where we are accidentally assigning a returned-by-value
VclPtr<T> to a T*, which generally ends up in a use-after-free.

Adding Cc: to Noel Grandin
Comment 7 Mike Kaganski 2017-11-06 17:13:37 UTC
The problem here is not a crash per se. The problem is allowing clicks on toolbars when executing modal dialogs (those dialogs are *modal*, and their execution is expected to block any interactions with other UI, so some assumptions are made of that fact), and so it becomes possible to break the invariants that the modal execution relies upon.
Comment 8 Xisco Faulí 2017-11-06 17:16:07 UTC
Yep, that's how the crash is avoid in GTK3
Comment 9 Mike Kaganski 2017-11-06 17:18:05 UTC
(And the erroneous possibility to interact with floating toolbars pre-exists those commits that made resources management more robust, and made the problem more evident.)
Comment 10 Caolán McNamara 2017-11-09 14:26:43 UTC
https://gerrit.libreoffice.org/#/c/44550/ for the gallery dialog
Comment 11 Caolán McNamara 2017-11-09 16:11:02 UTC
the other case is trickier but the same fundamental issue
Comment 12 Commit Notification 2017-11-09 20:32:26 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=6dc1087b37a9ce5bc9b728d6a23fef69a66bb3d2

tdf#113647 set parent for modal gallery dialog

It will be available in 6.0.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 13 Commit Notification 2017-11-10 10:28:53 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=31b26130d90d4746cbb126fd9b6c1cb3487f644f

tdf#113647 bullet dialog has wrong modal parent

It will be available in 6.0.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 Caolán McNamara 2017-11-10 13:23:47 UTC
that should fix this, though I have another follow up commit
Comment 15 Xisco Faulí 2017-11-10 14:02:06 UTC
Issue with New theme button verified in

Version: 6.0.0.0.alpha1+
Build ID: cf36b5d5f598d9b5528b273858584e2b419b23e9
CPU threads: 4; OS: Linux 4.10; UI render: default; VCL: x11; 
Locale: th-TH (ca_ES.UTF-8); Calc: group
Comment 16 Commit Notification 2017-11-10 15:40:54 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=4dba9820cf44718121a38b3f89eb8caa244d7321

rework tdf#113647 solution to be safe

It will be available in 6.0.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 17 Commit Notification 2017-11-20 08:48:15 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

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

tdf#113647 set parent for modal gallery dialog

It will be available in 5.4.4.

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 18 Kevin Suo 2017-11-20 09:44:13 UTC
I tested on recent master, it is not possible now to open two "More Numbering" dialogues, so the bug is gone on master.

However, the bug still exists on branch libreoffice-5-4 as of commit 19a50a.
Comment 19 Caolán McNamara 2017-11-20 13:02:21 UTC
There were two bugs bundled into this issue, at the moment only the "new theme in gallery" bug is backported to 5-4, the trickier bullets and numbering one isn't backported. I'll backport that today.
Comment 20 Commit Notification 2017-12-01 01:14:50 UTC
Caolán McNamara committed a patch related to this issue.
It has been pushed to "libreoffice-5-4":

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

tdf#113647 bullet dialog has wrong modal parent

It will be available in 5.4.4.

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.