HUGE MEMORY LEAK pinpointed!

[rickg22]

Hey guys, I think I know why some of the plugins (like the TODO list and perhaps even Code Completion) waste too much memory!

Look!

Code Select Expand


bool cbRead(wxFile& file, wxString& st, wxFontEncoding encoding)
{
    st.Empty();
    if (!file.IsOpened())
        return false;
    int len = file.Length();
    if(!len)
    {
        file.Close();
        return true;
    }

    char* buff = new char[len+1];
    if (!buff)
    {
        file.Close();
        return false;
    }
    file.Read((void*)buff, len);
    file.Close();
    buff[len]='\0';

DetectEncodingAndConvert(buff, st, encoding);
// NOW WHERE'S DELETE BUFF?????????

    return true;
}


Notice that DetectEncodingAndConvert takes for parameter a const char*, so it means it doesn't do anything to buff. However, buff isn't released at the end. Hence, the leak.

I'll add a delete operator and see if nothing breaks.

[rickg22]

Update: It works!!! 

Memory consumption with the TODO plugin remains *steady* at 42MB!

I wonder how will code completion be affected with this... hmmm
Holy Cow...  :shock: 42MB was *WITH* code completion!!!! (No global includes tho, with them it's around 100mb). Without it's *JUST* 27.6MB! Woo hoo!!!!!!!

I'll commit ASAP.

(Hmm.......... code completion does leak. Everytime i parse, memory consumption is increased. But that'll be another adventure! )

[David Perfors]

wow, that is a nice catch great work rick

[MortenMacFly]

Wow... how ugly... must patch my C::B to have some testing... Well done, Rick!

[jpaterso]

Nicely picked up.
Recompiling my C::B now!

[rickg22]

Everybody update their SVN's!

Fix posted at revision 4120!

[jpaterso]

Should it be delete [] instead of delete?

[TDragon]

Should it be delete [] instead of delete?

YES.

[Jan van den Borst]

Typing in delete by hand is almost always bad practice. Use f.e. auto_ptr.

[byo]

Should it be delete [] instead of delete?

YES.

Yup, should be called. Fortunately in this particular case it will work with normal delete since there's no destructor called (binary code will probably look the same after switching to delete[]). So there's no need to panic .

Ok, now it's time to build the new c::B. Thanks rickg22 for the fix

BYO

[Grom]

Guys you have to use std::string. That will fix lot's of problems... Specially in that stupid example.

[Alturin]

Why aren't you using wxString everywhere anyway?
Either that or as Grom said, use std::string!

Also, maybe someone with the required knowledge could run C::B through valgrind or similar?

[byo]

Why aren't you using wxString everywhere anyway?
Either that or as Grom said, use std::string!

wxString CAN NOT be used here because it is used to BUILD proper wxString. And std::string can allocate much more memory than needed. And this code was put into "global functions" because it does some tricky / risky thing and is provided to all developers to prevent them from writing their own (risky/tricky) code. Only because such bug was found in critical function doesn't mean that it's common technique in other parts of code.

Also, maybe someone with the required knowledge could run C::B through valgrind or similar?

That would require some knowledge since wxWidgets would have to compiled with valgrind first. It would be much easier to use wxWidget's built-in memory leak search features.I remember that when using wx with Visual Studio, such things was turned on automatically and gave nice reports at the end of execution.

BYO

[TDragon]

It would be much easier to use wxWidget's built-in memory leak search features.

Just make sure you get rid of:

#undef new

[thomas]

Why aren't you using wxString everywhere anyway?

Well, we could use wxString for that, file.Read(wxStringBuffer(buff, len+1), len); does just that... but it only adds a lot of overhead.

Rather than using a function that allocates buffers and reads data from disk, then makes a copy only to discard it a millisecond later, it would be better to use the FileManager for loading, anyway. This would make use of editors that are already loaded into memory, remove two allocations and memory copies, offer better I/O balancing, and be able to read from URLs.

But for the meantime, this is a nice catch, Rick.