[PATCH] memleak in CJSObject::AddProperty

[Jan Wassenberg]

How to reproduce: build in debug mode, run, click editor button on main menu, exit editor via [x] button.

The following allocations leak:

{2367220} normal block at 0x00BA9690, 20 bytes long.
 Data: <   ]        N   > 14 DC BE 5D 01 00 00 00 13 02 00 00 4E 00 00 00 
{839470} normal block at 0x0A522160, 52 bytes long.
 Data: <            d a > 01 00 00 00 04 00 00 00 13 00 00 00 64 00 61 00 
[...]	
{792147} normal block at 0x0A542BD0, 8 bytes long.
 Data: <        > FF FF FF FF FF FF FF FF 
{3972} normal block at 0x00AF9AD0, 12 bytes long.
 Data: <  U         > 20 E7 55 01 0A 0E D4 00 ED 81 D2 00 
{3966} normal block at 0x00AF9E00, 8 bytes long.
 Data: <  Q ,   > 20 93 51 01 2C 04 00 00 
{3960} normal block at 0x00AF9CA8, 12 bytes long.
 Data: <  U         > 20 E7 55 01 14 1A D2 00 AE ED D2 00 
{3954} normal block at 0x00AF9B40, 12 bytes long.
 Data: <  U ,`  t2  > 20 E7 55 01 2C 60 D3 00 74 32 D2 00 
{3949} normal block at 0x00AF9320, 12 bytes long.
 Data: <  U  9   U  > 20 E7 55 01 D4 39 D4 00 0B 55 D2 00 
{3944} normal block at 0x00AF98B8, 8 bytes long.
 Data: <  Q "   > 20 93 51 01 22 04 00 00 
{3938} normal block at 0x00AF9760, 8 bytes long.
 Data: <  U (   > B4 E7 55 01 28 04 00 00 
{3932} normal block at 0x00AF9608, 8 bytes long.
 Data: <  U $   > 94 E7 55 01 24 04 00 00 
{3926} normal block at 0x00AF94B0, 8 bytes long.
 Data: <  Q !   > 20 93 51 01 21 04 00 00 
{3920} normal block at 0x00AE31B8, 8 bytes long.
 Data: <  Q     > 20 93 51 01 20 04 00 00 
{3914} normal block at 0x00AF91C8, 12 bytes long.
 Data: <  U  W  (j  > 20 E7 55 01 FD 57 D3 00 28 6A D2 00 
{3907} normal block at 0x00AF62C8, 12 bytes long.
 Data: <  U         > 20 E7 55 01 A6 EB D2 00 E6 0A D3 00 
{3896} normal block at 0x00AE3328, 8 bytes long.
 Data: <  Q     > 20 93 51 01 E6 03 00 00 
{3890} normal block at 0x00C27280, 8 bytes long.
 Data: <  Q     > 20 93 51 01 E4 03 00 00 
{3884} normal block at 0x00AE0DA0, 8 bytes long.
 Data: <  Q     > 20 93 51 01 E5 03 00 00 

3884 has the following call stack:

	msvcr100d.dll!operator new(unsigned int size=8)  Line 59 + 0x9 bytes	C++
	pyrogenesis_dbg.exe!CJSObject<CGameViewImpl,0>::AddProperty<bool>(const CStrW & PropertyName={...}, bool * Native=0x000003e5, bool PropReadOnly=false)  Line 340 + 0x7 bytes	C++
	pyrogenesis_dbg.exe!CGameViewImpl::ScriptingInit()  Line 387 + 0x4a bytes	C++
	pyrogenesis_dbg.exe!CGameView::ScriptingInit()  Line 367	C++
	pyrogenesis_dbg.exe!RegisterJavascriptInterfaces()  Line 310	C++
	pyrogenesis_dbg.exe!InitScripting()  Line 334	C++
	pyrogenesis_dbg.exe!Init(const CmdLineArgs & args={...}, int __formal=0)  Line 815	C++
	pyrogenesis_dbg.exe!RunGameOrAtlas(int argc=1, const char * * argv=0x00c25db8)  Line 488 + 0xb bytes	C++
	pyrogenesis_dbg.exe!main(int argc=1, char * * argv=0x00c25db8)  Line 512 + 0xd bytes	C++
	pyrogenesis_dbg.exe!wmain(int argc=1, wchar_t * * argv=0x00c25210)  Line 373 + 0x14 bytes	C++
	pyrogenesis_dbg.exe!__tmainCRTStartup()  Line 552 + 0x19 bytes	C
	pyrogenesis_dbg.exe!wmainCRTStartup()  Line 371	C
	pyrogenesis_dbg.exe!CallStartupWithinTryBlock()  Line 385 + 0x5 bytes	C++
	pyrogenesis_dbg.exe!wseh_EntryPoint()  Line 413	C++

[Markus]

AddProperty is overwriting a pointer: source/scripting/ScriptableObject.h line 323 and 342

The same PropertyName is assigned twice, resulting in a memory leak.

I think the right solution would be to delete the old property and replace with the new one. Any opinions?

[Markus]

Delete already existing property before adding new one.

[historic_bruno]

According to IRC logs, CJSObject may be going away with the next Spidermonkey upgrade, so I'm not sure this needs to be solved, unless we're sure the patch doesn't cause problems.

[historic_bruno]

Alternate solution that calls ScriptingShutdown explicitly

[historic_bruno]

On second thought, that patch still isn't a complete solution and rather than shutting all those script interfaces down properly, I'm going to close this and we'll consider CJSObject deprecated. IIRC, Yves and stwf have been working on removing them all and it won't even require the SM upgrade.