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: Crash if WebView is disabled #34483

Closed
wants to merge 1 commit into from
Closed

fix: Crash if WebView is disabled #34483

wants to merge 1 commit into from

Conversation

Pajn
Copy link
Contributor

@Pajn Pajn commented Aug 23, 2022

Summary

When the webview is updated by play store, all apps that has loaded webview gets killed. For background see https://issuetracker.google.com/issues/228611949?pli=1

We have a long-running app and don't want this to happen and while we are not using webview anywhere, we are using websockets that loads it for reading cookies, however we are not using cookies to authenticate the websocket and thus want to disable the webview to avoid loading it unnecessarily and then getting killed unnecessarily.

The webview has support for beeing disabled https://developer.android.com/reference/android/webkit/WebView#disableWebView() however as-is this crashes React Native. It seems to me like this case is very similar to not having web view installed and should be handled in the same way.

Changelog

[Android] [Fixed] - Avoid crash in ForwardingCookieHandler if webview is disabled

Test Plan

Add WebView.disableWebView(); as the first line in MainActivity.onCreate.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Aug 23, 2022
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Aug 23, 2022
@analysis-bot
Copy link

analysis-bot commented Aug 23, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,617,924 +12
android hermes armeabi-v7a 7,032,157 -6
android hermes x86 7,917,986 +13
android hermes x86_64 7,891,625 +35
android jsc arm64-v8a 9,495,632 -62
android jsc armeabi-v7a 8,273,240 -77
android jsc x86 9,433,426 -53
android jsc x86_64 10,026,421 -43

Base commit: 2452c5f
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@cipolleschi
Copy link
Contributor

Hi @Pajn, thanks for the PR. Could you please rebase on main? We should have fixed the CI issues.

@cipolleschi
Copy link
Contributor

Sorry to ask that, but could you rebase again? The CI is still red but for some other commits that landed in between.. :(

@analysis-bot
Copy link

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

Base commit: 2452c5f
Branch: main

@facebook-github-bot
Copy link
Contributor

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

@Pajn Pajn force-pushed the patch-1 branch 2 times, most recently from cf47331 to a68ab50 Compare August 26, 2022 16:07
@Pajn
Copy link
Contributor Author

Pajn commented Aug 26, 2022

Sorry, my additions skipped the earlier nullcheck. It should be correct now.

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Pajn in 5451cd4.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Aug 30, 2022
dmytrorykun pushed a commit that referenced this pull request Sep 14, 2022
Summary:
When the webview is updated by play store, all apps that has loaded webview gets killed. For background see https://issuetracker.google.com/issues/228611949?pli=1

We have a long-running app and don't want this to happen and while we are not using webview anywhere, we are using websockets that loads it for reading cookies, however we are not using cookies to authenticate the websocket and thus want to disable the webview to avoid loading it unnecessarily and then getting killed unnecessarily.

The webview has support for beeing disabled https://developer.android.com/reference/android/webkit/WebView#disableWebView() however as-is this crashes React Native. It seems to me like this case is very similar to not having web view installed and should be handled in the same way.

## 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] - Avoid crash in ForwardingCookieHandler if webview is disabled

Pull Request resolved: #34483

Test Plan: Add `WebView.disableWebView();` as the first line in `MainActivity.onCreate`.

Reviewed By: christophpurrer

Differential Revision: D38981827

Pulled By: cipolleschi

fbshipit-source-id: 335a8420568ad0c80b834ae8a3b164e55ebe26f3
rthic23 pushed a commit to electrode-io/react-native that referenced this pull request May 23, 2023
Summary:
When the webview is updated by play store, all apps that has loaded webview gets killed. For background see https://issuetracker.google.com/issues/228611949?pli=1

We have a long-running app and don't want this to happen and while we are not using webview anywhere, we are using websockets that loads it for reading cookies, however we are not using cookies to authenticate the websocket and thus want to disable the webview to avoid loading it unnecessarily and then getting killed unnecessarily.

The webview has support for beeing disabled https://developer.android.com/reference/android/webkit/WebView#disableWebView() however as-is this crashes React Native. It seems to me like this case is very similar to not having web view installed and should be handled in the same way.

## 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] - Avoid crash in ForwardingCookieHandler if webview is disabled

Pull Request resolved: facebook#34483

Test Plan: Add `WebView.disableWebView();` as the first line in `MainActivity.onCreate`.

Reviewed By: christophpurrer

Differential Revision: D38981827

Pulled By: cipolleschi

fbshipit-source-id: 335a8420568ad0c80b834ae8a3b164e55ebe26f3

# Conflicts:
#	ReactAndroid/src/main/java/com/facebook/react/modules/network/ForwardingCookieHandler.java
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. Merged This PR has been merged. Platform: Android Android applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants