-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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: handle webview provider missing exception #34456
Conversation
Base commit: 2fc44ac |
} else { | ||
throw exception; | ||
} | ||
// fatal exception is no good for the user experience, |
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.
I'm generally against this change as we would blindly catch all the exceptions and return null
here while we want to distinguish between MissingWebViewPackageException
and other type of exceptions
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.
@cortinico i totally agree. this is not a best practice, or in principal good code.
Although the thought here is that, for any case when the CookieManager
instance is not available, returning a null
is better than the fatal
exception.
I opened #34483 but IMHO this is better. It's very weird to me that not having a webview at all is fine but having one that can't be loaded for whatever reason is a fatal crash |
Base commit: 2fc44ac |
I've looked again at this PR and I believe we should merge it as the scope is limited to only the CookieManager. @rachitmishra Can you update the comment before the |
@cortinico I have updated the comment and our reasoning, let me know if we can improve this more :) |
ReactAndroid/src/main/java/com/facebook/react/modules/network/ForwardingCookieHandler.java
Outdated
Show resolved
Hide resolved
…ForwardingCookieHandler.java Co-authored-by: Nicola Corti <corti.nico@gmail.com>
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @rachitmishra in 3f3394a. When will my fix make it into a release? | Upcoming Releases |
Summary: The [existing fix](https://github.com/facebook/react-native/blob/fb936dfffb3ca2d9bc0969dfe36a70bdf66783e0/ReactAndroid/src/main/java/com/facebook/react/modules/network/ForwardingCookieHandler.java#L151) to handle missing WebView provider uses string comparison based checks to handle the exception gracefully, or otherwise simply throw the exception. This ends up crashing the app for the end user. <img width="1319" alt="Screenshot 2022-08-19 at 4 33 31 PM" src="https://user-images.githubusercontent.com/933314/185605137-24757dad-806e-4cca-b000-7d6ce2d191e1.png"> Fatal exceptions are bad in any case and not good user experience, we can gracefully handle this by [returning](facebook/react-native@main...rachitmishra:react-native-1:patch-2#diff-f7ca1976002c4612051e4949395e64511b6f769e347c488e9a0d15cb5331fe76R141) `null` for all cases when WebView provider is not found. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. For an example, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [Android] [Fixed] - Gracefully handle crash if no WebView provider is found on the device Pull Request resolved: facebook#34456 Test Plan: IMO no testing is required as we were already returning null in certain cases after handling the exception message, also `ForwardingCookieManager::getCookieManager` is already marked `Nullable` so we are safe there. Reviewed By: lunaleaps Differential Revision: D39809020 Pulled By: cortinico fbshipit-source-id: 54b290ad7740859bdc84401904236c32761a4631
Summary
The existing fix to handle missing WebView provider uses string comparison based checks to handle the exception gracefully, or otherwise simply throw the exception. This ends up crashing the app for the end user.
Fatal exceptions are bad in any case and not good user experience, we can gracefully handle this by returning
null
for all cases when WebView provider is not found.Changelog
[Android] [Fixed] - Gracefully handle crash if no WebView provider is found on the device
Test Plan
IMO no testing is required as we were already returning null in certain cases after handling the exception message, also
ForwardingCookieManager::getCookieManager
is already marked@Nullable
so we are safe there.