Allow users to recover from WebView renderer failures #644
Conversation
* Error forwarded from WebViewClient to ViewModel * Introduced a new command to show an error with recovery action. * GlobalLayoutViewState as sealed class to model invalidated state or Browser. * Fragment invalidates WebView after crash destroying it.
…outViewState. Detected some issues when having multiple tabs crashed. Snackbar is like a singleton view that will be added only to one view hierarchy. We need to avoid adding the error snackbar in a non visible instance of BrowserTabFragment.
… tab and open query in a new tab.
…dated state, otherwise refresh URL.
cmonfortep
changed the title
* Disabling navigation should affect also webNavigationState * Included a new StateChanged and new WebNavigationState to clear user navigations without affecting Site properties * Clear navigation using navigationStateChanged * Cover new logic and types with tests
cmonfortep
force-pushed
the
feature/cm/recover_from_webview_crashes
branch
from
b174a94 to
0435a69
Compare
cmonfortep
requested review from
CDRussell,
marcosholgado and
subsymbolic
as code owners
subsymbolic
left a comment
subsymbolic
left a comment
There was a problem hiding this comment.
This is looking great so far however the new state acts as if the user has "gone home" which they haven't and introduces odd behavior like showing the logo and call-to-action (CTA)
app/src/androidTest/java/com/duckduckgo/app/browser/BrowserTabViewModelTest.kt
Show resolved
Hide resolved
|
|
||
| showErrorWithAction.action() | ||
|
|
||
| assertCommandIssued<Command.OpenInNewTab>() |
There was a problem hiding this comment.
Should we check the URL too?
There was a problem hiding this comment.
Sure, I will add a new testcase to cover that part.
|
|
||
| <resources> | ||
| <string name="crashedWebViewErrorMessage" translatable="false">"The webpage could not be displayed."</string> | ||
| <string name="crashedWebViewErrorAction" translatable="false">"Reload"</string> |
There was a problem hiding this comment.
Update translation="false" to tools:ignore="MissingTranslation", that way when we open the translation editor in android studio the missing translations are highlighted.
| } | ||
|
|
||
| override fun onRenderProcessGone(view: WebView?, detail: RenderProcessGoneDetail?): Boolean { | ||
| viewModel.onSurveyFailedToLoad() |
There was a problem hiding this comment.
Is this valid? It eventually leads to a call to webView.gone() 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
In the case of an onRenderProcessGone failure the WebView should removed and set to null not just hidden.
There was a problem hiding this comment.
Yes, it's valid to call webView.gone() but not enough. I will improve this part in order to do a better clean up for that WebView.
There was a problem hiding this comment.
I went for adding extra logic inside showError in order to fully remove the WebView, instead of creating a new method in SurveyViewModel + a new Command to destroy the WebView.
Since currently when an error happens user can't interact with Survey, I just destroy the WebView too, there's no need to keep it alive.
| browserLayout.show() | ||
| } | ||
| } | ||
| is GlobalLayoutViewState.Invalidated -> destroyWebView() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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();
* show error when using is browing, not when is in home page. * when goes to home page after a crash, disable forward navigation.
2 participants
Task/Issue URL: https://app.asana.com/0/0/1152512237237203/f
Tech Design URL: https://app.asana.com/0/0/1151307893053517/f
CC:
Description:
Introducing gracefully handling WebView crash errors into our Browser.
A WebView render process can be gone due to a rendering crash or memory pressure situations. When we receive such event from WebViewClient we will avoid crashing, and instead, we will show a snackbar to inform about what happened and the possibility to reload the current URL. Also, application will still be usable so users will be able to perform other actions as closing the tab or navigate to a new URL.
Steps to test this PR:
As we can't reproduce easily memory pressure situations or crashes, we will need to change our source code a bit in order to manually produce a crash.
Easiest way is to call
recoverFromRenderProcessGone()when user clicks in a menu option, example will be insideonBrokenSiteSelected().Scenarios to test
Scenario to test:
Scenario to test:
Scenario to test:
Scenario to test:
Scenario to test:
Scenario to test:
Scenario to test:
Scenario to test:
Internal references:
Software Engineering Expectations
Technical Design Template