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

Add null check in UIIMplementation#setChildren #19651

Conversation

fourlastor
Copy link
Contributor

This PR implements the suggested fix for #17123, basically adding a null check after getting the view via tag in UIImplementation#setChildren.

Test Plan

As per issue:

  1. Create a native ViewPager with a ReactRootView as content, preferably with some images inside the ReactRootView
  2. Swipe fast (back and forth)
  3. The app will crash with an NPE before the fix, no NPE and a warning is logged after the fix.

Release Notes

[ANDROID] [BUGFIX] [UIImplementation] - Added null check in UIImplementation#setChildren

@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 Jun 11, 2018
Copy link
Contributor

@mdvacca mdvacca left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @fourlastor

With this fix you might be hiding another bug.
what happened with the viewTag received by parameter? was this deleted? was it never added? can you elaborate about this?

Thank you

@fourlastor
Copy link
Contributor Author

@mdvacca as far as I understood, the viewpager removes the view (or a view containing the view), then the method gets called. At that point the view isn't there anymore and getting it via tag returns null. Can investigate more but it's kinda tricky, because it's very hard to reproduce reliably (you have to keep swiping back and forth).

@facebook-github-bot
Copy link
Contributor

@fourlastor 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.

@patrickkempff
Copy link
Contributor

I agree with @mdvacca. I think this is an threading issue. I don't think that silencing the exception is the right approach. PR #20025 should fix this issue.

Util this has landed you can try the steps described by @SudoPlz here: #17178 (comment)

@fourlastor
Copy link
Contributor Author

That's fair enough, I'll close this then! Thanks for getting back to me (I tried to investigate but got pretty much stuck at the same point every time)

@fourlastor fourlastor closed this Jul 12, 2018
@fourlastor fourlastor deleted the avoid-npe-ui-implementation branch July 12, 2018 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants