Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
On 09/25/2011 01:48 PM, Mathias Bauer wrote: The differentiation between hard and soft refererences would require a solid understanding of the relationships of all objects. With this understanding you could most probably fix the existing code without refcounting also: a weak reference always could be replaced by a callback function that tells possible consumers of an object that this object is going to die (and so manually sets the pointer to Null), a pattern that can be found in OOo's code in other places (e.g. SfxBroadcaster or SwNotify objects). But of course using refcounting here would make the code clearer. And another advantage of weak refs over notifications is that the former tend to behave correctly in multi-threaded scenarios while the latter tend to misbehave. -Stephan ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
On Sun, 25 Sep 2011 13:48:26 +0200 Mathias Bauer nospamfor...@gmx.de wrote: The biggest part of the work is getting the right picture: which objects control the life time of others and which just need to know if some other objects are present or not. ^- That is the hard part. And the one with the biggest boon in the end. If one had a clear ownership relation between layout objects one would neither need notifications nor weak references for the most part -- and have the code a lot less fragile and easier to debug in the end too. Best, Bjoern -- https://launchpad.net/~bjoern-michaelsen ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
Am 24.09.2011 09:57, schrieb Michael Meeks: On Sat, 2011-09-24 at 00:36 +0200, Mathias Bauer wrote: Really, the layout is a complete mess, at least from an architectural POV. There is no clear relationship and ownership between objects. That might work better in a world with garbage collection, but in the ugly world of pointers this is the road to hell. Hey :-) first - many thanks for your review, and great to see you here ! I just did a size-able cleanup of color pieces to use reference-counting everywhere - fixing a number of rather hideous tangled lifecycle issues in the process, and I was wondering, would it make sense to add a ref-count + 'dispose' pattern to these Layout objects in a big one-shot re-factor ? I expect that just adding refcounting for all layout objects here would lead to cyclic references and so probably would just replace one problem with another. So surely some of the involved references would need to be weak references. If I'm not wronf, that's probably a different way to express the refcount + dispose pattern you have mentioned. Using weak refs requires checking them before usage, OTOH you don't rely on a proper implementation of the usage and implementation of dispose() calls and you don't need to pave your code with Dispose() calls. Probably a matter of taste. The differentiation between hard and soft refererences would require a solid understanding of the relationships of all objects. With this understanding you could most probably fix the existing code without refcounting also: a weak reference always could be replaced by a callback function that tells possible consumers of an object that this object is going to die (and so manually sets the pointer to Null), a pattern that can be found in OOo's code in other places (e.g. SfxBroadcaster or SwNotify objects). But of course using refcounting here would make the code clearer. The biggest part of the work is getting the right picture: which objects control the life time of others and which just need to know if some other objects are present or not. In the footnotes example obviously some objects have pointer of other objectes that are already destroyed. We had to decide if they either need to keep these objects alive until they are done with them or if they just had to keep a weak reference to them to avoid the crash we are seeing now. Just my 2 cents. Regards, Mathias ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
On Sat, 2011-09-24 at 00:36 +0200, Mathias Bauer wrote: Really, the layout is a complete mess, at least from an architectural POV. There is no clear relationship and ownership between objects. That might work better in a world with garbage collection, but in the ugly world of pointers this is the road to hell. Hey :-) first - many thanks for your review, and great to see you here ! I just did a size-able cleanup of color pieces to use reference-counting everywhere - fixing a number of rather hideous tangled lifecycle issues in the process, and I was wondering, would it make sense to add a ref-count + 'dispose' pattern to these Layout objects in a big one-shot re-factor ? Thanks, Michael. -- michael.me...@suse.com , Pseudo Engineer, itinerant idiot ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
Am 22.09.2011 19:12, schrieb Michael Stahl: On 22.09.2011 18:51, Bjoern Michaelsen wrote: Hi Cedric, Hi Caolan, could you please review: http://cgit.freedesktop.org/libreoffice/core/commit/?id=ac1912ecb13709082026428d2b2a56c4915b939f for backporting to -3-4? It kills the footnotes before killing the rest of the layout saving us from the trouble later. It seems to fix the problem with the test document here and also that fix passes all unittests and subequenttests on master. is this the kind of crash that was introduced by mba's layout refactoring (moving the layout from the model to the view) that went into one of the last DEV300 milestones? That's indeed possible. i think the workaround for OOo 3.4 was this: http://svn.apache.org/viewvc?rev=1172362view=rev wonder what Oliver and Mathias had in mind as the proper fix... The idea was to add a new method IsInDtor to the layout and use this instead of the method of SwDoc where appropriate. The *real* fix would be fixing the layout mess so that it does not crash when a layout of a certain complexity is destroyed. But most probably that come close to a rewrite. The patch with the workaround you mentioned (that BTW meanwhiles is integrated in AOOo) has an explanation for the problem in vnew.cxx. Regards, Mathias ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
[Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
Hi Cedric, Hi Caolan, could you please review: http://cgit.freedesktop.org/libreoffice/core/commit/?id=ac1912ecb13709082026428d2b2a56c4915b939f for backporting to -3-4? It kills the footnotes before killing the rest of the layout saving us from the trouble later. It seems to fix the problem with the test document here and also that fix passes all unittests and subequenttests on master. Best, Bjoern @Rene: If this passes review, you probably want to backport it on Debians 3.4.3. -- https://launchpad.net/~bjoern-michaelsen ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
On 22.09.2011 18:51, Bjoern Michaelsen wrote: Hi Cedric, Hi Caolan, could you please review: http://cgit.freedesktop.org/libreoffice/core/commit/?id=ac1912ecb13709082026428d2b2a56c4915b939f for backporting to -3-4? It kills the footnotes before killing the rest of the layout saving us from the trouble later. It seems to fix the problem with the test document here and also that fix passes all unittests and subequenttests on master. is this the kind of crash that was introduced by mba's layout refactoring (moving the layout from the model to the view) that went into one of the last DEV300 milestones? i think the workaround for OOo 3.4 was this: http://svn.apache.org/viewvc?rev=1172362view=rev wonder what Oliver and Mathias had in mind as the proper fix... if your patch helps then i don't object to it, but i'd really like a comment in the added line why it's been added (which is certainly non-obvious here). -- ENOSIG ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
Re: [Libreoffice] [REVIEW 3-4] fdo#39510 crash on closing document with footnotes
On Thu, 22 Sep 2011 19:12:38 +0200 Michael Stahl m...@openoffice.org wrote: is this the kind of crash that was introduced by mba's layout refactoring (moving the layout from the model to the view) that went into one of the last DEV300 milestones? Likely. i think the workaround for OOo 3.4 was this: http://svn.apache.org/viewvc?rev=1172362view=rev wonder what Oliver and Mathias had in mind as the proper fix... Given that that would work only by luck (and a good dose of evil IsInDtor() sprayed in), I would consider it better to fix this like I did. (Although: If more such gremlins pop up we should make that SwDoc/SwRootFrm one janusfaced object to rule them all on the -3-4 branch again). if your patch helps then i don't object to it, but i'd really like a comment in the added line why it's been added (which is certainly non-obvious here). git blame is your friend. ;) Anyway: http://cgit.freedesktop.org/libreoffice/core/commit/?id=1b37830b605972649fe54c29fbb952df0e4c9682 adds some prose. Best, Bjoern -- https://launchpad.net/~bjoern-michaelsen ___ LibreOffice mailing list LibreOffice@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/libreoffice
