-
Notifications
You must be signed in to change notification settings - Fork 24.2k
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
Conversation
@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. |
c9a76e3
to
052d9fb
Compare
Rebased |
@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. |
Our team also noticed this bug after inspecting a crash in production. We actually got a very similar stacktrace to #19141 starting with
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 @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 |
@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. |
@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. |
@facebook-github-bot shipit |
@haitaoli Can you link me the iOS PR, I can have a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
janicduplessis is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
@janicduplessis thanks! Here is the iOS PR I mentioned: #17230 |
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
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
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
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
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
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
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
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
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.