Mem leak with target chooser when filtering inside of Game Shop by Mana Ability

[GoogleCodeExporter]

Filter cards by Mana Ability inside of Game Shop and quit game.  Notice that 
VS2008 will issue out a Mem leak problem with the creation of a 
DescriptorTargetChooser

This has been a part of the code for at least a month, perhaps even longer.  I 
found this in the course of my investigation of issue 601.  My earliest 
checkout of the code was dated early February and this memory leak existed 
then.  I have not tried any earlier than that.

Original issue reported on code.google.com by techdragon.nguyen@gmail.com on 1 Mar 2011 at 1:06

[GoogleCodeExporter]

this issue is related to filters...when you filter by mana ability it bools a 
match...
in the bool function it runs a parseManaCost on the lines it is trying to 
match...
        ManaCost * mc = ManaCost::parseManaCost(s);
if the mana cost is not a colored mana cost while parsing...it creates a 
targetchooser...line 77 manacost.cpp....

my guess is something in related to mana cost is sending false returns to the 
filter, claiming its a manaproducer when it really isnt....

Original comment by omegabla...@gmail.com on 1 Mar 2011 at 12:09

[GoogleCodeExporter]

This could be true.  From recollection the targetchooser is created before any 
other evaluation is done.  There are conditions where nothing is done with the 
tc and the method returns without cleaning it up.  Thus, a memory leak.  
However, putting a general statement to delete the tc at the end of the method 
seems to break things as well.  Ultimately, I believe it lies in how objects 
references are tracked in object creation.  So when an object gets deleted any 
references pointing to that object do not get updated appropriately, which is 
why the  deletion I put in didn't work.  Without doing an overhaul of all the 
objects in Wagic, there should be a remedy for this.

Original comment by techdragon.nguyen@gmail.com on 1 Mar 2011 at 4:46

[GoogleCodeExporter]

Found the leak.  Apparently "Honor-Worn Shaku" has a defnition TargetChooser
does not like.
auto={T(*[legendary,-tapped]|mybattlefield)}:untap

Everytime the parser tries to read in this card for a mana ability filter
the TC comes back as NULL b/c of an error inside of the
TargetChooserFactory.

I'm pretty sure this is the culprit as most calls to a factory should return
a non-NULL value otherwise an exception should be thrown or something else
should be done.

tc = tcf.createTargetChooser(target, c);

This should return a valid tc.  The number of mem leaks created is always
equal to the number of failed TargetChooser attempts in my tests.  So it's
one of two things:

1) the card code is incorrect and needs a slight modification
2) the factory needs to be updated to handle cases like this so it doesn't
die in the creation of the TargetChooser.

The first one needs to be verified before we can fix the 2nd as card coding
errors are quicker to fix than trying to figure out if the factory is
correct.

--Mike

Original comment by techdragon.nguyen@gmail.com on 1 Mar 2011 at 5:50

[GoogleCodeExporter]

about Honor-Worn Shaku, you can remove the -tapped...this is not needed as tap 
target as cost will only let you tap an untapped target....this might be the 
correction for that issue...

Original comment by omegabla...@gmail.com on 2 Mar 2011 at 1:54

[GoogleCodeExporter]

i actually updated the primitives to remove the -tapped check on all tap target 
cost...let me know if it corrected the leak....

Original comment by omegabla...@gmail.com on 2 Mar 2011 at 2:04

[GoogleCodeExporter]

It didn't fix the leak but rather averted it.  if "-tapped" is a legit trait, 
then this implies that any use of "-tapped" in this way will cause the leak 
again. By removing this trait from the only card the uses "-tapped" the leak no 
longer occurs.  What happens if another card uses it?

Original comment by techdragon.nguyen@gmail.com on 12 Mar 2011 at 4:50

[GoogleCodeExporter]

in a cost? not sure any other cards would....infact i think i can say for 
certain.

Original comment by omegabla...@gmail.com on 20 Mar 2011 at 1:24

[GoogleCodeExporter]

So the one card this affected is the only card in all of magic?  

Original comment by techdragon.nguyen@gmail.com on 21 Mar 2011 at 2:10

[GoogleCodeExporter]

Reread the thread. For future reference  we can not have tapped as part of the 
mana cost else we will see this bug again

Original comment by techdragon.nguyen@gmail.com on 21 Mar 2011 at 2:12

[GoogleCodeExporter]

not quite mike, here is where the leak was happening

[legendary,-tapped] <-----
notice it contains 2 qualities...
if you used [-tapped] it would work fine, however for some reason adding the 
2nd (which can be anything [green,-tapped] for exsample) would cause it....
the need to -tapped in a cost was no longer needed...as i corrected the canpay 
of "tap target" to automatically check for tapped status....

Original comment by omegabla...@gmail.com on 21 Mar 2011 at 1:28

[GoogleCodeExporter]

Ok let's close this then

Original comment by techdragon.nguyen@gmail.com on 21 Mar 2011 at 3:32

[GoogleCodeExporter]

closed for now. if in the future we come across this again. we will need to 
look deeper in to the filters and how its parsing the abilities.

Original comment by omegabla...@gmail.com on 22 Mar 2011 at 2:44

• Changed state: Fixed