Issue 120020 - Application crash when undo "Ctrl+Delete" under "Outline" view in SD.
Application crash when undo "Ctrl+Delete" under "Outline" view in SD.
Status: VERIFIED FIXED
Product: Impress
Classification: Application
Component: editing
3.4.0
All All
: P3 normal (vote)
: 4.0.0
Assigned To: Armin Le Grand
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-18 07:10 UTC by Tang Meng
Modified: 2013-07-11 13:36 UTC (History)
7 users (show)

See Also:
Issue Type: DEFECT
Latest Confirmation on: ---
Developer Difficulty: ---
jsc: 4.0.0_release_blocker+


Attachments
Do not delete linked undo actions. (534 bytes, patch)
2013-06-18 14:56 UTC, Andre
awf.aoo: review?
Details | Diff

Note You need to log in before you can comment on or make changes to this issue.
Description Tang Meng 2012-06-18 07:10:19 UTC
1. Launch Aoo then new a SD document.
2. Create 2 blank pages.
3. Switch to "Outline" view(Keep focus in Page2)
4. Press Ctrl+Delete.
5. Press Ctrl+Z,application crash.
Comment 1 Tang Meng 2012-06-18 07:10:48 UTC
I am looking at this bug.
Comment 2 Yan Ji 2012-06-18 14:09:41 UTC
Change project to presentation
Comment 3 Yan Ji 2012-06-18 14:19:28 UTC
Issue can be reproduced, change status to "Confirmed"
Comment 4 Andre 2013-06-17 08:53:29 UTC
I can reproduce this crash on a 4.0 developer build (Windows7).
After step 5 I have to insert some text to trigger the crash.
Comment 5 Andre 2013-06-18 14:56:57 UTC
Created attachment 80882 [details]
Do not delete linked undo actions.

The crash is triggered in ~SfxLinkUndoAction() when the pAction member is accessed.  This member is destroyed a little earlier by UndoManagerGuard().

The root cause is that undo actions are not reference counted and that ownership is not clearly defined.

The attached patch prevents undo actions from being deleted in ~UndoManagerGuard() when they are linked.  This would work under the assumption that linked actions would be destroyed by their owners.  But that seems to not be the case.  Therefore the patch prevents the crash for the cost of leaking undo action objects.
Comment 6 Oliver-Rainer Wittmann 2013-06-20 09:13:47 UTC
I produced the crash also occurs in OOo 3.1 and OOo 3.3 under Windows 7.
Comment 7 Armin Le Grand 2013-06-20 13:47:10 UTC
ALG: Can reproduce on Win7 on trunk. Also happens in 3.4.1 version. Also happens in OOo3.3 version. Also happens in OOo2.2 version. Seems to be a rather old crash, taking a look.
Comment 8 Armin Le Grand 2013-06-20 15:20:46 UTC
ALG: There is already some handling for the SfxUndoAction locally remembered inside of SfxLinkUndoAction to avoid deletion, but does not work everywhere. And it should be called a 'bad solution' that something like SfxLinkUndoAction exists at all, remembering an alien svl::IUndoManager and an alien SfxUndoAction in the local svl::IUndoManager. To generally fix this SfxUndoAction would need to be at least ref-counted, but also need a better relationship to their undo manager.
This would be a big change, for now I will try to make it more safe in a simplified way.
Comment 9 Armin Le Grand 2013-06-21 10:18:56 UTC
ALG: Oops, forget to grab it. Experimenting with ownership...
Comment 10 Armin Le Grand 2013-06-21 11:12:08 UTC
ALG: Have now secured ownership, but executing the undo itself crashes, too. Looking why...
Comment 11 Armin Le Grand 2013-06-23 11:11:46 UTC
ALG: Reason is a wrong order in some cases of the two to-be-merged paragraphs (thats what happens here in the editengine). Corrected this in the called consumer, also asserting this situations since they produce an illegal SfxLinkUndoAction which will crash on execution. Also corrected the caller to behave nice.
Comment 12 Armin Le Grand 2013-06-23 11:18:37 UTC
ALG: This also brings ub the asserion error in non-pro mode when page numbers are not equal for a short moment. Secured that in SD with asserting directly without calling SlideSorterModel::GetPageDescriptor again (which causes the crash).
Comment 13 Armin Le Grand 2013-06-23 11:22:09 UTC
ALG: Preparing commit. Be warned that this change is highly incompatible (from the codebase hierarchy, not from the code ;-), so after it a rebuild from svl will be needed.
Comment 14 Armin Le Grand 2013-06-23 11:25:46 UTC
ALG: Okay, done.
Comment 15 jsc 2013-07-02 12:04:28 UTC
grant showstopper flag, already fixed
Comment 16 fanyuzhen 2013-07-03 10:58:53 UTC
Which revision number the fix goes?
Comment 17 Armin Le Grand 2013-07-03 14:55:22 UTC
ALG: it's r1495808, somehow the commit did not create an entry here automagically.
Comment 18 fanyuzhen 2013-07-04 09:49:37 UTC
It is verified fixed in AOO 4.0 - revision 1496831