Skip to content
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 #6228: Crash in Android dialog module. #17392

Closed
wants to merge 1 commit into from

Conversation

dryganets
Copy link
Contributor

@dryganets dryganets commented Dec 29, 2017

Motivation

Android dialog module has a race condition as a result of which it crashes.
See this issue:
#6228.

The mIsInForeground flag is set on UI thread but was used from the ReactMethod thread.
Now all public methods of FragmentManagerHelper called from UI thread only.
Asserts are added in appropriate places to prevent future regressions.

Test Plan

Make sure that dialogs work after this change.
It will be nearly impossible to reproduce the issue manually but automatic regression tests should be able to catch this. At least our tests were crashing on some dialog scenarios from time to time.

Release Notes

[ANDROID] [MINOR] [BUGFIX] - Race condition fix in Android Dialogs module.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 29, 2017
…his issue:

facebook#6228.

The mIsInForeground flag is set on UI thread but was used from the ReactMethod thread.
I added asserts to make sure that FragmentManagerHelper wouldn't be used incorrectly.

Make sure that dialogs work after this change.
It will be nearly impossible to reproduce the issue manually but automatic regression tests should be able to catch this. At least our tests are crashing on some dialog scenarios from time to time.

[ANDROID] [MINOR] [BUGFIX] - Race condition fix for Dialogs module.
@dryganets
Copy link
Contributor Author

The failing test is unrelated to the change:

#!/bin/bash -eo pipefail
buck fetch ReactAndroid/src/test/java/com/facebook/react/modules
Not using buckd because watchman isn't installed.
Picked up _JAVA_OPTIONS: -XX:+UnlockExperimentalVMOptions -XX:+UseCGroupMemoryLimitForHeap
PARSING BUCK FILES... 1.3s (100%)
BUILD FAILED: Buck wasn't able to parse /home/circleci/react-native/ReactCommon/yoga/BUCK:
NameError: name 'fb_xplat_cxx_library' is not defined
Call stack:
  File "/home/circleci/buck/python-dsl/buck_parser/buck.py", line 1210, in process_with_diagnostics
    diagnostics=diagnostics)
  File "/home/circleci/buck/python-dsl/buck_parser/buck.py", line 1101, in process
    os.path.join(self._project_root, path))
  File "/home/circleci/buck/python-dsl/buck_parser/buck.py", line 1095, in _process_build_file
    return self._process(build_env, path, is_implicit_include=False)
  File "/home/circleci/buck/python-dsl/buck_parser/buck.py", line 1047, in _process
    exec(code, module.__dict__)
  File "/home/circleci/react-native/ReactCommon/yoga/BUCK", line 1
    fb_xplat_cxx_library(


This error happened while trying to get dependency '//ReactCommon/yoga:yoga' of target '//ReactAndroid/src/test/java/com/facebook/react/modules:modules'
Exited with code 1

@dryganets dryganets changed the title Bug fix for https://github.com/facebook/react-native/issues/6228. Fix #6228: Crash in Android dialog module. Dec 29, 2017
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@elicwhite elicwhite added p: Microsoft Partner: Microsoft Partner and removed Partner p: Microsoft Partner: Microsoft labels Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants