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

Fix DynamicFromMap object pool synchronization #17842

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@haitaoli
Contributor

haitaoli commented Feb 2, 2018

Motivation

DynamicFromMap internally uses SimplePool object to recycle dynamic prop objects. But the pool is not multi-thread safe. Currently the most used dynamic props are size props such as left, paddingVertical, marginTop and so on. These props are only accessed from the layout thread so the pool works fine. If a dynamic prop is needed in UI thread, then the two threads can access the same pool object and cause random errors. This PR make the pool object thread local to avoid synchronization. After this change there are two pool objects created in the process.

Test Plan

Tested in official Airbnb app after updating accessibilityComponentType to be dynamic.

Related PRs

Once this is merged, I'll send another PR to support "disabled" state in accessibilityComponentType.

Release Notes

[ANDROID] [BUGFIX] [DynamicFromMap] - Fix a crash caused by dynamic props.

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Mar 5, 2018

@haitaoli I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@haitaoli haitaoli force-pushed the haitaoli:android-fix-synchronization branch from c9a76e3 to 052d9fb Mar 6, 2018

@haitaoli

This comment has been minimized.

Contributor

haitaoli commented Mar 6, 2018

Rebased

@facebook-github-bot

This comment has been minimized.

facebook-github-bot commented Apr 5, 2018

@haitaoli I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@hey99xx

This comment has been minimized.

hey99xx commented Oct 3, 2018

Our team also noticed this bug after inspecting a crash in production. We actually got a very similar stacktrace to #19141 starting with

Caused by java.lang.ArrayIndexOutOfBoundsException: length=10; index=-1
       at android.support.v4.util.Pools$SimplePool.release(Pools.java:120)
       at com.facebook.react.bridge.DynamicFromMap.recycle(DynamicFromMap.java:42)

only the line number at DynamicFromMap class is now 40 instead of 42 (running more recent version of RN). So this is an obviously an issue that needs to be fixed, either with ThreadLocal usage as in this PR or by using Pools.SynchronizedPool<DynamicFromMap>.

@haitaoli Is this PR abandoned (probably since AirBnb is leaving RN) or will you continue working on it? I think the same fix should also be done for DynamicFromArray class for correctness, even if it's not an issue yet.

@haitaoli

This comment has been minimized.

Contributor

haitaoli commented Oct 3, 2018

@hey99xx I don't think anyone will review and merge this PR, since this probably doesn't affect FB's own apps. I have another PR to fix a bug on iOS and that's not going anywhere either. As you mentioned Airbnb is deprecating RN so I won't be pushing this fix to get merged. It might be easier for you to fork RN and merge any fixes you need.

@janicduplessis

This comment has been minimized.

Collaborator

janicduplessis commented Oct 13, 2018

@haitaoli Sorry this PR got unnoticed. We actually hit an issue caused by this in react-native-svg. Looks good to me so we can go ahead and ship this.

@janicduplessis

This comment has been minimized.

Collaborator

janicduplessis commented Oct 13, 2018

@janicduplessis

This comment has been minimized.

Collaborator

janicduplessis commented Oct 13, 2018

@haitaoli Can you link me the iOS PR, I can have a look.

@facebook-github-bot

janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@haitaoli

This comment has been minimized.

Contributor

haitaoli commented Oct 13, 2018

@janicduplessis thanks! Here is the iOS PR I mentioned: #17230

@react-native-bot

This comment has been minimized.

Collaborator

react-native-bot commented Oct 14, 2018

@haitaoli merged commit b0d68c0 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Oct 14, 2018

jkimbo added a commit to Zegocover/react-native that referenced this pull request Oct 19, 2018

Fix DynamicFromMap object pool synchronization (facebook#17842)
Summary:
DynamicFromMap internally uses SimplePool object to recycle dynamic prop objects. But the pool is not multi-thread safe. Currently the most used dynamic props are size props such as left, paddingVertical, marginTop and so on. These props are only accessed from the layout thread so the pool works fine. If a dynamic prop is needed in UI thread, then the two threads can access the same pool object and cause random errors. This PR make the pool object thread local to avoid synchronization. After this change there are two pool objects created in the process.

Tested in official Airbnb app after updating accessibilityComponentType to be dynamic.

Once this is merged, I'll send another PR to support "disabled" state in `accessibilityComponentType`.

[ANDROID] [BUGFIX] [DynamicFromMap] - Fix a crash caused by dynamic props.
Pull Request resolved: facebook#17842

Differential Revision: D10374238

Pulled By: hramos

fbshipit-source-id: 7ebf89c5abf06bd5fb43b205348ba4dc7e19517d

kelset added a commit that referenced this pull request Oct 23, 2018

Fix DynamicFromMap object pool synchronization (#17842)
Summary:
DynamicFromMap internally uses SimplePool object to recycle dynamic prop objects. But the pool is not multi-thread safe. Currently the most used dynamic props are size props such as left, paddingVertical, marginTop and so on. These props are only accessed from the layout thread so the pool works fine. If a dynamic prop is needed in UI thread, then the two threads can access the same pool object and cause random errors. This PR make the pool object thread local to avoid synchronization. After this change there are two pool objects created in the process.

Tested in official Airbnb app after updating accessibilityComponentType to be dynamic.

Once this is merged, I'll send another PR to support "disabled" state in `accessibilityComponentType`.

[ANDROID] [BUGFIX] [DynamicFromMap] - Fix a crash caused by dynamic props.
Pull Request resolved: #17842

Differential Revision: D10374238

Pulled By: hramos

fbshipit-source-id: 7ebf89c5abf06bd5fb43b205348ba4dc7e19517d

sjchmiela added a commit to expo/react-native that referenced this pull request Oct 24, 2018

Fix DynamicFromMap object pool synchronization (facebook#17842)
Summary:
DynamicFromMap internally uses SimplePool object to recycle dynamic prop objects. But the pool is not multi-thread safe. Currently the most used dynamic props are size props such as left, paddingVertical, marginTop and so on. These props are only accessed from the layout thread so the pool works fine. If a dynamic prop is needed in UI thread, then the two threads can access the same pool object and cause random errors. This PR make the pool object thread local to avoid synchronization. After this change there are two pool objects created in the process.

Tested in official Airbnb app after updating accessibilityComponentType to be dynamic.

Once this is merged, I'll send another PR to support "disabled" state in `accessibilityComponentType`.

[ANDROID] [BUGFIX] [DynamicFromMap] - Fix a crash caused by dynamic props.
Pull Request resolved: facebook#17842

Differential Revision: D10374238

Pulled By: hramos

fbshipit-source-id: 7ebf89c5abf06bd5fb43b205348ba4dc7e19517d

sjchmiela added a commit to expo/react-native that referenced this pull request Oct 24, 2018

Fix DynamicFromMap object pool synchronization (facebook#17842)
Summary:
DynamicFromMap internally uses SimplePool object to recycle dynamic prop objects. But the pool is not multi-thread safe. Currently the most used dynamic props are size props such as left, paddingVertical, marginTop and so on. These props are only accessed from the layout thread so the pool works fine. If a dynamic prop is needed in UI thread, then the two threads can access the same pool object and cause random errors. This PR make the pool object thread local to avoid synchronization. After this change there are two pool objects created in the process.

Tested in official Airbnb app after updating accessibilityComponentType to be dynamic.

Once this is merged, I'll send another PR to support "disabled" state in `accessibilityComponentType`.

[ANDROID] [BUGFIX] [DynamicFromMap] - Fix a crash caused by dynamic props.
Pull Request resolved: facebook#17842

Differential Revision: D10374238

Pulled By: hramos

fbshipit-source-id: 7ebf89c5abf06bd5fb43b205348ba4dc7e19517d

sjchmiela added a commit to expo/react-native that referenced this pull request Oct 24, 2018

Fix DynamicFromMap object pool synchronization (facebook#17842)
Summary:
DynamicFromMap internally uses SimplePool object to recycle dynamic prop objects. But the pool is not multi-thread safe. Currently the most used dynamic props are size props such as left, paddingVertical, marginTop and so on. These props are only accessed from the layout thread so the pool works fine. If a dynamic prop is needed in UI thread, then the two threads can access the same pool object and cause random errors. This PR make the pool object thread local to avoid synchronization. After this change there are two pool objects created in the process.

Tested in official Airbnb app after updating accessibilityComponentType to be dynamic.

Once this is merged, I'll send another PR to support "disabled" state in `accessibilityComponentType`.

[ANDROID] [BUGFIX] [DynamicFromMap] - Fix a crash caused by dynamic props.
Pull Request resolved: facebook#17842

Differential Revision: D10374238

Pulled By: hramos

fbshipit-source-id: 7ebf89c5abf06bd5fb43b205348ba4dc7e19517d
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.