Bug: Concurrent Mod Bug on Act of Treason

[durp]

Description: AI Draw Phase after Act of Treason on Buffed (5/5) Zulaport Enforcer

On opponent's upkeep and return of Treasonous Buffed Zulaport Enforcer bad things happened

ConcurrentModificationException | Open

Code: Select all

Forge Version:    1.5.20-r26365Mu (mixed revisions detected; please update from the root directory)
Operating System: Mac OS X 10.9.3 x86_64
Java Version:     1.7.0_60 Oracle Corporation

java.util.ConcurrentModificationException
   at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:859)
   at java.util.ArrayList$Itr.next(ArrayList.java:831)
   at forge.game.card.Card.getHiddenExtrinsicKeyword(Card.java:4645)
   at forge.game.card.Card.getKeyword(Card.java:4352)
   at forge.game.card.Card.getAmountOfKeyword(Card.java:5215)
   at forge.game.card.Card.getNetAttack(Card.java:4048)
   at forge.view.arcane.CardPanel.setText(CardPanel.java:627)
   at forge.view.arcane.CardPanel.setCard(CardPanel.java:664)
   at forge.view.arcane.PlayArea.updateCard(PlayArea.java:699)
   at forge.view.arcane.PlayArea.updateSingleCard(PlayArea.java:359)
   at forge.screens.match.CMatchUI.updateSingleCard(CMatchUI.java:385)
   at forge.screens.match.CMatchUI.updateCards(CMatchUI.java:377)
   at forge.GuiDesktop.updateCards(GuiDesktop.java:376)
   at forge.control.FControlGameEventHandler$9.run(FControlGameEventHandler.java:225)
   at java.awt.event.InvocationEvent.dispatch(InvocationEvent.java:312)
   at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:733)
   at java.awt.EventQueue.access$200(EventQueue.java:103)
   at java.awt.EventQueue$3.run(EventQueue.java:694)
   at java.awt.EventQueue$3.run(EventQueue.java:692)
   at java.security.AccessController.doPrivileged(Native Method)
   at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
   at java.awt.EventQueue.dispatchEvent(EventQueue.java:703)
   at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
   at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
   at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
   at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
   at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
   at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)


Actually, the problem is pretty clear:

public final ArrayList<String> getHiddenExtrinsicKeyword() {
while (true) {
try {
final ArrayList<String> keywords = new ArrayList<String>();
for (String keyword : this.hiddenExtrinsicKeyword) {
if (keyword == null) {
continue;
}
if (keyword.startsWith("HIDDEN")) {
keyword = keyword.substring(7);
}
keywords.add(keyword);
}
return keywords;
} catch (IndexOutOfBoundsException ex) {
// Do nothing and let the while loop retry
}
}
}


----> Substring makes a new string, but we are assigning it back to to the iterator holder (keyword) effectively modifying the list while iterating (which is a NONO!)

Correct code:

public final ArrayList<String> getHiddenExtrinsicKeyword() {
while (true) {
try {
final ArrayList<String> keywords = new ArrayList<String>();
for (String keyword : this.hiddenExtrinsicKeyword) {
if (keyword == null) {
continue;
}
if (keyword.startsWith("HIDDEN")) {
keywords.add(keyword.substring(7));
} else {
keywords.add(keyword);
}
}
return keywords;
} catch (IndexOutOfBoundsException ex) {
// Do nothing and let the while loop retry
}
}
}



---> If I get a chance, I will add a test and the fix, but y'all should really think about moving from SVN to Git to make a casual pull like this a piece o'cake.

[timmermac]

We used git for some time, but there were problems with it, so we switched back to SVN. If the devs like the patch they may talk to you about granting commit privileges.

[elcnesh]

Really, this is what causes the annoying concurrent modification error? Wow, well spotted

For good measure, I'd suggest always declaring iterator variables 'final', to prevent this from happening.

[Fizanko]

The infamous omnipresent concurrent error may finally get solved ?
Considering how often the bug happens, that sounds really awesome !

[KrazyTheFox]

Anyone mind if I go commit this? I'm leaving on vacation for California early tomorrow morning for two weeks and I'd like this fix on my phone before I go.

[friarsol]

Anyone mind if I go commit this? I'm leaving on vacation for California early tomorrow morning for two weeks and I'd like this fix on my phone before I go.

Is it actually the fix? If that were the case wouldn't Act of Treason crash every time it was cast? I think it has more to do with the keywords getting cleared on a different thread and this one trying to access the same array.

[KrazyTheFox]

Anyone mind if I go commit this? I'm leaving on vacation for California early tomorrow morning for two weeks and I'd like this fix on my phone before I go.

Is it actually the fix? If that were the case wouldn't Act of Treason crash every time it was cast? I think it has more to do with the keywords getting cleared on a different thread and this one trying to access the same array.

Ah, I didn't look at it too much yet, but now that I've actually examined it, it is not the fix.

The problem here is that the code for this particular foreach loop is essentially the following:

Code: Select all

for (Iterator<String> iterator = this.hiddenExtrinsicKeyword.iterator(); iterator.hasNext();) {
   String keyword = iterator.next();
   if (keyword.startsWith("HIDDEN")) {
      keyword = keyword.substring(7);
   }
   keywords.add(keyword);
}


In this case, you're only changing the local variable and not the one in the list. Thus, you're doing exactly the same thing, just in a different way. Sadly, the real fix for this problem is not the one posted above.

[friarsol]

Right, you can do things to the objects inside the Array, you just can't modify the Array itself.

If we change this foreach loop to a normal forloop with a .get(i), wrap the whole thing in a try/catch. We'd probably stop getting it for this scenario, but I don't know what other negative effects that would have. Ideally, this function wouldn't be called at all when the static effects are actually modifying the keywords array.

[drdev]

I just posted this in the Developer Bug Reports thread, but I think the fix for the concurrent bug is actually the same as the fix for the P/T flicker bug which I've already fixed for the mobile game.

The problem here is calling card.getNetAttack() from paint code. If we instead cache off the value of card.getNetAttack() and other similar card properties from a GameEventCardStatChanged event handler (using GuiDesktop.refreshCardDetails which I just added), we won't have to call it from paint code and these concurrent modification bugs should go away.

I'll look into fixing this tonight.

[Fizanko]

Thanks to all of you developers that are trying to make Forge better and more solid !

[durp]

Ah, yes. You are right. Been so long since I have written java. Rusty I am. I think I will hook to the repo and see if I can *actually* find and fix a bug this time

In any case, I am super-impressed by the quick response.