Hi, I've een looking in nsStringUtils.h and found a number of problems. A new version of the macros with some fixes is below. Please note that the only system i have is an NT machine with Visual C++ 6, so you might want to check if it doesn't break the builds on other configurations. 1. SCOPE OF STACK BUFFER When NS_ALLOC_CHAR_BUF uses the static buffer, the buffer is declared within the scope of the if statement. This is not a problem for DEBUG builds, but for optimized builds the buffer space will be reused for other variables. Fix: Move the allocation outside of the body of the if statement. 2. NEW[] AND DELETE[] When allocating a buffer with new[], it should be deallocated with delete[]. Same for NS_FREE_STR_BUF, the function nsString::ToNewCString returns a pointer to a buffer allocated with new[]. 3. TWO VERSIONS There are two similar versions of nsStringUtil.h on my tree: one in /widget/public and one in /widget/src/windows. I removed the latter and rebuild the files in widget, no errors whatsoever. 4. _ns_smallBufUsed not needed The variable _ns_smallBufUsed is not needed, to check if the buffer should be deallocated, just compare the value of aBuf with the address of the static buffer. 5. < and <= For NS_ALLOC_CHAR_BUF, the argument aActualSize should probably contain the number of bytes requested. The if-statement should check for <= instead of "aActualSize < _ns_kSmallBufferSize" 6. CONST-NESS Because the delete statement frees the buffer aBuf is pointing to, the user of the function should not change the location aBuf is pointing to, to prevent this aBuf can be declared as: char * const aBuf; 7. Two function calls In NS_ALLOC_CHAR_BUF the argument aActualSize will probably be a function call, since it is used twice, it makes sense of use a temporary variable to prevent the second call. NS_ALLOC_STR_BUF already does this (but why is that variable static?). 8. Merging I think it makes sense to call NS_ALLOC_CHAR_BUF from NS_ALLOC_STR_BUF. General comment: why not use alloca? --- #define NS_ALLOC_CHAR_BUF(aBuf, aSize, aActualSize) \ int _ns_tmpActualSize = aActualSize; \ char _ns_smallBuffer[aSize]; \ char * const aBuf = _ns_tmpActualSize <= aSize ? \ _ns_smallBuffer : new char[_ns_tmpActualSize]; #define NS_FREE_CHAR_BUF(aBuf) \ if (aBuf != _ns_smallBuffer) \ delete[] aBuf; #define NS_ALLOC_STR_BUF(varName, strName, tempSize) \ NS_ALLOC_CHAR_BUF(varName, tempSize, strName.Length()+1);\ strName.ToCString(varName, strName.Length()+1); #define NS_FREE_STR_BUF(varName) \ NS_FREE_CHAR_BUF(varName)

Setting all current Open/Normal to M4.

per leger, assigning QA contacts to all open bugs without QA contacts according to list at http://bugzilla.mozilla.org/describecomponents.cgi?product=Browser

I'm not sure if it's clear in the text, but point #1 means that stack space will be re-used for other variables, overwriting the contents of a buffer that could still be un use. This will break release builds. I suggest to at least move the char _ns_smallBuffer[_ns_kSmallBufferSize]; outside of the if statement ASAP as this might pop-up in strange and unexpected places.

Fixed in 5-10-99 4:00pm build. Removed widget/src/windows/nsStringUtil.h, nsStringUtil.cpp. widget/public now contains a nsStringUtil.h with the fixes described in this bug report. Moved GetAPCString from nsStringUtil.h to nsMenu since GetAPCString is WIN32 specific.

zee, can you verify these fixes? (and then mark bug as VERIFIED). Thanks.

Moving all Widget Set bugs, past and present, to new HTML Form Controls component per request from karnaze. Widget Set component will be retired shortly.