Bugfix/crash web view #3787

dmiales · 2023-08-12T08:41:12Z

This PR fixes issue #3500. What was the issue:
I have a tablet hanging as a home control. The tablet is running home assistant. And about once a day the application crashes. And when I pass by it, I see the desktop. The reason is that the WebView is leaking memory. And in the event of the death of the WebView process (com.google.android.webview:sandboxed_process0:org.chromium.content.app.SandboxedProcessService), our application also died. It is unacceptable.
I immediately provide a reproduction of the problem:

use adb: adb shell
without permissions, we won't be able to kill the process: su
find and kill the process: ps -A | grep sandboxed | awk '{print $2}' | xargs kill

For clarity, I will add two videos (before / after)

before.mp4

afrer.mp4

jpelgrom · 2023-08-12T09:13:22Z

The documentation specifically calls out that the actual view shouldn't be reused:

The given WebView can't be used, and should be removed from the view hierarchy, all references to it should be cleaned up, e.g any references in the Activity or other classes saved using View.findViewById(int) and similar calls, etc.

but this is reusing the view?

Clearing cache also should be removed as it isn't related to crash handling.

dmiales · 2023-08-12T10:06:37Z

Removed unnecessary calls.

Because we're using a trick.
The documentation says:

Notify host application that the given WebView's render process has exited. Our goal is not to let it complete, we process the situation and reboot (a new sandbox process is created).

if the host application handled the situation that process has exited, otherwise, application will crash if render process crashed, or be killed if render process was killed by the system.

And in this case, a running application is clearly preferable to a falling one.

jpelgrom · 2023-08-12T16:24:58Z

Now we've quoted all documentation for that function :)

I still don't understand your position. You're reusing the view. That is not a 'trick'? As I understand it, you can use this callback to tear out the view and handle it (which could be creating a new one).

dshokouhi · 2023-08-13T01:27:45Z

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

+                    view: WebView?,
+                    handler: RenderProcessGoneDetail?
+                ): Boolean {
+                    webView.reload()


Should we add a logging statement here? not sure if any details will be helpful but could be useful for troubleshooting purposes?

Also it looks like this method gets called for every open webview so maybe we need to match view to make sure its what we expected? view == webView before doing any recreation?

Multiple WebView instances may be associated with a single render process; onRenderProcessGone will be called for each WebView that was affected.

Not sure if we need to worry about an actual crash or not too?

https://developer.android.com/reference/android/webkit/RenderProcessGoneDetail#didCrash()

this is indeed a good find though, nice to know we can handle this.

@dshokouhi
I added logging. It's important, I agree.

didCrash() is a cool check. But there is a problem:

Added in API level 26

we have min 21

view == webView

wouldn't that be overkill? As far as I could check in webView is always the same as in the view parameter.

I keep you informed, the application on the tablet has been working for almost two days without failures

didCrash() is a cool check. But there is a problem:\n\nAdded in API level 26

So is the entire method you are overriding

https://developer.android.com/reference/android/webkit/WebViewClient#onRenderProcessGone(android.webkit.WebView,%20android.webkit.RenderProcessGoneDetail)

Just add a conditional check we have plenty of code like that

wouldn't that be overkill? As far as I could check in webView is always the same as in the view parameter.

The documentation tells us it may not be the same, also user can have multiple instances created using shortcut or notification.

this is strange behavior

Maybe then like this? Reload view from parameter

dshokouhi · 2023-08-13T18:12:28Z

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

+                    handler: RenderProcessGoneDetail?
+                ): Boolean {
+                    Log.e(TAG, "onRenderProcessGone: webView crashed")
+                    view!!.reload()


This will crash if view is null we should avoid crashing when possible

Reading the description again I dont think WebView can load in certain cases which is why the description says to remove the view from the hierarchy

The application's implementation of this callback should only attempt to clean up the specific WebView given as a parameter, and should not assume that other WebView instances are affected. The given WebView can't be used, and should be removed from the view hierarchy, all references to it should be cleaned up, e.g any references in the Activity or other classes saved using [View.findViewById(int)](https://developer.android.com/reference/android/view/View#findViewById(int)) and similar calls, etc.

have you tested this fix against the site Google recommended to test against?

To cause an render process crash for test purpose, the application can call loadUrl("chrome://crash") on the WebView.

dshokouhi

We need to make sure we follow Google's recommendation here and to clean up the WebView instance that is no longer available

The application's implementation of this callback should only attempt to clean up the specific WebView given as a parameter, and should not assume that other WebView instances are affected. The given WebView can't be used, and should be removed from the view hierarchy, all references to it should be cleaned up, e.g any references in the Activity or other classes saved using [View.findViewById(int)](https://developer.android.com/reference/android/view/View#findViewById(int)) and similar calls, etc.

home-assistant · 2023-09-19T15:35:00Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

dmiales · 2023-09-19T18:46:38Z

@dshokouhi

I was on vacation. But before that, I committed the changes and tested the hypotheses. Replying to the first comment:

have you tested this fix against the site Google recommended to test against?

Yes, I checked for chrome://crash, it works identical to killing the process.
Replying to the last comment:

We need to make sure we follow Google's recommendation here and to clean up the WebView instance that is no longer available

https://github.com/home-assistant/android/pull/3787/files#diff-76bbedb2918a5db79c7070e9bb2e01d9241388745f60b0719c0d1f74123d89bdR396 I corrected it here, we take exactly the instance that comes into the method.

While I was testing my solution, I found an unusual error that broke the application
home-assistant/core#71205 (the same error exists in neighboring projects.
#2076 (comment)
#2369 (comment)
The essence is the same: Disconnected: "Did not receive auth message within 10 seconds") I debugged and realized that when we recreate the WebView, addJavascriptInterface is not created again. Because of this, we lose communication with the internal API. As a result, I put it in a separate method; we use it during initial initialization and again when re-creating the WebView.

dshokouhi · 2023-09-19T19:27:51Z

https://github.com/home-assistant/android/pull/3787/files#diff-76bbedb2918a5db79c7070e9bb2e01d9241388745f60b0719c0d1f74123d89bdR396 I corrected it here, we take exactly the instance that comes into the method.

so looking at the commit we are attempting to reload() the webview if the view is still there. However that is not following Google's recommendation of removing the view from the hierarchy.

Reloading does not do anything to the view hierarchy according to the docs.

https://developer.android.com/reference/android/webkit/WebView#reload()

dmiales · 2023-09-21T11:01:04Z

I understand what you are talking about. Probably the ideal solution is to completely recreate the webview? But it’s extremely difficult to do this with this hook. It’s unlikely that I have enough competence to implement this.

The question here is why this is not recommended, and whether we should adhere to this recommendation in this case. After all, we are rebooting in a super-critical situation, the other way out of which is a complete crash of the application.
And if you look at which is worse, the choice is obvious.

At least my smart home is on this commit, which makes me happy

dmiales · 2023-10-03T11:02:52Z

@dshokouhi check my comment

dshokouhi

So I ran some testing here and using Googles recommended way of testing the issue I can see that it is working and the page is refreshed accordingly. Lets just make sure to clean up any new introduced warnings and I think this should be ok.

dshokouhi · 2023-10-06T18:38:59Z

app/src/main/java/io/homeassistant/companion/android/webview/WebViewActivity.kt

@@ -224,6 +225,7 @@ class WebViewActivity : BaseActivity(), io.homeassistant.companion.android.webvi
    private var downloadFileUrl = ""
    private var downloadFileContentDisposition = ""
    private var downloadFileMimetype = ""
+    private var javascriptInterface = "externalApp"


this can be a val as its not going to be changed

dshokouhi

So I tested the PR using the recommendation from Google (loading chrome://crash). Actually could only test using remote debugging. Sure enough webview crashed and the code reloaded and the page was fine so the fix looks good here.

dmiales · 2023-10-06T22:26:20Z

I also tested using this method. And the main thing is that the tablet has been crash-free for almost two weeks now

home-assistant bot added the cla-signed label Aug 12, 2023

jpelgrom linked an issue Aug 12, 2023 that may be closed by this pull request

App crashes several times a day #3500

Closed

dshokouhi reviewed Aug 13, 2023

View reviewed changes

dshokouhi requested changes Sep 19, 2023

View reviewed changes

home-assistant bot marked this pull request as draft September 19, 2023 15:34

dmiales requested a review from dshokouhi September 21, 2023 11:01

dmiales force-pushed the bugfix/crashWebView branch from 5f74b63 to 523acc1 Compare October 2, 2023 10:45

dmiales marked this pull request as ready for review October 3, 2023 10:30

dmiales added 8 commits October 3, 2023 13:30

bugfix/crashWebView reload webview after crash

bbde5fb

bugfix/crashWebView reload webview after crash

4d3bde0

bugfix/crashWebView reload webview after crash

1488e8d

bugfix/crashWebView reload webview after crash

586bbc4

bugfix/crashWebView reload webview after crash

d2a81e1

bugfix/crashWebView reload webview after crash

6353e15

bugfix/crashWebView reload webview after crash

933cf29

bugfix/crashWebView reload webview after crash

118d532

dmiales force-pushed the bugfix/crashWebView branch from 523acc1 to 118d532 Compare October 3, 2023 10:30

dshokouhi reviewed Oct 6, 2023

View reviewed changes

bugfix/crashWebView codereview

3577cb7

dshokouhi approved these changes Oct 6, 2023

View reviewed changes

JBassett approved these changes Oct 13, 2023

View reviewed changes

JBassett merged commit eba9295 into home-assistant:master Oct 13, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix/crash web view #3787

Bugfix/crash web view #3787

dmiales commented Aug 12, 2023

jpelgrom commented Aug 12, 2023

dmiales commented Aug 12, 2023

jpelgrom commented Aug 12, 2023

dshokouhi Aug 13, 2023

dshokouhi Aug 13, 2023

dmiales Aug 13, 2023

dshokouhi Aug 13, 2023 •

edited

dmiales Aug 13, 2023

dshokouhi Aug 13, 2023

dmiales Aug 13, 2023

dshokouhi Aug 14, 2023

dshokouhi left a comment

home-assistant bot commented Sep 19, 2023

dmiales commented Sep 19, 2023

dshokouhi commented Sep 19, 2023

dmiales commented Sep 21, 2023

dmiales commented Oct 3, 2023

dshokouhi left a comment

dshokouhi Oct 6, 2023

dmiales Oct 6, 2023

dshokouhi left a comment

dmiales commented Oct 6, 2023

Bugfix/crash web view #3787

Bugfix/crash web view #3787

Conversation

dmiales commented Aug 12, 2023

jpelgrom commented Aug 12, 2023

dmiales commented Aug 12, 2023

jpelgrom commented Aug 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshokouhi Aug 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshokouhi left a comment

Choose a reason for hiding this comment

home-assistant bot commented Sep 19, 2023

dmiales commented Sep 19, 2023

dshokouhi commented Sep 19, 2023

dmiales commented Sep 21, 2023

dmiales commented Oct 3, 2023

dshokouhi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dshokouhi left a comment

Choose a reason for hiding this comment

dmiales commented Oct 6, 2023

dshokouhi Aug 13, 2023 •

edited