ScPostIt revision

[Mike Kaganski]

Hello!

While working on tdf#89226, I found out that there are a number of 
places in calc where ScPostIt's, as well as the SdrCaptionObj's 
(controlled by ScPostIt) are used after delete.

Turns out that quite a few regressions issued in bugzilla are rooted to 
that. To number a few:
tdf#89226
tdf#83192
tdf#91995
tdf#90741
tdf#83192

Another crash that I haven't issued to bugzilla (it would only add up to 
those already mentioned; don't know if it is already reported): Copy a 
cell with comment to clipboard → close original document → paste to 
another document.

So, I thought it's  manifestation of obscure interface/protocol of the 
objects that leads to broken conditions when implementers use them. 
Currently I'm evaluating possible changes to (mainly) ScPostIt that 
would make its usage more safe and robust.

This message is intended to notify everyone interested of this work, and 
to initiate a discussion on it, if someone happens to have an opinion on 
how to do things best. I would be most grateful for any opinion that 
could help me avoid making other's lives more difficult.

So, these are the outline of what I found and what intend to do.

1. Classes and their relationship.
ScPostIt is a class that represents a cell's note in calc documents. Its 
main "place of life" is in ScColumn::maCellNotes (sparse multitype 
vector of pointers to ScPostIt objects). The ScPostIt's that are pointed 
from maCellNotes of columns of ordinary documents (SCDOCMODE_DOCUMENT) 
are "ordinary" notes: they represent real notes that belong to some 
specific cells, have a reference to owning document, an author, creation 
date, and (possibly) a graphic caption object (SdrCaptionObj) with its 
text, size, position, formatting etc., or an information required to 
construct such object on demand. They may be used to add new notes, to 
control the caption objects (show/hide, etc.), or to remove existing notes.
Also, there are other kinds of documents and other objects that may 
"own" or use pointers to ScPostIt objects: Undo documents 
(SCDOCMODE_UNDO), Clipboard documents (SCDOCMODE_CLIP); 
CopyFromClipContext::maSingleNotes. Depending on the its document kind, 
the ScPostIt behaviour differ: e.g. when in Undo document, it doesn't 
try to delete caption object it points to, because the caption is 
"shared" with an "ordinary" note.

SdrCaptionObj is the graphic object (a rectangle with a text and a 
callout) of the note. It usually belongs to a ScPostIt (ScPostIt has a 
pointer to it). In the normal edit mode, it is created using ScPostIt 
methods and belong to it right away. Also, other ways are possible:
- creation of "temporary" caption that does not belong to any ScPostIt;
- creation of SdrCaptionObj during import (to later create an ScPostIt 
to hold it);
- passing ownership of the SdrCaptionObj to different Undo objects (like 
SdrUndoDelObj);
- passing pointers to SdrCaptionObj of an "ordinary" ScPostIt to 
different objects (like Undo objects: SdrUndoGeoObj, ScUndoObjData, 
Undo/Clipboard ScPostIt's, and so on) without transferring ownership.

This leads to these currently implemented crash scenarios:
1. When forming an undo/redo documents for an action, the ScPostIt's are 
not copied to the documents, but pointers to them (see current 
ScColumn::CopyCellToDocument). If afterwards the action is undone, the 
restoration of the note is implemented as "cloning", i.e. creating a new 
copy of the ScPostIt, and assigning that copy to cell (thus deleting 
original ScPostIt, that is still pointed to from Undo/Redo documents). 
Thus, subsequent Redo naturally crashes LO.
2. When creating a note, then modifying cell, then undoing twice - the 
first undo deletes the note, the second tries to delete already deleted 
note;
3. When creating a note, then modifying a cell, then removing the note, 
then undo twice, then redo twice - the same.
4. When copying a cell with a note to clipboard, then closing original 
document, then pasting to another document (that was open before closing 
the first) - the inserted note is badly formatted.
5. Same as above, but the second document is created after the first was 
closed - crash (has nothing to do with unloading DLLs, because another 
calc document may be open all the time) - the clipboard references 
already deleted object.
... and so on.

2. Intended modifications.
One possible approach is to avoid multiple pointers to same 
ScPostIt/SdrCaptionObj, and always do deep copies. While somewhat more 
expensive, taking into account small number of notes that are typically 
in a document, only a small overhead should be expected. However, it may 
lead to much more widespread changes: e.g., undoing any modification of 
the note itself (say, typing a text) should not expect to find the 
object it was created for, but instead always re-create new notes. I 
suppose that this route is not optimal.
Another one is to keep as much of current approach as possible. I 
suppose that the following policy should be established for the objects 
(and documented in code): when copying a "ordinary" note to another cell 
of ordinary document - regardless if it's the same document or another - 
always do deep copy. If copying "ordinary" note to/from undo, then only 
do "shallow" copy, and the shared reference to SdrCaptionObj must be 
made using std::shared_ptr. This will allow to keep the object while 
there's at least one client is alive. Passing the pointer of 
SdrCaptionObj to other *owners* should be done via shared_ptr, too. 
Copying to/from clipboard must be a deep copy.
Regarding deep copies: I'm unsure if it's possible to fit everything 
that substitutes the SdrCaptionObj into current implementation of 
initial information (ScCaptionInitData). There are the following data:
- pointer to SfxItemSet;
- pointer to OutlinerParaObject;
- OUString maSimpleText (mutually exclusive with OutlinerParaObject);
- Point maCaptionOffset;
- Size maCaptionSize;
- bool mbDefaultPosSize.
If it is enough, then the deep copy should be simply constructing the 
ScCaptionInitData from SdrCaptionObj or another ScCaptionInitData. If 
not, then deep copy should be copy of the SdrCaptionObj itself, but 
(possibly) without placing to drawing layer (e.g. when in clipboard).

The constructors should be easy to use (and understand when to use 
which). The purpose of the object should be set on creation (I think 
using ScDocumentMode). The decision of strategy (shallow/deep copy) 
should sit inside copy constructors, and depend on types of original and 
target, and not on other data.

That's a summary of what I'm trying to do. I'm very thankful for 
anything you'd like to share on this topic. Thank you.

-- 
Best regards,
Mike Kaganski
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/libreoffice/attachments/20150617/b9672b40/attachment.html>