Bug in DOMStringHandle::operator delete() causes crashes

[Rick Byers <ri...@quack.com>]

[Sorry if this message gets duplicated - it didn't appear to get sent to
the list, presumably because my from address was different than the
address subscribed to the list]

Hi,
We're using Xerces-c 1.2 under Win32, and I believe I've finally tracked
down a bug that has been causing us serious reliability problems.

The comment in DOMStringHandle::operator delete() says:

If ALL of the string handles are gone, delete the storage blocks used
for the handles as well.  This will generally only happen on
PlatFormUtils::Terminate(), since any use of the DOM will cache some
commonly used DOMStrings forever (until Terminate).

This doesn't appear to be true.  It looks like most DOMStrings get
deleted when all instances of DOMParser are destroyed.  Its also
possibile to just create DOMStrings without using a DOMParser, in which
case there won't be ANY cached DOMStrings.  Also, the lazily allocated
constant strings like "#text" won't necessarily get created at all (as
is the case in our application).

We have an application that is constantly creating and destroying
DOMParsers in multiple threads.  Occasionally (after running anywhere
from 1 minute to 10 hours) it crashes due to an attempt to dereference a
NULL pointer in DOMStringHandle::operator new().  Since the code that
deletes the storage blocks is not gaurded with a mutex lock, its
possible for it to clash with code in DOMStringHandle::operator new(),
which could cause all sorts of problems - including dereferencing a NULL
pointer.  I extended the XMLMutexLock to cover the whole function (like
the comment says it does), and commented out the call to 
DOMStringTerminate.  We'll leave the application running tonight to 
verify that it solved the problem.

Whats the best solution to this problem?  In our case, it's probably
inefficient to be constantly allocating and deleting the DOMStringHandle
block.  However, its nice to be able to free the blocks incase you'll
never need all of them again.  Also, I think that atleast
DOMStringTerminate should be in XMLPlatformUtils::Terminate(), but I'm
not sure what else in Terminate() uses DOMStrings.  

Any thoughts on the best solution?  I expect that extended the MutexLock
will do the trick, but its probably less than ideal.  

Thanks,
	Rick

Re: Bug in DOMStringHandle::operator delete() causes crashes

[Rick Byers <ri...@quack.com>]

Andy Heninger wrote:
> 
> > I've traced through it and when all my DOMParseres gets destroyed,
> > gLiveStringHandleCount goes down to 0 and that terminate code DOES
> > DEFINANTLY get called.  Just setting a breakpoint in there makes it
> > obvious that the code isn't getting called only at terminate time as the
> > breakpoint is triggered over and over again.  Which strings specifically
> > "should remain until terminate() is called"?  I couldn't seem to find
> > any that were gaurenteed to be around.
> >
> 
> It sounds like you've checked this more carefully than me.  And that
> fixing things so that DOMString behaves as the documentation says ought to
> be done.
 
I see 2 different ways to fix this (DISCLAMER: I haven't had a chance to
look at the 1.3 code yet - so I'm assuming, as Andy says, that there
aren't any changes that affect this problem):

1) Extend the mutex lock in DOMStringHandle::operator delete to cover
the whole function.  Then move the call to DOMStringTerminate into
XMLPlatformUtils::Terminate (probably right before or right after the
call to XMLString::termString).  This has the advantage that if no
strings are being used, the memory for the string handle pool is
released.

2) Move all the code in the if(DOMString::gLiveStringHandleCount == 0)
block into DOMStringTerminate and then just call DOMString terminate
from XMLPlatformUtils::Terminate.  This has the advantage that if you're
constantly creating and destroying parsers, you won't keep allocating
and freeing the string handle pool block.

I think #2 would be better, thoughts?.  What are the dependancies on
DOMStrings?  Should DOMStringTerminate be called before or after
XMLString::termString?  Should we set some flag that says
DOMStringTerminate has been called which is checked by
DOMStringHandle::operator new to ensure we're not attempting to create
any strings after calling terminate just as a sanity check?

Rick

Re: Bug in DOMStringHandle::operator delete() causes crashes

[Andy Heninger <an...@jtcsv.com>]

----- Original Message -----
From: "Rick Byers" <ri...@quack.com> writes


> Andy Heninger wrote
> > Something else is probably going on here.  All but a small handful of
> > strings will be deleted when all parsers are destroyed, but some
> > should remain until terminate() is called.
>
> I've traced through it and when all my DOMParseres gets destroyed,
> gLiveStringHandleCount goes down to 0 and that terminate code DOES
> DEFINANTLY get called.  Just setting a breakpoint in there makes it
> obvious that the code isn't getting called only at terminate time as the
> breakpoint is triggered over and over again.  Which strings specifically
> "should remain until terminate() is called"?  I couldn't seem to find
> any that were gaurenteed to be around.
>

It sounds like you've checked this more carefully than me.  And that
fixing things so that DOMString behaves as the documentation says ought to
be done.

Andy Heninger
IBM XML Technology Group, Cupertino, CA
heninger@us.ibm.com

Re: Bug in DOMStringHandle::operator delete() causes crashes

[Rick Byers <ri...@quack.com>]

> > Hi,
> > We're using Xerces-c 1.2 under Win32, and I believe I've finally tracked
> > down a bug that has been causing us serious reliability problems.
> >
> > The comment in DOMStringHandle::operator delete() says:
> >
> > If ALL of the string handles are gone, delete the storage blocks used
> > for the handles as well.  This will generally only happen on
> > PlatFormUtils::Terminate(), since any use of the DOM will cache some
> > commonly used DOMStrings forever (until Terminate).
> >
> > This doesn't appear to be true.  It looks like most DOMStrings get
> > deleted when all instances of DOMParser are destroyed.
> 
> Something else is probably going on here.  All but a small handful of
> strings will be deleted when all parsers are destroyed, but some
> should remain until terminate() is called.

I've traced through it and when all my DOMParseres gets destroyed,
gLiveStringHandleCount goes down to 0 and that terminate code DOES
DEFINANTLY get called.  Just setting a breakpoint in there makes it
obvious that the code isn't getting called only at terminate time as the
breakpoint is triggered over and over again.  Which strings specifically
"should remain until terminate() is called"?  I couldn't seem to find
any that were gaurenteed to be around.

> As an experiment, how about having your program create a DOMString when
> it starts that will last forever.  A ugly approach, but it should get
> rid of any chance of deleting the DOMStringHandle storage, and let
> us find out whether there are any more multithreading problems lurking.

I'm quite sure that would also prevent the problem.  After making the
changes I described to Xerces, our application has now set a record for
uptime.  It's been running over 18 hours now under a simulated heavy
load, where it never lasted past 10 before.  I'd like to test the
hypothesis that leaving a string around would prevent the problem, but
we unfortunantly don't have the resources to let the machine run
overnight as an experiment (we're close to deployment and the dev team
is working like crazy).  

Regardless of whether it happens in practice or not, shouldn't the code
verify the assertion that "this will only get called at termination
time" or atleast do something so that it couldn't cause serious problems
if it was called at some other time?  Whats the disadvantage to just
moving that code into the Terminate() function (possibly by registering
lazily allocated data).  I didn't do this myself because I didn't know
if there were any interdependancy issues.

> Moving up to the latest code would be good also.  I don't believe that
> anything has changed in this exact area, but a lot of other bugs
> have been fixed.  The latest code is about to be blessed as the
> 1.3.0 stable release - the build for this is happening now.

Excellent.  We're in a feature freeze right now, so I'm not sure I can
get away with such a major change.  I will definantly upgrade as soon as
I can though.

Thanks,
	Rick

Re: Bug in DOMStringHandle::operator delete() causes crashes

[Andy Heninger <an...@jtcsv.com>]

----- Original Message -----
From: "Rick Byers" <ri...@quack.com>
To: "Xerces-C-Dev" <xe...@xml.apache.org>
Sent: Friday, September 29, 2000 9:05 AM
Subject: Bug in DOMStringHandle::operator delete() causes crashes


> Hi,
> We're using Xerces-c 1.2 under Win32, and I believe I've finally tracked
> down a bug that has been causing us serious reliability problems.
>
> The comment in DOMStringHandle::operator delete() says:
>
> If ALL of the string handles are gone, delete the storage blocks used
> for the handles as well.  This will generally only happen on
> PlatFormUtils::Terminate(), since any use of the DOM will cache some
> commonly used DOMStrings forever (until Terminate).
>
> This doesn't appear to be true.  It looks like most DOMStrings get
> deleted when all instances of DOMParser are destroyed.

Something else is probably going on here.  All but a small handful of
strings will be deleted when all parsers are destroyed, but some
should remain until terminate() is called.

>  Its also
> possibile to just create DOMStrings without using a DOMParser, in which
> case there won't be ANY cached DOMStrings.  Also, the lazily allocated
> constant strings like "#text" won't necessarily get created at all (as
> is the case in our application).

This may be true, if you are using DOMStrings without ever creating a
DOM document, either via the parser or programatically.  I haven't
tried it.

>
> We have an application that is constantly creating and destroying
> DOMParsers in multiple threads.  Occasionally (after running anywhere
> from 1 minute to 10 hours) it crashes due to an attempt to dereference a
> NULL pointer in DOMStringHandle::operator new().  Since the code that
> deletes the storage blocks is not gaurded with a mutex lock, its
> possible for it to clash with code in DOMStringHandle::operator new(),
> which could cause all sorts of problems - including dereferencing a NULL
> pointer.  I extended the XMLMutexLock to cover the whole function (like

> the comment says it does), and commented out the call to
> DOMStringTerminate.  We'll leave the application running tonight to
> verify that it solved the problem.
>
> Whats the best solution to this problem?  In our case, it's probably
> inefficient to be constantly allocating and deleting the DOMStringHandle
> block.  However, its nice to be able to free the blocks incase you'll
> never need all of them again.  Also, I think that atleast
> DOMStringTerminate should be in XMLPlatformUtils::Terminate(), but I'm
> not sure what else in Terminate() uses DOMStrings.
>
> Any thoughts on the best solution?  I expect that extended the MutexLock
> will do the trick, but its probably less than ideal.

As an experiment, how about having your program create a DOMString when
it starts that will last forever.  A ugly approach, but it should get
rid of any chance of deleting the DOMStringHandle storage, and let
us find out whether there are any more multithreading problems lurking.

Moving up to the latest code would be good also.  I don't believe that
anything has changed in this exact area, but a lot of other bugs
have been fixed.  The latest code is about to be blessed as the
1.3.0 stable release - the build for this is happening now.


Andy Heninger
IBM XML Technology Group, Cupertino, CA
heninger@us.ibm.com

Re: Bug in DOMStringHandle::operator delete() causes crashes

[Andy Heninger <an...@jtcsv.com>]

"Jon Smirl" <jo...@mediaone.net> wrote

> [...] I'd like to be able to set
> Xerces up with a per thread memory allocator and get rid of all of the
> locks.

Me too.  It's probably a per-DOM-Document storage allocator, which,
in use, becomes effectively a per-thread allocator.

> If you profile Xerces you'll see large amounts of time spent in the
> locking code, especially on an SMP box.  The DOM tree itself is not
thread
> safe, only the strings are.
>

I see the same thing.  Performance doesn't scale well at all on
SMP boxes with multiple threads.  The problem is more than just the
strings.  Individual allocations for each all of the nodes themselves
bangs on the C++ runtime's storage allocator hard, and it is
sychronized.

Andy Heninger
IBM XML Technology Group, Cupertino, CA
heninger@us.ibm.com

Re: Bug in DOMStringHandle::operator delete() causes crashes

[Jon Smirl <jo...@mediaone.net>]

I'm in the same environment where I'm using Xerces from multiple threads.
The threads never communicate with each other.  I'd like to be able to set
Xerces up with a per thread memory allocator and get rid of all of the
locks. If you profile Xerces you'll see large amounts of time spent in the
locking code, especially on an SMP box.  The DOM tree itself is not thread
safe, only the strings are.

Jon Smirl
jonsmirl@mediaone.net