[PATCH] NPE in TagEditHelper

[taylor.smock]

I don't know how to reproduce this bug, especially since TagEditHelper gets information from OsmDataManager.getInstance().getInProgressSelection() which uses DataSet.getSelected() which will ignore deleted primitives and (presumably) null primitives.

The only way I can think of to fix the NPE is to add a check somewhere to filter out primitives that are null, deleted, or have no DataSet. In this case, it looks like we where trying to call .getDataSet() on a null object.

URL:https://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2019-05-20 02:27:26 +0200 (Mon, 20 May 2019)
Build-Date:2019-05-20 01:31:08
Revision:15101
Relative:URL: ^/trunk

Identification: JOSM/1.5 (15101 en) Mac OS X 10.14.4
OS Build number: Mac OS X 10.14.4 (18E226)
Memory Usage: 1063 MB / 1820 MB (392 MB allocated, but free)
Java version: 1.8.0_211-b12, Oracle Corporation, Java HotSpot(TM) 64-Bit Server VM
Screen: Display 188947522 1920x1080, Display 69981408 2048x1152
Maximum Screen Size: 2048x1152
VM arguments: [-Djava.security.policy=file:<java.home>/lib/security/javaws.policy, -DtrustProxy=true, -Djnlpx.home=<java.home>/bin, -Djava.security.manager, -Djnlpx.origFilenameArg=${HOME}/Library/Application Support/Oracle/Java/Deployment/cache/6.0/31/583aa85f-7f8234e1, -Djnlpx.remove=false, -Dsun.awt.warmup=true, -Djava.util.Arrays.useLegacyMergeSort=true, -Djnlpx.heapsize=NULL,2048m, -Dmacosx.jnlpx.dock.name=JOSM (development version), -Dmacosx.jnlpx.dock.icon=${HOME}/Library/Application Support/Oracle/Java/Deployment/cache/6.0/25/4c122699-477b07e1.icns, -Djnlp.application.href=https://josm.openstreetmap.de/download/josm-latest.jnlp , -Djnlpx.jvm="<java.home>/bin/java"]
Dataset consistency test: No problems found

Plugins:
+ Mapillary (1.5.18)
+ apache-commons (34908)
+ apache-http (34908)
+ auto_tools (73)
+ buildings_tools (34982)
+ ejml (34908)
+ geotools (34908)
+ highwayNameModification (1557754861)
+ imagery_offset_db (34908)
+ jaxb (34908)
+ jna (34908)
+ jts (34908)
+ markseen (13)
+ opendata (34997)
+ osm-obj-info (51)
+ reltoolbox (34977)
+ turnrestrictions (34977)
+ utilsplugin2 (34977)

Map paint styles:
+ https://raw.githubusercontent.com/KaartGroup/Kaart-Styles/master/Kaart-Styles.mapcss
+ https://josm.openstreetmap.de/josmfile?page=Styles/Lane_and_Road_Attributes&zip=1
+ https://raw.githubusercontent.com/KaartGroup/Kaart-Styles/master/Overlapping%20Ways.mapcss
- https://josm.openstreetmap.de/josmfile?page=Styles/Coloured_Streets&zip=1
- https://josm.openstreetmap.de/josmfile?page=Styles/Surface&zip=1

Validator rules:
+ https://raw.githubusercontent.com/KaartGroup/KaartValidator/master/kaart.validator.mapcss

Last errors/warnings:
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- W: java.io.IOException: Attribution is not loaded yet
- E: Error header: The node with the id 1689682814 has already been deleted
- E: Handled by bug report queue: java.lang.NullPointerException


=== REPORTED CRASH DATA ===
BugReportExceptionHandler#handleException:
No data collected.

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: AWT-EventQueue-2 (39) of javawsApplicationThreadGroup
java.lang.NullPointerException
	at org.openstreetmap.josm.command.ChangePropertyCommand.<init>(ChangePropertyCommand.java:108)
	at org.openstreetmap.josm.gui.dialogs.properties.TagEditHelper$EditTagDialog.performTagEdit(TagEditHelper.java:521)
	at org.openstreetmap.josm.gui.dialogs.properties.TagEditHelper.editTag(TagEditHelper.java:278)
	at org.openstreetmap.josm.gui.dialogs.properties.PropertiesDialog$EditAction.actionPerformed(PropertiesDialog.java:1127)
	at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1668)
	at javax.swing.JComponent.processKeyBinding(JComponent.java:2882)
	at javax.swing.KeyboardManager.fireBinding(KeyboardManager.java:307)
	at javax.swing.KeyboardManager.fireKeyboardAction(KeyboardManager.java:263)
	at javax.swing.JComponent.processKeyBindingsForAllComponents(JComponent.java:2974)
	at javax.swing.JComponent.processKeyBindings(JComponent.java:2966)
	at javax.swing.JComponent.processKeyEvent(JComponent.java:2845)
	at java.awt.Component.processEvent(Component.java:6316)
	at java.awt.Container.processEvent(Container.java:2239)
	at java.awt.Component.dispatchEventImpl(Component.java:4889)
	at java.awt.Container.dispatchEventImpl(Container.java:2297)
	at java.awt.Component.dispatchEvent(Component.java:4711)
	at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1954)
	at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:835)
	at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:1103)
	at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:974)
	at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:800)
	at java.awt.Component.dispatchEventImpl(Component.java:4760)
	at java.awt.Container.dispatchEventImpl(Container.java:2297)
	at java.awt.Window.dispatchEventImpl(Window.java:2746)
	at java.awt.Component.dispatchEvent(Component.java:4711)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:760)
	at java.awt.EventQueue.access$500(EventQueue.java:97)
	at java.awt.EventQueue$3.run(EventQueue.java:709)
	at java.awt.EventQueue$3.run(EventQueue.java:703)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:74)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:84)
	at java.awt.EventQueue$4.run(EventQueue.java:733)
	at java.awt.EventQueue$4.run(EventQueue.java:731)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:74)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:730)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:205)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:116)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:105)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:93)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:82)

[taylor.smock]

Modify the sel list such that there are no null objects, no deleted objects (duplicates some functionality), and no objects without a dataset.

[Don-vip]

We should rather try to detect the root cause instead of adding such workarounds.

[taylor.smock]

It kind of looks like a concurrent modification issue, based off of the fact that we get the currently selected elements from a user POV, and somewhere between that and actually modifying the tags the node gets deleted.

I suppose we could issue a DataSet.beginUpdate() either before we get the the selection or once we get the selection(probably through OsmDataManager.getInstance().getActiveDataSet().beginUpdate();, but we would need to save the dataset for a finally block so we can run DataSet.endUpdate(), kind of like in 17740_v2.patch (to be attached).

[taylor.smock]

Lock the dataset before we get the selection to avoid disappearing primitives (apparently, finally always executes, so that is the only endUpdate() needed)

[Don-vip]

In 15125/josm:

fix #17740 - Lock the dataset before we get the selection to avoid disappearing primitives (patch by taylor.smock)