<html> <head> <meta http-equiv="content-type" content="text/html; charset=utf-8"> </head> <body smarttemplateinserted="true" bgcolor="#FFFFFF" text="#000000"> <div class="moz-text-flowed" style="font-family: -moz-fixed; font-size: 14px;" lang="x-unicode">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. <div class="moz-txt-sig">-- Best regards, Mike Kaganski </div> </div> <div class="st4cursor"></div> </body> </html>