Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
90496d3
Recover BrowserTabFragment after WebView crashes.
cmonfortep Dec 3, 2019
94a28b2
Improving how to show snackbar adding isVisible property to GlobalLay…
cmonfortep Dec 4, 2019
d65e3f6
When Tab is invalidated and user submits another query: close current…
cmonfortep Dec 4, 2019
3864cfa
Handling WebView crashes in SurveyActivity
cmonfortep Dec 4, 2019
c91f8da
Disable report a site when WebView crashes.
cmonfortep Dec 4, 2019
ef50eda
Pipe refresh events into ViewModel to recover if Browser is in invali…
cmonfortep Dec 4, 2019
334d55d
Using translations from resource (no translated)
cmonfortep Dec 7, 2019
b32a596
Removing visibility from GlobalLayoutViewState + unit test
cmonfortep Dec 7, 2019
71445cb
cover BrowserWebViewClient logic added to pipe error into viewmodel
cmonfortep Dec 7, 2019
1c46cb4
Disable changing browsing mode when WebView is gone.
cmonfortep Dec 7, 2019
0435a69
Fix: onBackPressed was not working properly.
cmonfortep Dec 7, 2019
9ba5c83
Tidy up code.
cmonfortep Dec 8, 2019
31f0f6e
Moving untranslated string resources to a separated file.
cmonfortep Dec 9, 2019
ad312aa
Ignoring untranslated string resources instead of using translatable …
cmonfortep Dec 10, 2019
9a9d78a
Add assertion to validate expected URL when opening new tab.
cmonfortep Dec 10, 2019
fccad7b
Full clean up when WebView crashes.
cmonfortep Dec 10, 2019
0f29296
Use a more correct layout parent to show snackbar. (no behvior change)
cmonfortep Dec 10, 2019
254ba38
Object comparison using type and not instance.
cmonfortep Dec 10, 2019
ffad15d
Fix: Avoid showing home when webview crashes (only when home was behi…
cmonfortep Dec 10, 2019
b87e4f5
Avoid showing ErrorSnackbar when user goes to Home screen.
cmonfortep Dec 10, 2019
75183a8
When user navigates home, show navigate forward only if state is not …
cmonfortep Dec 10, 2019
41807ea
Unit tested cases:
cmonfortep Dec 10, 2019
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

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,15 @@ class BrowserWebViewClientTest {
verify(offlinePixelCountDataStore, times(1)).webRendererGoneKilledCount = 1
}

@Test
@SdkSuppress(minSdkVersion = Build.VERSION_CODES.O)
fun whenRenderProcessGoneThenEmitEventIntoListener() {
val detail: RenderProcessGoneDetail = mock()
whenever(detail.didCrash()).thenReturn(true)
testee.onRenderProcessGone(webView, detail)
verify(listener, times(1)).recoverFromRenderProcessGone()
}

private class TestWebView(context: Context) : WebView(context)

companion object {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
/*
* Copyright (c) 2019 DuckDuckGo
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.duckduckgo.app.browser

import org.junit.Assert.assertEquals
import org.junit.Assert.assertFalse
import org.junit.Test

class EmptyNavigationStateTest {

@Test
fun whenEmptyNavigationStateFromNavigationStateThenBrowserPropertiesAreTheSame() {
val previousState = buildState("originalUrl", "currentUrl", "titlle")
val emptyNavigationState = EmptyNavigationState(previousState)

assertEquals(emptyNavigationState.currentUrl, previousState.currentUrl)
assertEquals(emptyNavigationState.originalUrl, previousState.originalUrl)
assertEquals(emptyNavigationState.title, previousState.title)
}

@Test
fun whenEmptyNavigationStateFromNavigationStateThenNavigationPropertiesAreCleared() {
val emptyNavigationState = EmptyNavigationState(buildState("originalUrl", "currentUrl", "titlle"))

assertEquals(emptyNavigationState.stepsToPreviousPage, 0)
assertFalse(emptyNavigationState.canGoBack)
assertFalse(emptyNavigationState.canGoForward)
assertFalse(emptyNavigationState.hasNavigationHistory)
}

private fun buildState(originalUrl: String?, currentUrl: String?, title: String? = null): WebNavigationState {
return TestNavigationState(
originalUrl = originalUrl,
currentUrl = currentUrl,
title = title,
stepsToPreviousPage = 1,
canGoBack = true,
canGoForward = true,
hasNavigationHistory = true
)
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright (c) 2019 DuckDuckGo
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.duckduckgo.app.browser

data class TestNavigationState(
override val originalUrl: String?,
override val currentUrl: String?,
override val title: String?,
override val stepsToPreviousPage: Int,
override val canGoBack: Boolean,
override val canGoForward: Boolean,
override val hasNavigationHistory: Boolean
) : WebNavigationState
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,6 @@ class WebNavigationStateComparisonTest {
assertEquals(UrlUpdated("http://same.com/latest"), latestState.compare(previousState))
}

@Test
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestContainsSameOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() {
val previousState = buildState("http://same.com", "http://subdomain.previous.com")
val latestState = buildState("http://same.com", null)
assertEquals(Other, latestState.compare(previousState))
}

@Test
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestStateContainsNoOriginalUrlAndNoCurrentUrlThenCompareReturnsPageCleared() {
val previousState = buildState("http://previous.com", "http://subdomain.previous.com")
Expand All @@ -112,6 +105,20 @@ class WebNavigationStateComparisonTest {
assertEquals(PageCleared, latestState.compare(previousState))
}

@Test
fun whenLatestStateIsEmptyNavigationCompareReturnsPageNavigationCleared() {
val previousState = buildState("http://previous.com", "http://subdomain.previous.com")
val latestState = EmptyNavigationState(previousState)
assertEquals(PageNavigationCleared, latestState.compare(previousState))
}

@Test
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestContainsSameOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() {
val previousState = buildState("http://same.com", "http://subdomain.previous.com")
val latestState = buildState("http://same.com", null)
assertEquals(Other, latestState.compare(previousState))
}

@Test
fun whenPreviousContainsAnOriginalUrlAndCurrentUrlAndLatestStateContainsDifferentOriginalUrlAndNoCurrentUrlThenCompareReturnsOther() {
val previousState = buildState("http://previous.com", "http://subdomain.previous.com")
Expand All @@ -131,13 +138,3 @@ class WebNavigationStateComparisonTest {
)
}
}

data class TestNavigationState(
override val originalUrl: String?,
override val currentUrl: String?,
override val title: String?,
override val stepsToPreviousPage: Int,
override val canGoBack: Boolean,
override val canGoForward: Boolean,
override val hasNavigationHistory: Boolean
) : WebNavigationState
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ class BrowserActivity : DuckDuckGoActivity(), CoroutineScope {
Timber.i("Processing command: $command")
when (command) {
is Query -> currentTab?.submitQuery(command.query)
is Refresh -> currentTab?.refresh()
is Refresh -> currentTab?.onRefreshRequested()
is Command.DisplayMessage -> applicationContext?.longToast(command.messageId)
is Command.LaunchPlayStore -> launchPlayStore()
is Command.ShowAppEnjoymentPrompt -> showAppEnjoymentPrompt(AppEnjoymentDialogFragment.create(command.promptCount, viewModel))
Expand Down
54 changes: 43 additions & 11 deletions app/src/main/java/com/duckduckgo/app/browser/BrowserTabFragment.kt
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,11 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {

private var webView: WebView? = null

private val errorSnackbar: Snackbar by lazy {
Snackbar.make(browserLayout, R.string.crashedWebViewErrorMessage, Snackbar.LENGTH_INDEFINITE)
.setBehavior(NonDismissibleBehavior())
}

private val findInPageTextWatcher = object : TextChangedWatcher() {
override fun afterTextChanged(editable: Editable) {
viewModel.userFindingInPage(findInPageInput.text.toString())
Expand Down Expand Up @@ -309,7 +314,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
popupMenu.apply {
onMenuItemClicked(view.forwardPopupMenuItem) { viewModel.onUserPressedForward() }
onMenuItemClicked(view.backPopupMenuItem) { activity?.onBackPressed() }
onMenuItemClicked(view.refreshPopupMenuItem) { refresh() }
onMenuItemClicked(view.refreshPopupMenuItem) { viewModel.onRefreshRequested() }
onMenuItemClicked(view.newTabPopupMenuItem) { viewModel.userRequestedOpeningNewTab() }
onMenuItemClicked(view.bookmarksPopupMenuItem) { browserActivity?.launchBookmarks() }
onMenuItemClicked(view.addBookmarksPopupMenuItem) { launch { viewModel.onBookmarkAddRequested() } }
Expand Down Expand Up @@ -374,6 +379,8 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
}

private fun showHome() {
errorSnackbar.dismiss()
newTabLayout.show()
showKeyboardImmediately()
appBarLayout.setExpanded(true)
webView?.onPause()
Expand All @@ -383,6 +390,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
}

private fun showBrowser() {
newTabLayout.gone()
webView?.show()
webView?.onResume()
omnibarScrolling.enableOmnibarScrolling(toolbarContainer)
Expand All @@ -398,13 +406,17 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
webView?.loadUrl(url)
}

fun onRefreshRequested() {
viewModel.onRefreshRequested()
}

fun refresh() {
webView?.reload()
}

private fun processCommand(it: Command?) {
when (it) {
Command.Refresh -> refresh()
is Command.Refresh -> refresh()
is Command.OpenInNewTab -> {
browserActivity?.openInNewTab(it.query)
}
Expand All @@ -419,10 +431,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
is Command.NavigateBack -> {
webView?.goBackOrForward(-it.steps)
}
Command.NavigateForward -> {
is Command.NavigateForward -> {
webView?.goForward()
}
Command.ResetHistory -> {
is Command.ResetHistory -> {
resetWebView()
}
is Command.DialNumber -> {
Expand All @@ -439,10 +451,10 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
val intent = Intent(Intent.ACTION_SENDTO, Uri.parse("smsto:${it.telephoneNumber}"))
openExternalDialog(intent)
}
Command.ShowKeyboard -> {
is Command.ShowKeyboard -> {
showKeyboard()
}
Command.HideKeyboard -> {
is Command.HideKeyboard -> {
hideKeyboard()
}
is Command.BrokenSiteFeedback -> {
Expand All @@ -458,7 +470,7 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
}
is Command.DownloadImage -> requestImageDownload(it.url)
is Command.FindInPageCommand -> webView?.findAllAsync(it.searchTerm)
Command.DismissFindInPage -> webView?.findAllAsync(null)
is Command.DismissFindInPage -> webView?.findAllAsync(null)
is Command.ShareLink -> launchSharePageChooser(it.url)
is Command.CopyLink -> {
clipboardManager.primaryClip = ClipData.newPlainText(null, it.url)
Expand All @@ -481,6 +493,14 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
is Command.SaveCredentials -> saveBasicAuthCredentials(it.request, it.credentials)
is Command.GenerateWebViewPreviewImage -> generateWebViewPreviewImage()
is Command.LaunchTabSwitcher -> launchTabSwitcher()
is Command.ShowErrorWithAction -> showErrorSnackbar(it)
}
}

private fun showErrorSnackbar(command: Command.ShowErrorWithAction) {
//Snackbar is global and it should appear only the foreground fragment
if (!errorSnackbar.view.isAttachedToWindow && isVisible) {
errorSnackbar.setAction(R.string.crashedWebViewErrorAction) { command.action() }.show()
}
}

Expand Down Expand Up @@ -1124,13 +1144,23 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
}

fun renderGlobalViewState(viewState: GlobalLayoutViewState) {
if (lastSeenGlobalViewState is GlobalLayoutViewState.Invalidated &&
viewState is GlobalLayoutViewState.Browser) {
throw IllegalStateException("Invalid state transition")
}

renderIfChanged(viewState, lastSeenGlobalViewState) {
lastSeenGlobalViewState = viewState

if (viewState.isNewTabState) {
browserLayout.hide()
} else {
browserLayout.show()
when (viewState) {
is GlobalLayoutViewState.Browser -> {
if (viewState.isNewTabState) {
browserLayout.hide()
} else {
browserLayout.show()
}
}
is GlobalLayoutViewState.Invalidated -> destroyWebView()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this valid? It eventually calls webview.destroy() however according to the docs:

The given WebView can't be used, and should be removed from the view hierarchy, all references to it should be cleaned up

I recommend creating an invalidate method that removes and nulls out the WebView rather than reusing this method that interacts with the Webview instance when we don't know what state it is in.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I had the same doubt because "WebView can't be used" is not clear enough.
I did some code investigations and tests, and I deduced that they refer to continue rendering in that WebView instance, or any action that will directly involve the renderer process.

While reading again the docs, their sample is using exactly that method call, so I would say that it's safe and correct in order to clean up WebView internal references.

// Remove the inflated view from the container and cleanup
// the dead webview specifically (if it is not GC'ed it will cause
// problems later, the next time the renderer dies)
(...)
view.destroy();

}
}
}
Expand Down Expand Up @@ -1174,6 +1204,8 @@ class BrowserTabFragment : Fragment(), FindListener, CoroutineScope {
newTabPopupMenuItem.isEnabled = browserShowing
addBookmarksPopupMenuItem?.isEnabled = viewState.canAddBookmarks
sharePageMenuItem?.isEnabled = viewState.canSharePage
brokenSitePopupMenuItem?.isEnabled = viewState.canReportSite
requestDesktopSiteCheckMenuItem?.isEnabled = viewState.canChangeBrowsingMode

addToHome?.let {
it.visibility = if (viewState.addToHomeVisible) VISIBLE else GONE
Expand Down
Loading