feat: add plugin hooks for WebViewClient.onRenderProcessGone

[peitschie]

Platforms affected

Android

Motivation and Context

This change allows more advanced handling of the webview render process termination. This can be useful for capturing additional information, or even conceivably allow the crash to be prevented should a plugin choose to.

Description

Expose the WebViewClient.onRenderProcessGone method down to plugins, and allow a plugin to interrupt the crash should it choose so.

Testing

I created the hello world cordova application and added a javascript call to load chrome://crash (recommended by Android docs as the best way to trigger a renderer crash).

Checklist

• I've run the tests to see all new and existing tests pass
• I added automated test coverage as appropriate for this change
• Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
• If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
• I've updated the documentation if necessary

[codecov-commenter]

Codecov Report

Merging #1574 (9f320f4) into master (7efe90f) will increase coverage by 0.78%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1574      +/-   ##
==========================================
+ Coverage   71.82%   72.61%   +0.78%     
==========================================
  Files          23       23              
  Lines        1796     1800       +4     
==========================================
+ Hits         1290     1307      +17     
+ Misses        506      493      -13     

see 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[lgh06]

Nice work. I face webview hang , found no solution. I will try your cordova-android and write a plugin

[breautek]

or even conceivably allow the crash to be prevented should a plugin choose to.

Given that the Android Docs states

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.

I think it's unsafe to attempt to "prevent" a crash or continue using the webview. I think at minimum the webview needs to be recreated. I do agree that it is definitely useful for debugging or crashlytics however.

The RenderProcessGoneDetail API was added in API 26. cordova-android@12 supports as far back as API 24, but we will compile with API 33. Because this is a delegate function, I believe this is fine, it just won't be invoked by the underlying system on API 24 or 25, I presume. Was this tested on API 24 devices/emulators?

[breautek]

Tested both on API 24 and API 33 emulators, both of which I believe works as expected.

On API 24, the hook is simply not invoked and the process will simply crash.

On API 33, the hook is invoked before the crash, allowing the system to gracefully handle the crash and to potential recover from it.

[peitschie]

I think it's unsafe to attempt to "prevent" a crash or continue using the webview. I think at minimum the webview needs to be recreated. I do agree that it is definitely useful for debugging or crashlytics however.

Yep! Agree that more work would be needed if some kind of crash recovery was needed... returning true here would not be a good thing to do 😅

I've tweaked this a bit, so should be good for another review when you have time @breautek

[breautek]

Alright, had to do some tweaks in cordova-android to test it, but at the core I think this is good.

I implemented the on hook in CoreAndroid and was able to do something like:

@Override
    public boolean onRenderProcessGone(final WebView view, RenderProcessGoneDetail detail) {
        if (webView.getEngine().getView() == view) {
            // Is the crash view the cordova webview?
            LOG.e(TAG, "CORDOVA WEBVIEW CRASHED!");
            return true;
        }
        return false;
    }

Naturally returning true prevents the app from crashing and actually yields the webview still visible, but in an unsafe manner. Returning false will crash the application, assuming nothing else actually handles the webview. To be clear, I'm not suggesting to implement something like this into this PR, I'm just using the PR to demonstrate what is possible.

There may be some plugins that want to listen to every crash (e.g., a crash report integration plugin of some sort),

I do agree with this sentiment. I see that PluginManager will still call through all plugins hooks even if a plugin has already return true, so I think that covers that.

[peitschie]

Thanks for the review and merge @breautek 🍻

Agree with the wording change there. We definitely want plugins to leave the return value alone unless they really know what they're doing!

[erisu]

LGTM

[breautek]

Awesome work, thanks @peitschie

This feature will be available in the upcoming cordova-android@12 release.