Modify

Opened 7 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#19432 closed defect (fixed)

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

Reported by: skyper Owned by: GerdP
Priority: major Milestone: 20.07
Component: Core Version: latest
Keywords: template_report split way relation member, regression Cc: JeroenHoek

Description

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)

Attachments (3)

josm_split_way_exception_example.osm.bz2 (45.3 KB) - added by skyper 7 weeks ago.
example file
19432.patch (2.2 KB) - added by GerdP 4 weeks ago.
undo.PNG (9.3 KB) - added by GerdP 4 weeks ago.

Download all attachments as: .zip

Change History (20)

Changed 7 weeks ago by skyper

example file

comment:1 Changed 4 weeks ago by GerdP

Keywords: regression added
Owner: changed from team to GerdP
Status: newassigned

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.

Last edited 4 weeks ago by GerdP (previous) (diff)

Changed 4 weeks ago by GerdP

Attachment: 19432.patch added

comment:2 Changed 4 weeks ago by GerdP

Cc: JeroenHoek added; GerdP removed

@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.

Changed 4 weeks ago by GerdP

Attachment: undo.PNG added

comment:3 Changed 4 weeks ago by 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)


comment:4 in reply to:  3 Changed 4 weeks ago by 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.

comment:5 in reply to:  2 Changed 4 weeks ago by 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);

comment:6 Changed 4 weeks ago by 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).

comment:7 Changed 3 weeks ago by 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?

comment:8 Changed 3 weeks ago by 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.

comment:9 Changed 3 weeks ago by 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...

comment:10 Changed 3 weeks ago by 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.

comment:11 Changed 3 weeks ago by 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.

comment:12 Changed 3 weeks ago by 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.

comment:13 Changed 3 weeks ago by GerdP

Resolution: fixed
Status: assignedclosed

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

comment:14 Changed 3 weeks ago by 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.

comment:15 Changed 3 weeks ago by 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?

comment:16 Changed 3 weeks ago by GerdP

In 16782/josm:

see #19432: fix comment in unit test

comment:17 Changed 3 weeks ago by GerdP

Milestone: 20.07

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.