Swatch dialog has a serious bug
Hi all, I just discovered a very serious bug in the swatches dialog (/src/ui/dialog/swatches.cpp). Using a vector of DocTrack*, it tracks all current documents, and using a timer callback updates itself. The problem is, no DocTrack* is ever removed from that list. A DocTrack* (that points to an SPDocument) is added whenever you open a new document. Then when closing a 2nd document, the DocTrack* should be removed from the list and ~DocTrack should be called. Because this is _not_ happen, the timerfunction will use an already deleted SPDoc and will eventually crash in SPDocument::getResourceList (priv is set to nullptr for deleted SPDoc).
I added proper ref-counting of the SPDoc object in DocTrack's constructor and destructor, but still the SPDoc is deleted and the crash occurs.
Some code in swatches.cpp looks unfinished, creating data structures that are never used...
Thanks for any help in tracking down and fixing the issue, Johan
On Feb 27, 2014, at 2:44 PM, Johan Engelen wrote:
Hi all, I just discovered a very serious bug in the swatches dialog (/src/ui/dialog/swatches.cpp). Using a vector of DocTrack*, it tracks all current documents, and using a timer callback updates itself. The problem is, no DocTrack* is ever removed from that list. A DocTrack* (that points to an SPDocument) is added whenever you open a new document. Then when closing a 2nd document, the DocTrack* should be removed from the list and ~DocTrack should be called. Because this is _not_ happen, the timerfunction will use an already deleted SPDoc and will eventually crash in SPDocument::getResourceList (priv is set to nullptr for deleted SPDoc).
I added proper ref-counting of the SPDoc object in DocTrack's constructor and destructor, but still the SPDoc is deleted and the crash occurs.
Some code in swatches.cpp looks unfinished, creating data structures that are never used...
Thanks for any help in tracking down and fixing the issue, Johan
I can take a look at this.
There are a few things that are well overdue for refactoring. We also need to tune things up to not keep so much in memory.
On Feb 27, 2014, at 3:06 PM, Johan Engelen wrote:
On 27-2-2014 23:57, Jon Cruz wrote:
I can take a look at this.
There are a few things that are well overdue for refactoring. We also need to tune things up to not keep so much in memory.
Thank you very much Jon.
Well, I fixed the cleanup with adding a destroy signal to SPDocument... but there are still odd problems in the cleanup of the undo stack. That seems a little more problematic as it is not in the swatches code. Fun.
On 2-3-2014 19:25, Jon Cruz wrote:
On Feb 27, 2014, at 3:06 PM, Johan Engelen wrote:
On 27-2-2014 23:57, Jon Cruz wrote:
I can take a look at this.
There are a few things that are well overdue for refactoring. We also need to tune things up to not keep so much in memory.
Thank you very much Jon.
Well, I fixed the cleanup with adding a destroy signal to SPDocument... but there are still odd problems in the cleanup of the undo stack. That seems a little more problematic as it is not in the swatches code. Fun.
Thanks for the fix.
I am surprised that SPDoc was deleted at all after adding an extra ref to it in DocTrack. Somehow the refcounting of SPDoc is broken?
Perhaps you can add a brief comment to the signal that any attached handler may not throw?
ciao, Johan
On Mar 2, 2014, at 12:59 PM, Johan Engelen wrote:
On 2-3-2014 19:25, Jon Cruz wrote:
On Feb 27, 2014, at 3:06 PM, Johan Engelen wrote:
On 27-2-2014 23:57, Jon Cruz wrote:
I can take a look at this.
There are a few things that are well overdue for refactoring. We also need to tune things up to not keep so much in memory.
Thank you very much Jon.
Well, I fixed the cleanup with adding a destroy signal to SPDocument... but there are still odd problems in the cleanup of the undo stack. That seems a little more problematic as it is not in the swatches code. Fun.
Thanks for the fix.
I am surprised that SPDoc was deleted at all after adding an extra ref to it in DocTrack. Somehow the refcounting of SPDoc is broken?
Well... I checked the involved code, and the View was actually explicitly deleting the SPDesktop. So refcounting doesn't help if there is an explicit delete.
I think this is another point supporting my initial position on GC: if garbage collection is not part of the language itself, then it is tricky to successfully retrofit.
On 2-3-2014 22:21, Jon Cruz wrote:
On Mar 2, 2014, at 12:59 PM, Johan Engelen wrote:
On 2-3-2014 19:25, Jon Cruz wrote:
On Feb 27, 2014, at 3:06 PM, Johan Engelen wrote:
On 27-2-2014 23:57, Jon Cruz wrote:
I can take a look at this.
There are a few things that are well overdue for refactoring. We also need to tune things up to not keep so much in memory.
Thank you very much Jon.
Well, I fixed the cleanup with adding a destroy signal to SPDocument... but there are still odd problems in the cleanup of the undo stack. That seems a little more problematic as it is not in the swatches code. Fun.
Thanks for the fix.
I am surprised that SPDoc was deleted at all after adding an extra ref to it in DocTrack. Somehow the refcounting of SPDoc is broken?
Well... I checked the involved code, and the View was actually explicitly deleting the SPDesktop. So refcounting doesn't help if there is an explicit delete.
I think this is another point supporting my initial position on GC: if garbage collection is not part of the language itself, then it is tricky to successfully retrofit.
Can't we make the destructor of SPDoc private to prevent explicit delete? (btw, I think of explicit refcounting as a bit different from GC, but probably that's just me)
cheers, Johan
On Mar 2, 2014, at 1:54 PM, Johan Engelen wrote:
On 2-3-2014 22:21, Jon Cruz wrote:
On Mar 2, 2014, at 12:59 PM, Johan Engelen wrote:
On 2-3-2014 19:25, Jon Cruz wrote:
On Feb 27, 2014, at 3:06 PM, Johan Engelen wrote:
On 27-2-2014 23:57, Jon Cruz wrote:
I can take a look at this.
There are a few things that are well overdue for refactoring. We also need to tune things up to not keep so much in memory.
Thank you very much Jon.
Well, I fixed the cleanup with adding a destroy signal to SPDocument... but there are still odd problems in the cleanup of the undo stack. That seems a little more problematic as it is not in the swatches code. Fun.
Thanks for the fix.
I am surprised that SPDoc was deleted at all after adding an extra ref to it in DocTrack. Somehow the refcounting of SPDoc is broken?
Well... I checked the involved code, and the View was actually explicitly deleting the SPDesktop. So refcounting doesn't help if there is an explicit delete.
I think this is another point supporting my initial position on GC: if garbage collection is not part of the language itself, then it is tricky to successfully retrofit.
Can't we make the destructor of SPDoc private to prevent explicit delete? (btw, I think of explicit refcounting as a bit different from GC, but probably that's just me)
That doesn't really help much in this case. In fact, the proper fix is to add that notify signal. Other signals can be connected to, and that (emitting a destroy signal to tell other parts of the code to release it) is the standard GTK+ design approach.
participants (2)
-
Johan Engelen -
Jon Cruz