Split way: AIOOB: Problem with member check with duplicate members

[skyper]

What steps will reproduce the problem?

1. Have some new route relations, see josm_split_way_exception_example.osm.bz2​
2. Select a middle node of way with membership (1523436358)
3. Split way

What is the expected result?

Either the dialog asking to download members comes up or the way is splitted

What happens instead?

AIOOB

Please provide any additional information below. Attach a screenshot if possible.

The problem seems to be a relation where the way is multiple times present in the relation.
If I choose to give the long part (5 nodes) a new id it works but this is only an option in expert mode.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-06-23 23:38:54 +0200 (Tue, 23 Jun 2020)
Revision:16713
Build-Date:2020-06-24 01:30:49
URL:https://josm.openstreetmap.de/svn/trunk

- E: Handled by bug report queue: java.lang.ArrayIndexOutOfBoundsException: Index 24 out of bounds for length 23


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

Warning issued by: BugReportExceptionHandler#handleException

=== STACK TRACE ===
Thread: AWT-EventQueue-0 (18) of main
java.lang.ArrayIndexOutOfBoundsException: Index 24 out of bounds for length 23
	at org.openstreetmap.josm.data.osm.Relation.getMember(Relation.java:74)
	at org.openstreetmap.josm.command.SplitWayCommand.doSplitWay(SplitWayCommand.java:373)
	at org.openstreetmap.josm.actions.SplitWayAction.doSplitWay(SplitWayAction.java:291)
	at org.openstreetmap.josm.actions.SplitWayAction$SegmentToKeepSelectionDialog.buttonAction(SplitWayAction.java:233)
	at org.openstreetmap.josm.gui.ExtendedDialog$1.actionPerformed(ExtendedDialog.java:378)
	at java.desktop/javax.swing.AbstractButton.fireActionPerformed(AbstractButton.java:1967)
	at java.desktop/javax.swing.AbstractButton$Handler.actionPerformed(AbstractButton.java:2308)
	at java.desktop/javax.swing.DefaultButtonModel.fireActionPerformed(DefaultButtonModel.java:405)
	at java.desktop/javax.swing.DefaultButtonModel.setPressed(DefaultButtonModel.java:262)
	at java.desktop/javax.swing.plaf.basic.BasicButtonListener.mouseReleased(BasicButtonListener.java:279)
	at java.desktop/java.awt.Component.processMouseEvent(Component.java:6631)
	at java.desktop/javax.swing.JComponent.processMouseEvent(JComponent.java:3342)
	at java.desktop/java.awt.Component.processEvent(Component.java:6396)
	at java.desktop/java.awt.Container.processEvent(Container.java:2263)
	at java.desktop/java.awt.Component.dispatchEventImpl(Component.java:5007)
	at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2321)
	at java.desktop/java.awt.Component.dispatchEvent(Component.java:4839)
	at java.desktop/java.awt.LightweightDispatcher.retargetMouseEvent(Container.java:4918)
	at java.desktop/java.awt.LightweightDispatcher.processMouseEvent(Container.java:4547)
	at java.desktop/java.awt.LightweightDispatcher.dispatchEvent(Container.java:4488)
	at java.desktop/java.awt.Container.dispatchEventImpl(Container.java:2307)
	at java.desktop/java.awt.Window.dispatchEventImpl(Window.java:2772)
	at java.desktop/java.awt.Component.dispatchEvent(Component.java:4839)
	at java.desktop/java.awt.EventQueue.dispatchEventImpl(EventQueue.java:772)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:721)
	at java.desktop/java.awt.EventQueue$4.run(EventQueue.java:715)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:95)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:745)
	at java.desktop/java.awt.EventQueue$5.run(EventQueue.java:743)
	at java.base/java.security.AccessController.doPrivileged(Native Method)
	at java.base/java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege(ProtectionDomain.java:85)
	at java.desktop/java.awt.EventQueue.dispatchEvent(EventQueue.java:742)
	at java.desktop/java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:203)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:124)
	at java.desktop/java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:113)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:109)
	at java.desktop/java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:101)
	at java.desktop/java.awt.EventDispatchThread.run(EventDispatchThread.java:90)

[skyper]

example file

[GerdP]

regression of r15943, error occurs when the way member appears multiple times in one relation.
Either it produces the crash (if member is also last member in relation) or it probably doesn't do what it should.
It's a bit difficult to understand what actually happens because the data in the sample file is outdated and thus the download of missing members updates the data.

[GerdP]

@JeroenHoek: Please review my quick patch. Do you think that it catches all problem cases? Storing positions of members in lists which are updated while these values are used is complex and quite dangerous.

[GerdP]

reg. updated member data: I think the split command should not proceed when the downloaded members are newer. During my experiements this lead to strange effects when using undo:
(Way 819163230 is missing here, it was added to the relation after the sample data was attached)

[skyper]

Replying to GerdP:

regression of r15943, error occurs when the way member appears multiple times in one relation.
Either it produces the crash (if member is also last member in relation) or it probably doesn't do what it should.
It's a bit difficult to understand what actually happens because the data in the sample file is outdated and thus the download of missing members updates the data.

Sorry, I continued and uploaded my changes. I try to remember and hopefully will add another layer with the completely download relation, next time.

This is related to the problem that not both members are taken into account, see #19217 and #19288. In the end, even #18018 is close to #19217.

Replying to GerdP:

reg. updated member data: I think the split command should not proceed when the downloaded members are newer. During my experiments this led to strange effects when using undo:
(Way 819163230 is missing here, it was added to the relation after the sample data was attached)

+1, an update of the relation (without downloading members) and the way to split is needed. Maybe, this could be offered similar to downloading needed members in order to not block completely.

[JeroenHoek]

Replying to GerdP:

@JeroenHoek: Please review my quick patch. Do you think that it catches all problem cases? Storing positions of members in lists which are updated while these values are used is complex and quite dangerous.

I'm not sure. At a quick glance it looks like a valid attempt at preventing this bug. Do the unit tests still work with it?

The ArrayIndexOutOfBoundsException shouldn't happen even with bogus data of course, and can be prevented by changing line 371. I would recommend modifying the == to >=:

RelationMember rmNext = position >= membersCount - 1
                    ? relation.getMember(0)
                    : relation.getMember(position + 1);

[GerdP]

It would prevent the exception but it would still get the wrong member when position is of by 1 or more (depending on the number of selected nodes).

[GerdP]

unit tests still work.

The ArrayIndexOutOfBoundsException shouldn't happen even with bogus data of course, and can be prevented by changing line 371. I would recommend modifying the == to >=:

I probably got you wrong. You did not suggest this as a fix for the ticket, just a general improvement for stability?

[JeroenHoek]

I probably got you wrong. You did not suggest this as a fix for the ticket, just a general improvement for stability?

Yes, it's just a basic sanity check to prevent that crash.

[GerdP]

Yes, but it helped to find the bug. I'd prefer to have a data structure that doesn't allow this bug in the first place.
I'll have a 2nd look at the code...

[GerdP]

There must be another problem with the calculation of missing ways. Either the sample data is corrupt or something is wrong in the code as it calculates the split-way as an incomplete member.

[JeroenHoek]

That's why it's OK to prevent the ArrayIndexOutOfBoundsException there. Bogus data exists; it may fail the operation, but it should never cause a crash.

I would start with adding a regression test in SplitWayCommandTest.java that captures this bug.

I've had a look to see if I could reproduce this, but I got stuck fighting SVN. I'll have to download a fresh copy and waste time on setting up the IDE again before I can contribute.

[GerdP]

Don't worry. I already found a fix for that. I've also added a sanity check in those routines which use the member position.

[GerdP]

In 16781/josm:

fix #19432: Split way: AIOOB: Problem with member check with duplicate members

• don't store position of member, instead always search it
• improve calculation of missing(incomplete) way members and store the result instead of calculating it twice
• add unit test

[GerdP]

The change relies on the fact that the copy of the relation refers to the identical members. I've added a sanity check which will fail if this changes.
I hope I did not break a case which is not tested in the unit tests.
I've not added code to handle the case that the split-way is updated with a newer version while the split is executed as this should not happen anymore.

[JeroenHoek]

I hope I did not break a case which is not tested in the unit tests.

I've tried to cover the code I wrote as completely as possible with regression tests, so anything you may have broken wasn't known to us in any case.

In the unit test you've left in a comment from the one above, but changed the SplitWayCommand.WhenRelationOrderUncertain enum:

    // This split requires no additional downloads. If any are needed, this command will fail.
    SplitWayCommand.WhenRelationOrderUncertain.SPLIT_ANYWAY

Are you sure you want SPLIT_ANYWAY instead of ABORT?

[GerdP]

In 16782/josm:

see #19432: fix comment in unit test