IsmAvatar/LateralGM

Property Link Factory Memory Leak

RobertBColton opened this issue · 2 comments

So 49115dd caused unnecessary retention of the resource frames because the property link factory class is statically holding a strong reference to the factories of an owner which are holding strong references to their owner through the exception listener. That is a circular reference which causes the factories in the owned map to always be reachable. We can verify this by printing a message in resource frame's finalize. Also need to mention there is in fact what is arguably a bug in the Windows look and feel (probably the desktop manager) that causes the last closed MDI frame to not be garbage collected, and this issue is talked about on the internet.

I don't see this a big deal because at least people can edit properties without an exception after reopening the frames now. However, we should address this and there's a couple of ways we can do it.

  1. Move the owner handling of the property link factories directly into the resource frame, and possibly require creating a factory through the base resource frame.
  2. Change the factory owner map to use a second WeakHashMap for the value type instead of List so the cycle is broken.
  3. Change the factory to have only a WeakReference to its owner. This is my least favorite because the class still always holds strong static references to the factories, which I don't think is a good idea.
  4. Change the factory owner map to use the WeakArrayList class we have. There's issues with this though because that class's implementation has inconsistencies with the official WeakHashMap in the Java API. It directly extends an ArrayList with WeakReference as the value type, which requires direct use of WeakReferences in the list, which WeakHashMap does not.

Closing as resolved by c85f958 which makes it the responsibility of the owner to manage its own property link factories. All of the other solutions to this suck because they are just adding unnecessary indirection to the problem. The real original issue here was just the mutate in notification caused by the fact that our property links don't handle the bidirectionality well. Some of the property links attempt to by checking if the value is actually different, but it's not quite the same as removing the listener during the synchronization. One other solution we could try later and I didn't mention is to give the property link factory a link creation listener in addition to an exception listener. Then the resource frame could just listen for link creation and keep a list of all its property links. For now, the memory leak is gone and the exception is gone so the user won't notice this anymore.

Just to show what I mean that the issue is with DocumentLink still. If I go to game settings frame and bind two different text fields, with different documents, to the same map property, I can reproduce the original mutate in notification exception. That's not good because at some point it may be useful to bind more than one Swing component to a map property. None of the other links can reproduce this issue.

		plf.make(author.getDocument(),PGameSettings.AUTHOR);
		plf.make(version.getDocument(),PGameSettings.AUTHOR);

It is resolved if we remove the document listener temporarily while we go to update the component. That breaks the cycle of the of the one document updating the map, triggering the other document to update the map. I actually favor this more than any SwingUtilities.invokeLater solution as it doesn't really make sense to allow the cycle to exist.

		document.removeDocumentListener(this);
		editComponent(t);
		document.addDocumentListener(this);

It's also fixed if we stop listening to the map when we update the map. This is probably the best solution if we actually want to fix the bidirectionality because it's right at the origin of the initial edit which causes the cycle. It fixes the problem for all of the links too, even those which don't manifest the exception. We also do the same thing when we go to change the map in PropertyLink.setMap for the same reasons. Note that doing it this way immediately breaks property validation as the value validated by the model is the one committed and then the component doesn't know if it was changed after submission. So additional work would be required to make this way usable.

	protected void editProperty(Object v)
		{
		map.getUpdateSource(key).removeListener(this);
		try
			{
			map.put(key,v);
			}
		catch (RuntimeException re)
			{
			reset();
			if (exceptionListener != null)
				exceptionListener.exceptionThrown(re);
			else
				throw re;
			}
		finally
			{
			map.getUpdateSource(key).addListener(this);
			}
		}