<div dir="ltr">Hey,<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jun 17, 2015 at 12:20 PM, Mike Kaganski <span dir="ltr"><<a href="mailto:mike.kaganski@collabora.com" target="_blank">mike.kaganski@collabora.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000">
<div style="font-family:-moz-fixed;font-size:14px" lang="x-unicode">Hello!
<br>
<br>
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.
<br>
<br>
Turns out that quite a few regressions issued in bugzilla are
rooted to that. To number a few:
<br>
tdf#89226
<br>
tdf#83192
<br>
tdf#91995
<br>
tdf#90741
<br>
tdf#83192
<br>
<br>
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.
<br>
<br>
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.
<br></div></div></blockquote><div><br></div><div>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, ...<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div style="font-family:-moz-fixed;font-size:14px" lang="x-unicode">
<br>
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.
<br>
<br>
So, these are the outline of what I found and what intend to do.
<br>
<br>
1. Classes and their relationship.
<br>
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.
<br>
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.
<br></div></div></blockquote><div><br></div><div>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.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div style="font-family:-moz-fixed;font-size:14px" lang="x-unicode">
<br>
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:
<br>
- creation of "temporary" caption that does not belong to any
ScPostIt;
<br>
- creation of SdrCaptionObj during import (to later create an
ScPostIt to hold it);
<br>
- passing ownership of the SdrCaptionObj to different Undo objects
(like SdrUndoDelObj);
<br>
- 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.
<br>
<br>
This leads to these currently implemented crash scenarios:
<br>
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.
<br>
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;
<br>
3. When creating a note, then modifying a cell, then removing the
note, then undo twice, then redo twice - the same.
<br>
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.
<br>
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.
<br>
... and so on.
<br>
<br>
2. Intended modifications.
<br>
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.
<br></div></div></blockquote><div><br><br></div><div>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.<br><br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div bgcolor="#FFFFFF" text="#000000"><div style="font-family:-moz-fixed;font-size:14px" lang="x-unicode">
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 <b><span>*</span>owners<span>*</span></b> should be done via
shared_ptr, too. Copying to/from clipboard must be a deep copy.
<br></div></div></blockquote><div><br><br></div><div>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.<br><br>[...]<br></div><div><br><br></div><div>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.<br><br></div><div>Maybe Kohei will add some more points that I forgot but in general I think an incremental approach is necessary here.<br></div><div><br></div><div>Regards,<br></div><div>Markus<br></div></div><br></div></div>