From: Mark H. <mar...@bt...> - 2009-09-29 17:12:58
|
Hello All While searching for the reason for a crash on exit of my application, I have come across what I think is a bug in OWL, but would like to get the opinions of others on the list to see if I am correct in my analysis. Basically if you create a TBrush that uses a system color, then ::GetSysColorBrush is called in the TBrush constructor. The handle for this brush is then generally added to the TBrushCache and is also reference counted. If useCache is false, which it isn't by default, then the brush is just reference counted. The problem is that according to MSDN "System color brushes are owned by the system and must not be destroyed". However, OWL will destroy the brush when the BrushCache is deleted and the reference count on the brush handle is zero. This seems wrong to me and is causing an issue with one of my apps. An ActiveX control that I am using is utilising a system color brush with the same color as one of the system color brushes created by OWL elsewhere in my app. When the app exits, OWL tries to delete the BrushCache and thus destroy the above mentioned brush. However, the brush has already been destroyed at this point, presumably by the system, so an exception is thrown from TGdiObject::RefDec when DeleteObject is called on the brush. Therefore, I propose that caching and also reference counting should not happen for a system color TBrush. This would mean the constructor should look something like the version given below. I would be interested to know if everyone agrees with these findings and the proposed solution. Best Regards Mark // // Construct a brush from a solid color. Gets handle from the cache if found, // otherwise create the brush and add it to the cache // If useCache == false not add brush to cache. TBrush::TBrush(const TColor& color, bool useCache) { //_TRACE("TBrush::TBrush(const TColor& color, bool useCache)\n"); ShouldDelete = !useCache; COLORREF cr = color; if(useCache && (Handle = GetBrushCache().Lookup(cr)) != 0) return; //Use new win4.0 function if available? Probably very fast. if (color.IsSysColor()) Handle = ::GetSysColorBrush(color.Index()); else { Handle = ::CreateSolidBrush(cr); } WARNX(OwlGDI, !Handle, 0, _T("Cannot create solid TBrush ") << hex << color); CheckValid(); if (!color.IsSysColor()) { if(useCache){ if(!GetBrushCache().AddRef(Handle, color)){ TRACEX(OwlGDI, OWL_CDLEVEL, _T("Cannot add new TBrush to cache ") << hex << color); ShouldDelete = true; } } else RefAdd(Handle, Brush); } TRACEX(OwlGDI, OWL_CDLEVEL, _T("TBrush constructed @") << (void*)this << _T(" with color ") << (COLORREF)color); } |
From: Jogy <jo...@si...> - 2009-10-02 19:19:27
|
Hello, Mark, Thanks for finding this issue, I also wonder, if it is sensible to store these system brushes in the cache? A reason for using cache is that creating and destroying brushes can be expensive operation, better to pool them. But for system brushes, it may be not so, as they are cached by the OS. Also, "when the user changes a system color, the associated system color brush automatically changes to the new color" but if the brush is cached by OWLNext, this may not happen. I will do some research and tests on these issues. Jogy Mark Hatsell wrote: > Hello All > > While searching for the reason for a crash on exit of my application, > I have come across what I think is a bug in OWL, but would like to get > the opinions of others on the list to see if I am correct in my analysis. > > Basically if you create a TBrush that uses a system color, then > ::GetSysColorBrush is called in the TBrush constructor. The handle for > this brush is then generally added to the TBrushCache and is also > reference counted. If useCache is false, which it isn't by default, > then the brush is just reference counted. > > The problem is that according to MSDN "System color brushes are owned > by the system and must not be destroyed". However, OWL will destroy > the brush when the BrushCache is deleted and the reference count on > the brush handle is zero. This seems wrong to me and is causing an > issue with one of my apps. An ActiveX control that I am using is > utilising a system color brush with the same color as one of the > system color brushes created by OWL elsewhere in my app. When the app > exits, OWL tries to delete the BrushCache and thus destroy the above > mentioned brush. However, the brush has already been destroyed at this > point, presumably by the system, so an exception is thrown from > TGdiObject::RefDec when DeleteObject is called on the brush. > > Therefore, I propose that caching and also reference counting should > not happen for a system color TBrush. This would mean the constructor > should look something like the version given below. I would be > interested to know if everyone agrees with these findings and the > proposed solution. > > Best Regards > Mark > > > > // > // Construct a brush from a solid color. Gets handle from the cache if > found, > // otherwise create the brush and add it to the cache > // If useCache == false not add brush to cache. > TBrush::TBrush(const TColor& color, bool useCache) > { > //_TRACE("TBrush::TBrush(const TColor& color, bool useCache)\n"); > ShouldDelete = !useCache; > > COLORREF cr = color; > if(useCache && (Handle = GetBrushCache().Lookup(cr)) != 0) > return; > > //Use new win4.0 function if available? Probably very fast. > if (color.IsSysColor()) > Handle = ::GetSysColorBrush(color.Index()); > else { > Handle = ::CreateSolidBrush(cr); > } > WARNX(OwlGDI, !Handle, 0, _T("Cannot create solid TBrush ") << hex > << color); > CheckValid(); > > if (!color.IsSysColor()) { > if(useCache){ > if(!GetBrushCache().AddRef(Handle, color)){ > TRACEX(OwlGDI, OWL_CDLEVEL, _T("Cannot add new TBrush to cache > ") << hex << color); > ShouldDelete = true; > } > } > else > RefAdd(Handle, Brush); > } > > TRACEX(OwlGDI, OWL_CDLEVEL, _T("TBrush constructed @") << (void*)this << > _T(" with color ") << (COLORREF)color); > } > |
From: Jogy <jo...@si...> - 2009-10-19 08:15:46
|
Hello, Mark, I did some more research on the issue with ::GetSysColroBrush(): 1. Speed I found out that calling ::GetSysColroBrush() is not that much faster than using ::CreateSolidBrush(). Of course, both calls are very fast on today's computers, I allocated millions of brushes to be able to compare the total time in seconds. Using the TBrushCache in both cases speeds thing up about ten times - so I suppose that it is not the actual WinAPI call that is slow, but the further OWLNext processing that is slower. 2. At least under Vista, calling ::GetSysColroBrush(), then ::DeleteObject() on the returned handle, and then again ::GetSysColroBrush() returns the same handle, and no problems seems to occur. I suspect that ::DeleteObject() internally checks the handle and does not free it if it should not ... but that may not be true for all past and future versions of Windows. To avoid the problem with calling ::DeleteObject(), but still be able to cache the system color brushes, a possible solution will be to store the handles returned by ::GetSysColroBrush() in an internal collection and TGdiObject can check is the handle it is about to delete is in this collection, skip the call to ::DeleteObject(). What do you think of this idea? Jogy Mark Hatsell wrote: > Hello All > > While searching for the reason for a crash on exit of my application, > I have come across what I think is a bug in OWL, but would like to get > the opinions of others on the list to see if I am correct in my analysis. > > Basically if you create a TBrush that uses a system color, then > ::GetSysColorBrush is called in the TBrush constructor. The handle for > this brush is then generally added to the TBrushCache and is also > reference counted. If useCache is false, which it isn't by default, > then the brush is just reference counted. > > The problem is that according to MSDN "System color brushes are owned > by the system and must not be destroyed". However, OWL will destroy > the brush when the BrushCache is deleted and the reference count on > the brush handle is zero. This seems wrong to me and is causing an > issue with one of my apps. An ActiveX control that I am using is > utilising a system color brush with the same color as one of the > system color brushes created by OWL elsewhere in my app. When the app > exits, OWL tries to delete the BrushCache and thus destroy the above > mentioned brush. However, the brush has already been destroyed at this > point, presumably by the system, so an exception is thrown from > TGdiObject::RefDec when DeleteObject is called on the brush. > > Therefore, I propose that caching and also reference counting should > not happen for a system color TBrush. This would mean the constructor > should look something like the version given below. I would be > interested to know if everyone agrees with these findings and the > proposed solution. > > Best Regards > Mark > > > > // > // Construct a brush from a solid color. Gets handle from the cache if > found, > // otherwise create the brush and add it to the cache > // If useCache == false not add brush to cache. > TBrush::TBrush(const TColor& color, bool useCache) > { > //_TRACE("TBrush::TBrush(const TColor& color, bool useCache)\n"); > ShouldDelete = !useCache; > > COLORREF cr = color; > if(useCache && (Handle = GetBrushCache().Lookup(cr)) != 0) > return; > > //Use new win4.0 function if available? Probably very fast. > if (color.IsSysColor()) > Handle = ::GetSysColorBrush(color.Index()); > else { > Handle = ::CreateSolidBrush(cr); > } > WARNX(OwlGDI, !Handle, 0, _T("Cannot create solid TBrush ") << hex > << color); > CheckValid(); > > if (!color.IsSysColor()) { > if(useCache){ > if(!GetBrushCache().AddRef(Handle, color)){ > TRACEX(OwlGDI, OWL_CDLEVEL, _T("Cannot add new TBrush to cache > ") << hex << color); > ShouldDelete = true; > } > } > else > RefAdd(Handle, Brush); > } > > TRACEX(OwlGDI, OWL_CDLEVEL, _T("TBrush constructed @") << (void*)this << > _T(" with color ") << (COLORREF)color); > } > > > ------------------------------------------------------------------------ > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry® Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9-12, 2009. Register now! > http://p.sf.net/sfu/devconf > ------------------------------------------------------------------------ > > _______________________________________________ > OwlNExt-users mailing list > Owl...@li... > https://lists.sourceforge.net/lists/listinfo/owlnext-users > |
From: Mark H. <mar...@bt...> - 2009-10-19 12:43:25
|
Hi Jogy Thanks for the useful info regarding the brush caching functionality of OWL. I think that your solution would work fine. However, is it not achieving the same as simply not reference counting a system color TBrush? If RefAdd is never called for the TBrush it should never get deleted via DeleteObject. Of course I may be missing something here completely, such as the reference count being used for purposes other than deletion?? btw, did you see my other post regarding TCmdLine? http://sourceforge.net/mailarchive/message.php?msg_name=0A42D6352CD1481785915D388BA44A46%40Laptop3 Any chance that this can make it into the update? I realize that it's not a perfect solution but at least it allows you to work with command line options containing whitespace. Regards Mark ----- Original Message ----- From: "Jogy" <jo...@si...> To: "Mark Hatsell" <mar...@bt...> Cc: <owl...@li...> Sent: Monday, October 19, 2009 9:15 AM Subject: Re: [OwlNExt-users] Should TBrush be caching or ref counting System color brushes > Hello, Mark, > > I did some more research on the issue with ::GetSysColroBrush(): > > 1. Speed > I found out that calling ::GetSysColroBrush() is not that much faster > than using ::CreateSolidBrush(). > Of course, both calls are very fast on today's computers, I allocated > millions of brushes to be able to compare the total time in seconds. > > Using the TBrushCache in both cases speeds thing up about ten times - so > I suppose that it is not the actual WinAPI call that is slow, > but the further OWLNext processing that is slower. > > 2. At least under Vista, calling ::GetSysColroBrush(), then > ::DeleteObject() on the returned handle, > and then again ::GetSysColroBrush() returns the same handle, and no > problems seems to occur. > I suspect that ::DeleteObject() internally checks the handle and does > not free it if it should not ... > but that may not be true for all past and future versions of Windows. > > > To avoid the problem with calling ::DeleteObject(), but still be able to > cache the system color brushes, > a possible solution will be to store the handles returned by > ::GetSysColroBrush() in an internal collection > and TGdiObject can check is the handle it is about to delete is in this > collection, skip the call to ::DeleteObject(). > > What do you think of this idea? > > Jogy > > > > > > > > > > > Mark Hatsell wrote: >> Hello All >> >> While searching for the reason for a crash on exit of my application, >> I have come across what I think is a bug in OWL, but would like to get >> the opinions of others on the list to see if I am correct in my analysis. >> >> Basically if you create a TBrush that uses a system color, then >> ::GetSysColorBrush is called in the TBrush constructor. The handle for >> this brush is then generally added to the TBrushCache and is also >> reference counted. If useCache is false, which it isn't by default, >> then the brush is just reference counted. >> >> The problem is that according to MSDN "System color brushes are owned >> by the system and must not be destroyed". However, OWL will destroy >> the brush when the BrushCache is deleted and the reference count on >> the brush handle is zero. This seems wrong to me and is causing an >> issue with one of my apps. An ActiveX control that I am using is >> utilising a system color brush with the same color as one of the >> system color brushes created by OWL elsewhere in my app. When the app >> exits, OWL tries to delete the BrushCache and thus destroy the above >> mentioned brush. However, the brush has already been destroyed at this >> point, presumably by the system, so an exception is thrown from >> TGdiObject::RefDec when DeleteObject is called on the brush. >> >> Therefore, I propose that caching and also reference counting should >> not happen for a system color TBrush. This would mean the constructor >> should look something like the version given below. I would be >> interested to know if everyone agrees with these findings and the >> proposed solution. >> >> Best Regards >> Mark >> >> >> >> // >> // Construct a brush from a solid color. Gets handle from the cache if >> found, >> // otherwise create the brush and add it to the cache >> // If useCache == false not add brush to cache. >> TBrush::TBrush(const TColor& color, bool useCache) >> { >> //_TRACE("TBrush::TBrush(const TColor& color, bool useCache)\n"); >> ShouldDelete = !useCache; >> >> COLORREF cr = color; >> if(useCache && (Handle = GetBrushCache().Lookup(cr)) != 0) >> return; >> >> //Use new win4.0 function if available? Probably very fast. >> if (color.IsSysColor()) >> Handle = ::GetSysColorBrush(color.Index()); >> else { >> Handle = ::CreateSolidBrush(cr); >> } >> WARNX(OwlGDI, !Handle, 0, _T("Cannot create solid TBrush ") << hex >> << color); >> CheckValid(); >> >> if (!color.IsSysColor()) { >> if(useCache){ >> if(!GetBrushCache().AddRef(Handle, color)){ >> TRACEX(OwlGDI, OWL_CDLEVEL, _T("Cannot add new TBrush to cache >> ") << hex << color); >> ShouldDelete = true; >> } >> } >> else >> RefAdd(Handle, Brush); >> } >> >> TRACEX(OwlGDI, OWL_CDLEVEL, _T("TBrush constructed @") << (void*)this >> << >> _T(" with color ") << (COLORREF)color); >> } >> >> >> ------------------------------------------------------------------------ >> >> ------------------------------------------------------------------------------ >> Come build with us! The BlackBerry® Developer Conference in SF, CA >> is the only developer event you need to attend this year. Jumpstart your >> developing skills, take BlackBerry mobile applications to market and stay >> ahead of the curve. Join us from November 9-12, 2009. Register >> now! >> http://p.sf.net/sfu/devconf >> ------------------------------------------------------------------------ >> >> _______________________________________________ >> OwlNExt-users mailing list >> Owl...@li... >> https://lists.sourceforge.net/lists/listinfo/owlnext-users >> > -------------------------------------------------------------------------------- No virus found in this incoming message. Checked by AVG - www.avg.com Version: 8.5.422 / Virus Database: 270.14.20/2444 - Release Date: 10/18/09 09:04:00 |
From: Jogy <jo...@si...> - 2009-10-20 10:25:07
|
Mark Hatsell wrote: > Hi Jogy > > Thanks for the useful info regarding the brush caching functionality of OWL. > I think that your solution would work fine. However, is it not achieving the > same as simply not reference counting a system color TBrush? If RefAdd is > never called for the TBrush it should never get deleted via DeleteObject. Of > course I may be missing something here completely, such as the reference > count being used for purposes other than deletion?? > > Hello, Mark, I tried some solution with not adding a reference count, but there are some problems, the destructors look like this: TBrush::~TBrush() { if(!ShouldDelete) GetBrushCache().DecRef(Handle); TRACEX(OwlGDI, OWL_CDLEVEL, _T("TBrush destructed @") << (void*)this); } TGdiObject::~TGdiObject() { if (ShouldDelete) RefDec(Handle, true); TRACEX(OwlGDI, 2, "TGdiObject destructed " << *this); } So, no matter what the value of ShouldDelete is, the reference is decremented, and there are internal checks and traces that rely on the reference counting. I tried a solution, where I added an additional flag in the internal class TBrushCache to indicate that the handle is a system brush and should not be reference counted, and setting ShouldDelete to false. but this failed for example in this scenario: // First create a system brush and put in the cache TBrush brush(TColor::SysActiveCaption, true); // Second, create the same system brush, without using the cache TBrush brush2(TColor::SysActiveCaption, false); because the returned brush handle is the same, the second object on destruction finds it in the brush cache and tries to decrement the reference, it goes negative and a check fails. I would have to add special handling for this case, and there can be other weird scenarios, like when using the TBrush copy constructor or TBrush(HANDLE) with a handle contained in another TBrush object ... In the next development version I can try to implement better mechanism, like adding a new constructor to TBrush that allows you to construct objects for the system color brushes, and they will not be reference counted and deleted. Jogy |
From: Mark H. <mar...@bt...> - 2009-10-21 16:19:34
|
Hello Jogy It does look like the functionality of this gets quite complex. It's worth noting that there are actually two separate reference counting systems happening - one for the cache entry (TBrushCache::TEntry::RefCnt) and another for the actual GDI object (TGdiObject::TObjInfo::RefCount). The TBrush destructor would not actually decrease the reference count on the TGdiObject and so would not directly delete it. However, it does look like if TBrushCache::AddRef is called later, and the cache is full, then TGdiObject::RefDec may be called unconditionally for any object with a cache RefCnt of zero. This will obviously then delete the object. I think that a separate constructor for system color brushes would be a good idea rather than relying on the high byte in the color to determine it. Would disabling caching and reference counting for system color brushes, as a quick fix, cause any problems do you think? Mark ----- Original Message ----- From: "Jogy" <jo...@si...> To: "Mark Hatsell" <mar...@bt...> Cc: <owl...@li...> Sent: Tuesday, October 20, 2009 11:24 AM Subject: Re: [OwlNExt-users] Should TBrush be caching or ref counting System color brushes > Mark Hatsell wrote: >> Hi Jogy >> >> Thanks for the useful info regarding the brush caching functionality of >> OWL. >> I think that your solution would work fine. However, is it not achieving >> the >> same as simply not reference counting a system color TBrush? If RefAdd is >> never called for the TBrush it should never get deleted via DeleteObject. >> Of >> course I may be missing something here completely, such as the reference >> count being used for purposes other than deletion?? >> >> > Hello, Mark, > > I tried some solution with not adding a reference count, but there are > some problems, > > the destructors look like this: > > TBrush::~TBrush() > { > if(!ShouldDelete) > GetBrushCache().DecRef(Handle); > TRACEX(OwlGDI, OWL_CDLEVEL, _T("TBrush destructed @") << (void*)this); > } > > TGdiObject::~TGdiObject() > { > if (ShouldDelete) > RefDec(Handle, true); > TRACEX(OwlGDI, 2, "TGdiObject destructed " << *this); > } > > So, no matter what the value of ShouldDelete is, the reference is > decremented, > and there are internal checks and traces that rely on the reference > counting. > > I tried a solution, where I added an additional flag in the internal > class TBrushCache > to indicate that the handle is a system brush and should not be > reference counted, > and setting ShouldDelete to false. > > but this failed for example in this scenario: > // First create a system brush and put in the cache > TBrush brush(TColor::SysActiveCaption, true); > > // Second, create the same system brush, without using the cache > TBrush brush2(TColor::SysActiveCaption, false); > > because the returned brush handle is the same, the second object on > destruction > finds it in the brush cache and tries to decrement the reference, it > goes negative > and a check fails. I would have to add special handling for this case, > and there can be other weird scenarios, like when using the TBrush copy > constructor > or TBrush(HANDLE) with a handle contained in another TBrush object ... > > > In the next development version I can try to implement better mechanism, > like adding a > new constructor to TBrush that allows you to construct objects for > the system color brushes, and they will not be reference counted and > deleted. > > > Jogy > > > > ------------------------------------------------------------------------------ > Come build with us! The BlackBerry(R) Developer Conference in SF, CA > is the only developer event you need to attend this year. Jumpstart your > developing skills, take BlackBerry mobile applications to market and stay > ahead of the curve. Join us from November 9 - 12, 2009. Register now! > http://p.sf.net/sfu/devconference > _______________________________________________ > OwlNExt-users mailing list > Owl...@li... > https://lists.sourceforge.net/lists/listinfo/owlnext-users -------------------------------------------------------------------------------- No virus found in this incoming message. Checked by AVG - www.avg.com Version: 8.5.422 / Virus Database: 270.14.23/2447 - Release Date: 10/20/09 03:55:00 |
From: jogy <jo...@si...> - 2009-10-21 19:47:27
|
Mark Hatsell wrote: > Hello Jogy > > It does look like the functionality of this gets quite complex. It's worth > noting that there are actually two separate reference counting systems > happening - one for the cache entry (TBrushCache::TEntry::RefCnt) and > another for the actual GDI object (TGdiObject::TObjInfo::RefCount). > > That is true. Also, I don't like that the TGdiObject reference count methods are exposed as public. I think that they should be protected, it's not a job for users of the library to mess with the internal reference counting. > The TBrush destructor would not actually decrease the reference count on the > TGdiObject and so would not directly delete it. However, it does look like > if TBrushCache::AddRef is called later, and the cache is full, then > TGdiObject::RefDec may be called unconditionally for any object with a cache > RefCnt of zero. This will obviously then delete the object. > > I think that a separate constructor for system color brushes would be a good > idea rather than relying on the high byte in the color to determine it. > Would disabling caching and reference counting for system color brushes, as > a quick fix, cause any problems do you think? > > I tested again your version of the code, which does not cache and ref count the system brushes, with the addition that is always sets ShouldDelete = false for them. When the TBrush is destroyed, it looks in the brush cache for the handle, and if not found - nothing happens. The performance too is good - better than using non-cached regular color brushes, and similar to using cached regular color brushes. There can be two possible problems with this: 1. Calling the TBrush copy constructor leads to putting the handle of the second copy in the TGdiObject reference counter and then calling DeleteObject on it. 2. If there is some user code that relies on the GDI handles in the TGdiObject reference counter. So, to solve these problems in the next development version, I can add a flag in TBrush to indicate that this is a system brush, and the copy constructor will make use of this flag. This, together with the new constructor, will lead to even better performance when using the system brushes. (And they are used quite a bit in OWLNext drawing code here and there) And I will make TGdiObject reference count methods protected to indicate that they should not be used in user code. Jogy |
From: Jogy <jo...@si...> - 2009-10-20 10:40:42
|
Mark Hatsell wrote: > btw, did you see my other post regarding TCmdLine? > > http://sourceforge.net/mailarchive/message.php?msg_name=0A42D6352CD1481785915D388BA44A46%40Laptop3 > > Any chance that this can make it into the update? I realize that it's not a > perfect solution but at least it allows you to work with command line > options containing whitespace. > > Regards > Mark > I added the changes. Now, the parameter /"cC:\Program Files\test.txt" will be returned as one token of type TKind::Option Note that parameter like /c"C:\Program Files\test.txt" is returned in two tokens - the first one is 'c' of type TKind::Option, and the second one is "C:\Program Files\test.txt" of type TKind::Name Jogy P.S. In the next development version I will do some chnages in TCmdLine, at the very least I will hide the public members TKind Kind; // Kind of current token LPTSTR Token; // Ptr to current token. (Not 0-terminated, use TokenLen) int TokenLen; // Length of current token and will replace them with public methods like these: TKind CurrentTokenKind() const owl_string CurrentToken() |