PropertyLinkFactory Disposal #159

Open
JoshDreamland opened this Issue Oct 30, 2014 · 16 comments

Comments

Projects
None yet
3 participants
@JoshDreamland

Occurred while typing a room caption:

Operating System: Linux
Version: 3.13.0-37-generic
Architecture: amd64

Java Vendor: Oracle Corporation
Version: 1.7.0_65

Available processors (cores): 8
Free memory (bytes): 22801656
Maximum memory (bytes): 2792357888
Total memory available to JVM (bytes): 147324928

File system root: /
Total space (bytes): 176680636416
Free space (bytes): 30919041024
Usable space (bytes): 21920534528

Stack trace:
java.lang.IllegalStateException: Attempt to mutate in notification
    at javax.swing.text.AbstractDocument.writeLock(AbstractDocument.java:1338)
    at javax.swing.text.AbstractDocument.remove(AbstractDocument.java:585)
    at org.lateralgm.ui.swing.propertylink.DocumentLink.setComponent(DocumentLink.java:37)
    at org.lateralgm.ui.swing.propertylink.DocumentLink.setComponent(DocumentLink.java:1)
    at org.lateralgm.util.PropertyLink.editComponent(PropertyLink.java:66)
    at org.lateralgm.util.PropertyLink.reset(PropertyLink.java:54)
    at org.lateralgm.util.PropertyLink.editProperty(PropertyLink.java:77)
    at org.lateralgm.ui.swing.propertylink.DocumentLink.update(DocumentLink.java:72)
    at org.lateralgm.ui.swing.propertylink.DocumentLink.insertUpdate(DocumentLink.java:60)
    at javax.swing.text.AbstractDocument.fireInsertUpdate(AbstractDocument.java:202)
    at javax.swing.text.AbstractDocument.handleInsertString(AbstractDocument.java:749)
    at javax.swing.text.AbstractDocument.insertString(AbstractDocument.java:708)
    at javax.swing.text.PlainDocument.insertString(PlainDocument.java:130)
    at javax.swing.text.AbstractDocument.replace(AbstractDocument.java:670)
    at javax.swing.text.JTextComponent.replaceSelection(JTextComponent.java:1379)
    at javax.swing.text.DefaultEditorKit$DefaultKeyTypedAction.actionPerformed(DefaultEditorKit.java:884)
    at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1662)
    at javax.swing.JComponent.processKeyBinding(JComponent.java:2869)
    at javax.swing.JComponent.processKeyBindings(JComponent.java:2916)
    at javax.swing.JComponent.processKeyEvent(JComponent.java:2832)
    at java.awt.Component.processEvent(Component.java:6293)
    at java.awt.Container.processEvent(Container.java:2229)
    at java.awt.Component.dispatchEventImpl(Component.java:4872)
    at java.awt.Container.dispatchEventImpl(Container.java:2287)
    at java.awt.Component.dispatchEvent(Component.java:4698)
    at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1887)
    at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:762)
    at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:1027)
    at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:899)
    at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:727)
    at java.awt.Component.dispatchEventImpl(Component.java:4742)
    at java.awt.Container.dispatchEventImpl(Container.java:2287)
    at java.awt.Window.dispatchEventImpl(Window.java:2719)
    at java.awt.Component.dispatchEvent(Component.java:4698)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:735)
    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.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
    at java.awt.EventQueue$4.run(EventQueue.java:708)
    at java.awt.EventQueue$4.run(EventQueue.java:706)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:705)
    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)

This prevents the room caption from being saved.

I also have a probably-unrelated exception which occurred randomly while accidentally clicking (or right clicking?) somewhere on the screen (ostensibly in the room editor, as the trace suggests). My hand slipped, and I got this exception:

Operating System: Linux
Version: 3.13.0-37-generic
Architecture: amd64

Java Vendor: Oracle Corporation
Version: 1.7.0_65

Available processors (cores): 8
Free memory (bytes): 56829688
Maximum memory (bytes): 2792357888
Total memory available to JVM (bytes): 151519232

File system root: /
Total space (bytes): 176680636416
Free space (bytes): 30951612416
Usable space (bytes): 21953105920

Stack trace:
java.lang.NullPointerException
    at org.lateralgm.subframes.RoomFrame.showSelectedInstance(RoomFrame.java:2039)
    at org.lateralgm.subframes.RoomFrame.access$4(RoomFrame.java:2018)
    at org.lateralgm.subframes.RoomFrame$3.mouseClicked(RoomFrame.java:452)
    at java.awt.AWTEventMulticaster.mouseClicked(AWTEventMulticaster.java:270)
    at java.awt.Component.processMouseEvent(Component.java:6519)
    at javax.swing.JComponent.processMouseEvent(JComponent.java:3311)
    at java.awt.Component.processEvent(Component.java:6281)
    at java.awt.Container.processEvent(Container.java:2229)
    at java.awt.Component.dispatchEventImpl(Component.java:4872)
    at java.awt.Container.dispatchEventImpl(Container.java:2287)
    at java.awt.Component.dispatchEvent(Component.java:4698)
    at java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4832)
    at java.awt.LightweightDispatcher.processMouseEvent(Container.java:4501)
    at java.awt.LightweightDispatcher.dispatchEvent(Container.java:4422)
    at java.awt.Container.dispatchEventImpl(Container.java:2273)
    at java.awt.Window.dispatchEventImpl(Window.java:2719)
    at java.awt.Component.dispatchEvent(Component.java:4698)
    at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:735)
    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.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
    at java.awt.EventQueue$4.run(EventQueue.java:708)
    at java.awt.EventQueue$4.run(EventQueue.java:706)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
    at java.awt.EventQueue.dispatchEvent(EventQueue.java:705)
    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)

This exception happened before the caption exception, but I have no reason to believe it was the cause.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Oct 30, 2014

Collaborator

The first issue seems like a mutual exclusion issue where two things are trying to set the component of the text at the same time. I have not messed with the link factories except for adding type safety so this is likely not a regression.
http://stackoverflow.com/questions/10005954/java-attempt-to-mutate-in-notification

The second issue is a regression introduced in 1.8.5 and is resolved as of a941309
Steps to reproduce the second issue are as follows:

  1. Create a new project
  2. Create an object
  3. Create an instance of the object in the room
  4. Click on the instance in the list view
  5. Exception should occur on instances that do not have sprites
Collaborator

RobertBColton commented Oct 30, 2014

The first issue seems like a mutual exclusion issue where two things are trying to set the component of the text at the same time. I have not messed with the link factories except for adding type safety so this is likely not a regression.
http://stackoverflow.com/questions/10005954/java-attempt-to-mutate-in-notification

The second issue is a regression introduced in 1.8.5 and is resolved as of a941309
Steps to reproduce the second issue are as follows:

  1. Create a new project
  2. Create an object
  3. Create an instance of the object in the room
  4. Click on the instance in the list view
  5. Exception should occur on instances that do not have sprites
@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Oct 30, 2014

Collaborator

@IsmAvatar Can you possibly weigh in on the first issue and whether you've seen it before or not? Have you seen anything similar?

Collaborator

RobertBColton commented Oct 30, 2014

@IsmAvatar Can you possibly weigh in on the first issue and whether you've seen it before or not? Have you seen anything similar?

@IsmAvatar

This comment has been minimized.

Show comment
Hide comment
@IsmAvatar

IsmAvatar Oct 30, 2014

Owner

I've never seen the first one, but I always wondered if it would occur. It looks like it might be a race-condition type of thing where two threads are accessing a PropertyLink at the same time? How reproducible is it?

Owner

IsmAvatar commented Oct 30, 2014

I've never seen the first one, but I always wondered if it would occur. It looks like it might be a race-condition type of thing where two threads are accessing a PropertyLink at the same time? How reproducible is it?

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Oct 30, 2014

Collaborator

@IsmAvatar Absolutely no reproducibility, we've yet to figure out how to reproduce it, I tried several different things and got as much info out of Josh as I could. He said he was placing instances, then hit undo, then tried to type in the room caption. A race condition was my initial assumption as well, it looks like this is getting pumped by AWT.

Collaborator

RobertBColton commented Oct 30, 2014

@IsmAvatar Absolutely no reproducibility, we've yet to figure out how to reproduce it, I tried several different things and got as much info out of Josh as I could. He said he was placing instances, then hit undo, then tried to type in the room caption. A race condition was my initial assumption as well, it looks like this is getting pumped by AWT.

@IsmAvatar

This comment has been minimized.

Show comment
Hide comment
@IsmAvatar

IsmAvatar Oct 30, 2014

Owner

Ok, it's a race condition, then, caused by Swing trying to update a textfield at the same time as we're trying to write to it. The solution is to wrap our write attempt in a SwingUtilities.invokeLater() as it should be. The stack trace should hopefully help you find it.

Owner

IsmAvatar commented Oct 30, 2014

Ok, it's a race condition, then, caused by Swing trying to update a textfield at the same time as we're trying to write to it. The solution is to wrap our write attempt in a SwingUtilities.invokeLater() as it should be. The stack trace should hopefully help you find it.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@JoshDreamland @IsmAvatar I was finally able to reproduce this, but not on purpose, it happened to me in the game information frame. This is likely creeping up due to changes in the JDK. My exception log to me to the exact same file that Josh's did.
http://pastie.org/9690661
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/DocumentLink.java#L37

I attempted the following solutions which failed.
http://pastebin.com/vNVVj9kc

Well I figured it out, if we make that function do nothing everything basically works. The race condition is happening because setComponent is calling insertString which in turn calls setComponent and somewhere in between editComponent is involved in this love triangle as well.
@IsmAvatar So I am really not sure what to do this looks like a pretty nasty bug. You literally can't edit the Window Title for Game Info as of right now because of this bug.

Collaborator

RobertBColton commented Nov 2, 2014

@JoshDreamland @IsmAvatar I was finally able to reproduce this, but not on purpose, it happened to me in the game information frame. This is likely creeping up due to changes in the JDK. My exception log to me to the exact same file that Josh's did.
http://pastie.org/9690661
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/DocumentLink.java#L37

I attempted the following solutions which failed.
http://pastebin.com/vNVVj9kc

Well I figured it out, if we make that function do nothing everything basically works. The race condition is happening because setComponent is calling insertString which in turn calls setComponent and somewhere in between editComponent is involved in this love triangle as well.
@IsmAvatar So I am really not sure what to do this looks like a pretty nasty bug. You literally can't edit the Window Title for Game Info as of right now because of this bug.

@RobertBColton RobertBColton added the Error label Nov 2, 2014

@IsmAvatar

This comment has been minimized.

Show comment
Hide comment
@IsmAvatar

IsmAvatar Nov 2, 2014

Owner

So this is now 100% reproducible?

Owner

IsmAvatar commented Nov 2, 2014

So this is now 100% reproducible?

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@IsmAvatar Yes mam, I can confirm the regression was introduced in 1.8.2
http://enigma-dev.org/docs/Wiki/LateralGM:_Revisions

In whatever version you test, just go to game information and try to set the Window title, version 1.8.2 is the first version that begins throwing the exception for me, though in anything earlier than 1.8.5 you will have to run from a command line to see the output because I had not made wide use of the ErrorDialog until that version.

After going back and running the old versions from git-bash to see the output, I can confirm the issue was introduced in 1.8.2 which was the first release of LGM that I worked on. lgm16b4 does appear to have the problem, and the change between these two versions was that I moved the information settings into a new window. It was in e35a1ac that I moved game information settings to their own frame instead of a tab.

While that may be a separate issue entirely it still does not address the underlying issues in DocumentLink which also caused Josh's errors.

Edit: As for the game info frame it appears that makeSettings() is getting called twice when the frame is shown.

Collaborator

RobertBColton commented Nov 2, 2014

@IsmAvatar Yes mam, I can confirm the regression was introduced in 1.8.2
http://enigma-dev.org/docs/Wiki/LateralGM:_Revisions

In whatever version you test, just go to game information and try to set the Window title, version 1.8.2 is the first version that begins throwing the exception for me, though in anything earlier than 1.8.5 you will have to run from a command line to see the output because I had not made wide use of the ErrorDialog until that version.

After going back and running the old versions from git-bash to see the output, I can confirm the issue was introduced in 1.8.2 which was the first release of LGM that I worked on. lgm16b4 does appear to have the problem, and the change between these two versions was that I moved the information settings into a new window. It was in e35a1ac that I moved game information settings to their own frame instead of a tab.

While that may be a separate issue entirely it still does not address the underlying issues in DocumentLink which also caused Josh's errors.

Edit: As for the game info frame it appears that makeSettings() is getting called twice when the frame is shown.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@IsmAvatar @JoshDreamland I have fixed the exception for Game Information in 35e3f35, it was indeed a race condition. When the user pressed the button to show the information settings it called the constructor which called makeSettings() and then called makeSettings() a second time creating two property links and therefore a race condition.

So it seems as though something similar happened to Josh on the Room frame but I am not quite sure how, will require some more investigation.

Collaborator

RobertBColton commented Nov 2, 2014

@IsmAvatar @JoshDreamland I have fixed the exception for Game Information in 35e3f35, it was indeed a race condition. When the user pressed the button to show the information settings it called the constructor which called makeSettings() and then called makeSettings() a second time creating two property links and therefore a race condition.

So it seems as though something similar happened to Josh on the Room frame but I am not quite sure how, will require some more investigation.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@IsmAvatar I can now reproduce @JoshDreamland's room editor exception, follow the steps below.

  1. Create a new room
  2. Set the room caption
  3. Close saving changes, just the resource not the whole program
  4. Reopen the same room and set the room caption

Looks like the bug is the same as the Game Information one, only this time it just seems the property links are not being disposed. This was a huge problem I warned about a while ago that we were not properly disposing the property links on several resource frames. After additional testing this issue is a regression from as far back as lgm16b4, so this is a really really old unreported issue.

@IsmAvatar Even if I fix this exception this is a wide spread issue, perhaps since I added that map to PropertyLinkFactory so that it can contact the links it created and tell them when the properties map itself has changed, I could add another map and a dispose method or similar to remove all links it created and force all resource frames to call it in the default dispose method?
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/PropertyLinkFactory.java#L37

After adding a JOptionPane message to for instance FormattedLink I was able to confirm that just about all of the PropertyLinkFactory settings on the room frame are subject to the same bug as the room caption.

This is a very very pervasive bug caused by very very poor foresight.

Collaborator

RobertBColton commented Nov 2, 2014

@IsmAvatar I can now reproduce @JoshDreamland's room editor exception, follow the steps below.

  1. Create a new room
  2. Set the room caption
  3. Close saving changes, just the resource not the whole program
  4. Reopen the same room and set the room caption

Looks like the bug is the same as the Game Information one, only this time it just seems the property links are not being disposed. This was a huge problem I warned about a while ago that we were not properly disposing the property links on several resource frames. After additional testing this issue is a regression from as far back as lgm16b4, so this is a really really old unreported issue.

@IsmAvatar Even if I fix this exception this is a wide spread issue, perhaps since I added that map to PropertyLinkFactory so that it can contact the links it created and tell them when the properties map itself has changed, I could add another map and a dispose method or similar to remove all links it created and force all resource frames to call it in the default dispose method?
https://github.com/IsmAvatar/LateralGM/blob/master/org/lateralgm/ui/swing/propertylink/PropertyLinkFactory.java#L37

After adding a JOptionPane message to for instance FormattedLink I was able to confirm that just about all of the PropertyLinkFactory settings on the room frame are subject to the same bug as the room caption.

This is a very very pervasive bug caused by very very poor foresight.

@RobertBColton RobertBColton changed the title from Cannot set room caption to PropertyLinkFactory Disposal Nov 2, 2014

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@IsmAvatar I have pushed patch 8662754 for this, there was a slight bug in PropertyLink and I added a new map to PropertyLinkFactory for it to mass dispose all links it has created which is now automatically called when a ResourceFrame is disposed. I can confirm that this fixes Josh's specific issue with the Room caption and the rest of the property link race conditions where I could reproduce them. Please let me know what you think of my fix and if it is good we can push it out into a new release.

Collaborator

RobertBColton commented Nov 2, 2014

@IsmAvatar I have pushed patch 8662754 for this, there was a slight bug in PropertyLink and I added a new map to PropertyLinkFactory for it to mass dispose all links it has created which is now automatically called when a ResourceFrame is disposed. I can confirm that this fixes Josh's specific issue with the Room caption and the rest of the property link race conditions where I could reproduce them. Please let me know what you think of my fix and if it is good we can push it out into a new release.

@IsmAvatar

This comment has been minimized.

Show comment
Hide comment
@IsmAvatar

IsmAvatar Nov 2, 2014

Owner

Cool beans. You're being promoted to CSI ;-)

Owner

IsmAvatar commented Nov 2, 2014

Cool beans. You're being promoted to CSI ;-)

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@IsmAvatar I have made the new release with the patch applied and I am just waiting for someone or @JoshDreamland to confirm my fixes work and have resolved the issues reported here.
http://enigma-dev.org/forums/index.php?topic=2269.new#new

Collaborator

RobertBColton commented Nov 2, 2014

@IsmAvatar I have made the new release with the patch applied and I am just waiting for someone or @JoshDreamland to confirm my fixes work and have resolved the issues reported here.
http://enigma-dev.org/forums/index.php?topic=2269.new#new

@JoshDreamland

This comment has been minimized.

Show comment
Hide comment
@JoshDreamland

JoshDreamland Nov 2, 2014

Seems to be fixed, here.

Seems to be fixed, here.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 2, 2014

Collaborator

@IsmAvatar You have to remove listeners, that's just the way Java works. The GC can't collect this because the plf is still in use, and in fact we were manually removing them in some places of the Room Frame for instance controls and such, which was also not actually working because we were removing them the wrong way. If you would like me to rename the method to removeAllLinks or something that will not cause confusion with garbage collection I can do that.

I am just waiting to see if anybody else pops up with anymore exceptions by tomorrow before closing this.

Edit: I refactored the method in c24ff98 to avoid confusion with garbage collection.

Collaborator

RobertBColton commented Nov 2, 2014

@IsmAvatar You have to remove listeners, that's just the way Java works. The GC can't collect this because the plf is still in use, and in fact we were manually removing them in some places of the Room Frame for instance controls and such, which was also not actually working because we were removing them the wrong way. If you would like me to rename the method to removeAllLinks or something that will not cause confusion with garbage collection I can do that.

I am just waiting to see if anybody else pops up with anymore exceptions by tomorrow before closing this.

Edit: I refactored the method in c24ff98 to avoid confusion with garbage collection.

@RobertBColton

This comment has been minimized.

Show comment
Hide comment
@RobertBColton

RobertBColton Nov 4, 2014

Collaborator

@IsmAvatar This ticket will be automatically closed in 2014 if no new issues arise.

Collaborator

RobertBColton commented Nov 4, 2014

@IsmAvatar This ticket will be automatically closed in 2014 if no new issues arise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment