Re: ScPostIt revision

Hey,

On Wed, Jun 17, 2015 at 12:20 PM, Mike Kaganski <mike.kaganski@collabora.com
> wrote:

>  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.

Notes are not a sim[ple object as they combine a drawinglayer object + a
calc core object. So the lifecycle of these will always be a bit more
complicated than that of a normal cell, ...


> 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.

That is the correct and useful behavior. You don't want to create new
drawinglayer objects in copy and undo documents if possible as they are
quite expensive and are causing quite a lot of pain.


> 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.


This assumption will not hold. There are some users, notably Laurent, who
have a huge number of notes and who spend quite some optimizing the notes
storage. As Laurent just recently spend a few days/weeks on optimizing the
notes storage and handling of documents with many notes it would be good
not to just dismiss his use case.



> 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.


IMHO we should use a shared_ptr for the drawing layer object but it will
not solve everything (it still means you need to use the correct method in
the callers). The general architecture is well known and worked for a long
time. The problem lies in the details and large refactorings like the calc
core change. In general the problem with the notes lifecycle are mostly in
the callers and not in the implementation itself.

[...]


My suggestion would be to start small with something simple as moving to
shared_ptr and adding asserts that make sure that the assumptions that you
have are valid. If after that task and realizing that at least 2 calc
developers only managed to fix some of the problems around notes and note
captions I recommend starting adding some tests before implementing
something new. You'd be surprised to discover how many corner cases there
are that you never thought about but are currently handled in the code.

Maybe Kohei will add some more points that I forgot but in general I think
an incremental approach is necessary here.

Regards,
Markus