Skip to content

Commit 69d826a

Browse files
cmonfortepcmonfortep
authored
Allow users to recover from WebView renderer failures (#644)
Allow users to recover from WebView renderer failures: * Recover BrowserTabFragment after WebView crashes. * GlobalLayoutViewState as sealed class to model invalidated state or Browser. * When Tab is invalidated and user submits another query: close current tab and open query in a new tab. * Disable report a site when WebView crashes. * Disable navigation when WebView crashes. * Disable changing browsing mode when WebView is gone. * Avoid showing ErrorSnackbar when user goes to Home screen. * Handling WebView crashes in SurveyActivity
1 parent fc04b75 commit 69d826a

File tree

15 files changed

+484
-82
lines changed

15 files changed

+484
-82
lines changed

‎app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt‎

Lines changed: 182 additions & 40 deletions
Large diffs are not rendered by default.

‎app/src/androidTest/java/com/duckduckgo/app/browser/BrowserWebViewClientTest.kt‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,15 @@ class BrowserWebViewClientTest {
117117
verify(offlinePixelCountDataStore, times(1)).webRendererGoneKilledCount = 1
118118
}
119119

120+
@Test
121+
@SdkSuppress(minSdkVersion = Build.VERSION_CODES.O)
122+
fun whenRenderProcessGoneThenEmitEventIntoListener() {
123+
val detail: RenderProcessGoneDetail = mock()
124+
whenever(detail.didCrash()).thenReturn(true)
125+
testee.onRenderProcessGone(webView, detail)
126+
verify(listener, times(1)).recoverFromRenderProcessGone()
127+
}
128+
120129
private class TestWebView(context: Context) : WebView(context)
121130

122131
companion object {

‎app/src/androidTest/java/com/duckduckgo/app/browser/EmptyNavigationStateTest.kt‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/*
2+
* Copyright (c) 2019 DuckDuckGo
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.duckduckgo.app.browser
18+
19+
import org.junit.Assert.assertEquals
20+
import org.junit.Assert.assertFalse
21+
import org.junit.Test
22+
23+
class EmptyNavigationStateTest {
24+
25+
@Test
26+
fun whenEmptyNavigationStateFromNavigationStateThenBrowserPropertiesAreTheSame() {
27+
val previousState = buildState("originalUrl", "currentUrl", "titlle")
28+
val emptyNavigationState = EmptyNavigationState(previousState)
29+
30+
assertEquals(emptyNavigationState.currentUrl, previousState.currentUrl)
31+
assertEquals(emptyNavigationState.originalUrl, previousState.originalUrl)
32+
assertEquals(emptyNavigationState.title, previousState.title)
33+
}
34+
35+
@Test
36+
fun whenEmptyNavigationStateFromNavigationStateThenNavigationPropertiesAreCleared() {
37+
val emptyNavigationState = EmptyNavigationState(buildState("originalUrl", "currentUrl", "titlle"))
38+
39+
assertEquals(emptyNavigationState.stepsToPreviousPage, 0)
40+
assertFalse(emptyNavigationState.canGoBack)
41+
assertFalse(emptyNavigationState.canGoForward)
42+
assertFalse(emptyNavigationState.hasNavigationHistory)
43+
}
44+
45+
private fun buildState(originalUrl: String?, currentUrl: String?, title: String? = null): WebNavigationState {
46+
return TestNavigationState(
47+
originalUrl = originalUrl,
48+
currentUrl = currentUrl,
49+
title = title,
50+
stepsToPreviousPage = 1,
51+
canGoBack = true,
52+
canGoForward = true,
53+
hasNavigationHistory = true
54+
)
55+
}
56+
}

‎app/src/androidTest/java/com/duckduckgo/app/browser/TestNavigationState.kt‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/*
2+
* Copyright (c) 2019 DuckDuckGo
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package com.duckduckgo.app.browser
18+
19+
data class TestNavigationState(
20+
override val originalUrl: String?,
21+
override val currentUrl: String?,
22+
override val title: String?,
23+
override val stepsToPreviousPage: Int,
24+
override val canGoBack: Boolean,
25+
override val canGoForward: Boolean,
26+
override val hasNavigationHistory: Boolean
27+
) : WebNavigationState

‎app/src/androidTest/java/com/duckduckgo/app/browser/WebNavigationStateTest.kt‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -91,13 +91,6 @@ class WebNavigationStateComparisonTest {
9191
assertEquals(UrlUpdated("http://same.com/latest"), latestState.compare(previousState))
9292
}
9393

94-
@Test
95-
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestContainsSameOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() {
96-
val previousState = buildState("http://same.com", "http://subdomain.previous.com")
97-
val latestState = buildState("http://same.com", null)
98-
assertEquals(Other, latestState.compare(previousState))
99-
}
100-
10194
@Test
10295
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestStateContainsNoOriginalUrlAndNoCurrentUrlThenCompareReturnsPageCleared() {
10396
val previousState = buildState("http://previous.com", "http://subdomain.previous.com")
@@ -112,6 +105,20 @@ class WebNavigationStateComparisonTest {
112105
assertEquals(PageCleared, latestState.compare(previousState))
113106
}
114107

108+
@Test
109+
fun whenLatestStateIsEmptyNavigationCompareReturnsPageNavigationCleared() {
110+
val previousState = buildState("http://previous.com", "http://subdomain.previous.com")
111+
val latestState = EmptyNavigationState(previousState)
112+
assertEquals(PageNavigationCleared, latestState.compare(previousState))
113+
}
114+
115+
@Test
116+
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestContainsSameOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() {
117+
val previousState = buildState("http://same.com", "http://subdomain.previous.com")
118+
val latestState = buildState("http://same.com", null)
119+
assertEquals(Other, latestState.compare(previousState))
120+
}
121+
115122
@Test
116123
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestStateContainsDifferentOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() {
117124
val previousState = buildState("http://previous.com", "http://subdomain.previous.com")
@@ -131,13 +138,3 @@ class WebNavigationStateComparisonTest {
131138
)
132139
}
133140
}
134-
135-
data class TestNavigationState(
136-
override val originalUrl: String?,
137-
override val currentUrl: String?,
138-
override val title: String?,
139-
override val stepsToPreviousPage: Int,
140-
override val canGoBack: Boolean,
141-
override val canGoForward: Boolean,
142-
override val hasNavigationHistory: Boolean
143-
) : WebNavigationState

‎app/src/main/java/com/duckduckgo/app/browser/BrowserActivity.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,7 +244,7 @@ class BrowserActivity : DuckDuckGoActivity(), CoroutineScope {
244244
Timber.i("Processing command: $command")
245245
when (command) {
246246
is Query -> currentTab?.submitQuery(command.query)
247-
is Refresh -> currentTab?.refresh()
247+
is Refresh -> currentTab?.onRefreshRequested()
248248
is Command.DisplayMessage -> applicationContext?.longToast(command.messageId)
249249
is Command.LaunchPlayStore -> launchPlayStore()
250250
is Command.ShowAppEnjoymentPrompt -> showAppEnjoymentPrompt(AppEnjoymentDialogFragment.create(command.promptCount, viewModel))

‎app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt‎

Lines changed: 43 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,11 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
203203

204204
private var webView: WebView? = null
205205

206+
private val errorSnackbar: Snackbar by lazy {
207+
Snackbar.make(browserLayout, R.string.crashedWebViewErrorMessage, Snackbar.LENGTH_INDEFINITE)
208+
.setBehavior(NonDismissibleBehavior())
209+
}
210+
206211
private val findInPageTextWatcher = object : TextChangedWatcher() {
207212
override fun afterTextChanged(editable: Editable) {
208213
viewModel.userFindingInPage(findInPageInput.text.toString())
@@ -309,7 +314,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
309314
popupMenu.apply {
310315
onMenuItemClicked(view.forwardPopupMenuItem) { viewModel.onUserPressedForward() }
311316
onMenuItemClicked(view.backPopupMenuItem) { activity?.onBackPressed() }
312-
onMenuItemClicked(view.refreshPopupMenuItem) { refresh() }
317+
onMenuItemClicked(view.refreshPopupMenuItem) { viewModel.onRefreshRequested() }
313318
onMenuItemClicked(view.newTabPopupMenuItem) { viewModel.userRequestedOpeningNewTab() }
314319
onMenuItemClicked(view.bookmarksPopupMenuItem) { browserActivity?.launchBookmarks() }
315320
onMenuItemClicked(view.addBookmarksPopupMenuItem) { launch { viewModel.onBookmarkAddRequested() } }
@@ -374,6 +379,8 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
374379
}
375380

376381
private fun showHome() {
382+
errorSnackbar.dismiss()
383+
newTabLayout.show()
377384
showKeyboardImmediately()
378385
appBarLayout.setExpanded(true)
379386
webView?.onPause()
@@ -383,6 +390,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
383390
}
384391

385392
private fun showBrowser() {
393+
newTabLayout.gone()
386394
webView?.show()
387395
webView?.onResume()
388396
omnibarScrolling.enableOmnibarScrolling(toolbarContainer)
@@ -398,13 +406,17 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
398406
webView?.loadUrl(url)
399407
}
400408

409+
fun onRefreshRequested() {
410+
viewModel.onRefreshRequested()
411+
}
412+
401413
fun refresh() {
402414
webView?.reload()
403415
}
404416

405417
private fun processCommand(it: Command?) {
406418
when (it) {
407-
Command.Refresh -> refresh()
419+
is Command.Refresh -> refresh()
408420
is Command.OpenInNewTab -> {
409421
browserActivity?.openInNewTab(it.query)
410422
}
@@ -419,10 +431,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
419431
is Command.NavigateBack -> {
420432
webView?.goBackOrForward(-it.steps)
421433
}
422-
Command.NavigateForward -> {
434+
is Command.NavigateForward -> {
423435
webView?.goForward()
424436
}
425-
Command.ResetHistory -> {
437+
is Command.ResetHistory -> {
426438
resetWebView()
427439
}
428440
is Command.DialNumber -> {
@@ -439,10 +451,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
439451
val intent = Intent(Intent.ACTION_SENDTO, Uri.parse("smsto:${it.telephoneNumber}"))
440452
openExternalDialog(intent)
441453
}
442-
Command.ShowKeyboard -> {
454+
is Command.ShowKeyboard -> {
443455
showKeyboard()
444456
}
445-
Command.HideKeyboard -> {
457+
is Command.HideKeyboard -> {
446458
hideKeyboard()
447459
}
448460
is Command.BrokenSiteFeedback -> {
@@ -458,7 +470,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
458470
}
459471
is Command.DownloadImage -> requestImageDownload(it.url)
460472
is Command.FindInPageCommand -> webView?.findAllAsync(it.searchTerm)
461-
Command.DismissFindInPage -> webView?.findAllAsync(null)
473+
is Command.DismissFindInPage -> webView?.findAllAsync(null)
462474
is Command.ShareLink -> launchSharePageChooser(it.url)
463475
is Command.CopyLink -> {
464476
clipboardManager.primaryClip = ClipData.newPlainText(null, it.url)
@@ -481,6 +493,14 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
481493
is Command.SaveCredentials -> saveBasicAuthCredentials(it.request, it.credentials)
482494
is Command.GenerateWebViewPreviewImage -> generateWebViewPreviewImage()
483495
is Command.LaunchTabSwitcher -> launchTabSwitcher()
496+
is Command.ShowErrorWithAction -> showErrorSnackbar(it)
497+
}
498+
}
499+
500+
private fun showErrorSnackbar(command: Command.ShowErrorWithAction) {
501+
//Snackbar is global and it should appear only the foreground fragment
502+
if (!errorSnackbar.view.isAttachedToWindow && isVisible) {
503+
errorSnackbar.setAction(R.string.crashedWebViewErrorAction) { command.action() }.show()
484504
}
485505
}
486506

@@ -1124,13 +1144,23 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
11241144
}
11251145

11261146
fun renderGlobalViewState(viewState: GlobalLayoutViewState) {
1147+
if (lastSeenGlobalViewState is GlobalLayoutViewState.Invalidated &&
1148+
viewState is GlobalLayoutViewState.Browser) {
1149+
throw IllegalStateException("Invalid state transition")
1150+
}
1151+
11271152
renderIfChanged(viewState, lastSeenGlobalViewState) {
11281153
lastSeenGlobalViewState = viewState
11291154

1130-
if (viewState.isNewTabState) {
1131-
browserLayout.hide()
1132-
} else {
1133-
browserLayout.show()
1155+
when (viewState) {
1156+
is GlobalLayoutViewState.Browser -> {
1157+
if (viewState.isNewTabState) {
1158+
browserLayout.hide()
1159+
} else {
1160+
browserLayout.show()
1161+
}
1162+
}
1163+
is GlobalLayoutViewState.Invalidated -> destroyWebView()
11341164
}
11351165
}
11361166
}
@@ -1174,6 +1204,8 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
11741204
newTabPopupMenuItem.isEnabled = browserShowing
11751205
addBookmarksPopupMenuItem?.isEnabled = viewState.canAddBookmarks
11761206
sharePageMenuItem?.isEnabled = viewState.canSharePage
1207+
brokenSitePopupMenuItem?.isEnabled = viewState.canReportSite
1208+
requestDesktopSiteCheckMenuItem?.isEnabled = viewState.canChangeBrowsingMode
11771209

11781210
addToHome?.let {
11791211
it.visibility = if (viewState.addToHomeVisible) VISIBLE else GONE

0 commit comments

Comments
 (0)