There is a small gap in the SynchronizedWeakHashSet implementation. T…

[dryganets]

Summary

There is a small gap in the SynchronizedWeakHashSet implementation - the containsKey method of the WeakHashMap is modifying hence calling it during the iteration might cause ConcurrentModificationException. Added a command DO_IF_CONTAINS to safely handle this case.

Changelog

[Android] [Bugfix] - Should fix a ConcurrentModificationException in onResume.

Test Plan

It is hard to reproduce this problem as this is a race condition.
You rather need to have a good set of integration tests or check it in production.

[pull-bot]

Messages

📖

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message. CATEGORY may be: General iOS Android TYPE may be: Added, for new features. Changed, for changes in existing functionality. Deprecated, for soon-to-be removed features. Removed, for now removed features. Fixed, for any bug fixes. Security, in case of vulnerabilities. MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

Generated by 🚫 dangerJS against 2a2d4e3

[facebook-github-bot]

@fkgozali has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @dryganets in 62d3409.

^{When will my fix make it into a release? | Upcoming Releases}

[fkgozali]

Thanks for the fix! We'll update you if related crash still happens in the future. cc @cpojer

[fkgozali]

@dryganets - looks like the same crash is still happening:

9Caused by: java.util.ConcurrentModificationException
10java.util.WeakHashMap$HashIterator.nextEntry(WeakHashMap.java:789)
11java.util.WeakHashMap$KeyIterator.next(WeakHashMap.java:822)
12+com.facebook.react.bridge.SynchronizedWeakHashSet.iterate(SynchronizedWeakHashSet.java:63)
13+com.facebook.react.bridge.ReactContext.onHostResume(ReactContext.java:218)
14+com.facebook.react.ReactInstanceManager.moveToResumedLifecycleState(ReactInstanceManager.java:660)
15+com.facebook.react.ReactInstanceManager.onHostResume(ReactInstanceManager.java:579)
16+com.facebook.react.ReactInstanceManager.onHostResume(ReactInstanceManager.java:535) 

It's on the same line as before in SynchronizedWeakHashSet.java:

      for (T listener: mMap.keySet()) {

Thoughts on further corner cases to handle? cc @cpojer

[dryganets]

@fkgozali,
I don't see how this could happen now.
All the methods are synchronized.

The iterators are protected with the lock.
For the same thread ConcurrentModification, I had a boolean variable and it was protected with the same lock.