New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Random crash during using osmdroid #1260

Open
taiffu opened this Issue Jan 27, 2019 · 18 comments

Comments

Projects
None yet
5 participants
@taiffu
Copy link

taiffu commented Jan 27, 2019

I have used osmdroid of several years and users are reported me to weird random crashes. I have not find any googd reason why this happens until now I started used firebase. I'm unable to reproduce this by self error but it seems happens in varied devices randomly every week. I have now updated to version 6.0.3

Any Idea how to fix this?

Here is log form firebase:
Fatal Exception: java.lang.IndexOutOfBoundsException: Index: 63, Size: 62
at java.util.concurrent.CopyOnWriteArrayList.listIterator(CopyOnWriteArrayList.java:1040)
at org.osmdroid.views.overlay.DefaultOverlayManager$1.iterator(DefaultOverlayManager.java:92)
at org.osmdroid.views.overlay.DefaultOverlayManager.onShowPress(DefaultOverlayManager.java:323)
at org.osmdroid.views.MapView$MapViewGestureDetectorListener.onShowPress(MapView.java:1521)
at android.view.GestureDetector$GestureHandler.handleMessage(GestureDetector.java:289)
at android.os.Handler.dispatchMessage(Handler.java:107)
at android.os.Looper.loop(Looper.java:198)
at android.app.ActivityThread.main(ActivityThread.java:6729)
at java.lang.reflect.Method.invoke(Method.java)
at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)

@cbalster

This comment has been minimized.

Copy link
Contributor

cbalster commented Jan 28, 2019

final ListIterator<Overlay> i = mOverlayList.listIterator(mOverlayList.size());

Might a threading issue due to mOverlayList beeing changed between calls to .size() and listIterator().

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 28, 2019

@cbalster You're right, but it would look like a very bad luck error considering that the code of DefaultOverlayManager didn't change that much in the last 3 years, and that the issue seems to be a fresh one.

@taiffu Do you by any chance sometimes add Overlays asynchronously, which could happen when the user touches the screen (maybe a new feature you introduced in your app)?

The problem is that we assume we are thread-safe because mOverlayList is a CopyOnWriteArrayList. Unfortunately, only the iteration itself is safe, and this issue is apparently caused when we create the ListIterator object, not when we use it.
The solution is probably:

final ListIterator<Overlay> i;
synchronized (mOverlayList) {
	i = mOverlayList.listIterator(mOverlayList.size());
}

Trying to reproduce the issue in a unit test class...

@monsieurtanuki monsieurtanuki self-assigned this Jan 28, 2019

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 28, 2019

So far, I haven't managed to reproduce the issue in a unit test class with 2 Threads: one adding items, and the other looping on the reverse list.

@taiffu

This comment has been minimized.

Copy link
Author

taiffu commented Jan 28, 2019

I have here two assync functions where I add KLM (xml file) point to map when starting app. One alarm loop which checking in loop if some point is near you, it actual shows only you nearest points on map and remove old by distance. In this app I'm using also RadiusMarkerClusterer. I have to check if I find something from those methods to reproduce this case also by my self.

It may be possible that something weird happens in that nearest point checking loop.

Edit: Unable to reproduced this case in my device's, I will continue more hack my app...

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

spyhunter99 commented Jan 29, 2019

it's pretty clear that we are missing a synchronized keyword somewhere. You must have added/removed a layer from the collection during a draw cycle. should be an easy fix

@cbalster

This comment has been minimized.

Copy link
Contributor

cbalster commented Jan 29, 2019

Question: Why are the overlays iterated over in reverse in the first place?

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 29, 2019

@cbalster The overlays are used in both ways:

  • they are displayed forwards (the last overlay being on top)
  • they can be tested backwards (for onTouch events, as we want to test the latest displayed overlays first)
@cbalster

This comment has been minimized.

Copy link
Contributor

cbalster commented Jan 29, 2019

Ah, of course - thanks.

So far, I haven't managed to reproduce the issue in a unit test class with 2 Threads: one adding items, and the other looping on the reverse list.

An IndexOutOfBoundsException should only happen when removing items. Only then it's possible to pass an index to .listIterator() that's larger than the actual size at call-time.

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

spyhunter99 commented Jan 29, 2019

it's pretty clear that we are missing a synchronized keyword somewhere. You must have added/removed a layer from the collection during a draw cycle. should be an easy fix

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 30, 2019

@cbalster Thanks, you're right, removing items is the key for my tests.

@spyhunter99 Theoretically the solution is around the use of synchronized, but I'm working on another solution, assuming that

  • synchronized creates a lock, and that has a cost, performance-wise
  • the reverse-order is used very often
  • the current issue is rare

I'm building my unit tests: I'll test both the actual Exception issue and the performances.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 30, 2019

Quickly said: I don't even think that synchronized is the answer.
Anyway, my initial thought is not the answer, as I still have the Exception:

final ListIterator<Overlay> i;
synchronized (mOverlayList) {
	i = mOverlayList.listIterator(mOverlayList.size());
}

The problem is that we should add synchronized to the other pieces of code that access the list - more specifically when we remove an item. That would be ugly, not good performance-wise, and not even bullet proof, as we cannot guarantee that the developers won't remove items using DefaultOverlayManager's public List<Overlay> overlays() rather than using DefaultOverlayManager.remove.

In my unit test I embedded the item removal in a synchronized block (same for the reverse order block), and then the issue is not there anymore. But as explained before that can't be the solution.

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 30, 2019

I've just finished my tests:

  • I managed to reproduce the crash using the current implementation
  • my fix works
  • synchronizing both item removal and reverse order iterator creation works too, but is slightly slower (about 20% slower on a 5 test basis) and is not bullet proof (as we cannot expect developers to have synchronized item removal)

For the record, my fix is:

public ListIterator<T> reverseIterator() {
	while(true) {
		try {
			return mList.listIterator(mList.size())
		} catch(IndexOutOfBoundsException e) {
			//
		}
	}
}

About to PR...

monsieurtanuki added a commit that referenced this issue Jan 30, 2019

bug/#1260 - bullet-proof reverse order for DefaultOverlayManager
New class:
* `DefaultOverlayManagerTest`: unit test for `DefaultOverlayManager`

Impacted class:
* `DefaultOverlayManager`: wrote a more robust construction of the `ListIterator<Overlay>` in method `overlaysReversed`, using new method `bulletProofReverseListIterator`
@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 30, 2019

Fixed in #1261

monsieurtanuki added a commit that referenced this issue Jan 30, 2019

Merge pull request #1261 from osmdroid/bug/#1260
bug/#1260 - bullet-proof reverse order for DefaultOverlayManager
@taiffu

This comment has been minimized.

Copy link
Author

taiffu commented Jan 30, 2019

Woooh! I will update this to in my app. Thanks!

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Jan 30, 2019

@taiffu If you finally managed to reproduce the bug with your current version (when Overlays are being asynchronously removed), please test the new version before making it available to your users as there are other new features (potentially new bugs) between 6.0.3 and 6.1.0.

taiffu added a commit to taiffu/osmdroid that referenced this issue Jan 31, 2019

@spyhunter99

This comment has been minimized.

Copy link
Collaborator

spyhunter99 commented Feb 3, 2019

updating the snapshots now...

@monsieurtanuki

This comment has been minimized.

Copy link
Collaborator

monsieurtanuki commented Feb 9, 2019

@taiffu Any luck with the latest commit?

@samitai

This comment has been minimized.

Copy link

samitai commented Feb 10, 2019

@taiffu Any luck with the latest commit?

Unable to reproduce this case by self, so far I think this fix is good. But I can verify after 6.1.0 release is officialy out and I will take it in my production line with latest OSMBonus pack.

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