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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

fix: handle webview provider missing exception #34456

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6be311
fix: handle webview provider missing exception
Aug 19, 2022
3323377
Merge branch 'main' into patch-2
Sep 21, 2022
237bd91
fix: update comment for graceful error handling
Sep 22, 2022
8959dae
docs: add more info to getCookieManager return
rachitmishra Sep 22, 2022
eb3bedc
Update ReactAndroid/src/main/java/com/facebook/react/modules/network/…
Sep 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 3 additions & 19 deletions ReactAndroid/src/main/java/com/facebook/react/modules/network/ForwardingCookieHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,25 +138,9 @@ protected void doInBackgroundGuarded(Void... params) {
// https://bugs.chromium.org/p/chromium/issues/detail?id=559720
return null;
} catch (Exception exception) {
String message = exception.getMessage();
// We cannot catch MissingWebViewPackageException as it is in a private / system API
// class. This validates the exception's message to ensure we are only handling this
// specific exception.
// The exception class doesn't always contain the correct name as it depends on the OEM
// and OS version. It is better to check the message for clues regarding the exception
// as that is somewhat consistent across OEMs.
// For instance, the Exception thrown on OxygenOS 11 is a RuntimeException but the message
// contains the required strings.
// https://android.googlesource.com/platform/frameworks/base/+/master/core/java/android/webkit/WebViewFactory.java#348
if (exception.getClass().getCanonicalName().contains("MissingWebViewPackageException")
|| (message != null
&& (message.contains("WebView provider")
|| message.contains("No WebView installed")
|| message.contains("Cannot load WebView")))) {
return null;
} else {
throw exception;
}
// fatal exception is no good for the user experience,
Copy link
Contributor

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

Copy link
Contributor Author

@rachitmishra rachitmishra Aug 20, 2022 •
edited
Loading

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.

// return null in any case when a webview provider is not found.
return null;
}
}

Expand Down