react-native icon indicating copy to clipboard operation
react-native copied to clipboard

fix: handle webview provider missing exception

Open rachitmishra opened this issue 1 year ago • 2 comments

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.

Screenshot 2022-08-19 at 4 33 31 PM

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.

rachitmishra avatar Aug 19 '22 11:08 rachitmishra

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,698,513 -27,445
android hermes armeabi-v7a 7,102,105 -26,248
android hermes x86 8,001,264 -30,580
android hermes x86_64 7,973,833 -31,366
android jsc arm64-v8a 9,569,022 -27,259
android jsc armeabi-v7a 8,336,040 -26,058
android jsc x86 9,509,448 -30,380
android jsc x86_64 10,101,129 -31,184

Base commit: 2fc44ac8e144b7d93e1ccc95163dd33c94d2a197 Branch: main

analysis-bot avatar Aug 19 '22 11:08 analysis-bot

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

Pajn avatar Aug 23 '22 17:08 Pajn

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 2fc44ac8e144b7d93e1ccc95163dd33c94d2a197 Branch: main

analysis-bot avatar Sep 22 '22 00:09 analysis-bot

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

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 return null as it's currently stale, explaining why we do this?

cortinico avatar Sep 22 '22 09:09 cortinico

@cortinico I have updated the comment and our reasoning, let me know if we can improve this more :)

rachitmishra avatar Sep 22 '22 19:09 rachitmishra

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

facebook-github-bot avatar Sep 26 '22 08:09 facebook-github-bot

This pull request was successfully merged by @rachitmishra in 3f3394a5668d515e3476933aec67f07794c6e765.

When will my fix make it into a release? | Upcoming Releases

react-native-bot avatar Sep 26 '22 20:09 react-native-bot