Add onRenderProcessGone override to webview logic, Fixes AB#3405900 #2788
Conversation
|
✅ Work item link check complete. Description contains link AB#3405900 to an Azure Boards work item. |
|
❌ Work item link check failed. Description contains AB#3405900 but the Bot could not link it to an Azure Boards work item. Click here to learn more. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds error handling for WebView render process crashes to prevent ANRs in calling applications. When the WebView render process fails, the new onRenderProcessGone override catches the crash and sends an error to the callback instead of letting the exception propagate uncaught.
- Overrides
onRenderProcessGonemethod to handle WebView process crashes gracefully - Implements version-specific error messages based on Android SDK level
- Returns true to indicate the crash was handled successfully
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Handle render process crash | ||
| // TODO: is there an errorCode that makes more sense in this spot? We aren't actually getting anything back from webView, we have to come up with it ourselves | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| sendErrorToCallback(view, 0, "WebView render process gone, crahsd? : " + detail.didCrash()); |
There was a problem hiding this comment.
Corrected spelling of 'crahsd' to 'crashed'.
| sendErrorToCallback(view, 0, "WebView render process gone, crahsd? : " + detail.didCrash()); | |
| sendErrorToCallback(view, 0, "WebView render process gone, crashed? : " + detail.didCrash()); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Agreed with CP her on spelling error to be updated.
common/src/main/java/com/microsoft/identity/common/internal/ui/webview/OAuth2WebViewClient.java
Show resolved
Hide resolved
github-actions
bot
changed the title
fadidurah
added
the
No-Changelog
| // Handle render process crash | ||
| // TODO: is there an errorCode that makes more sense in this spot? We aren't actually getting anything back from webView, we have to come up with it ourselves | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| sendErrorToCallback(view, 0, "WebView render process gone, crahsd? : " + detail.didCrash()); |
There was a problem hiding this comment.
Agreed with CP her on spelling error to be updated.
| @Override | ||
| public boolean onRenderProcessGone(WebView view, RenderProcessGoneDetail detail) { | ||
| // Handle render process crash | ||
| // TODO: is there an errorCode that makes more sense in this spot? We aren't actually getting anything back from webView, we have to come up with it ourselves |
There was a problem hiding this comment.
Is the plan to leave TODO here in comment?
There was a problem hiding this comment.
Nope, i usually add it to prompt discussion in PR stage, then remove it once we decide. Sometimes it stays if the discussion is not finalized but code can go in for the time.
There was a problem hiding this comment.
Maybe one could be created in ClientException for Webview crashes (WEBVIEW_CRASH?) which we can let oneauth know about.
| @Override | ||
| public boolean onRenderProcessGone(WebView view, RenderProcessGoneDetail detail) { | ||
| // Handle render process crash | ||
| // TODO: is there an errorCode that makes more sense in this spot? We aren't actually getting anything back from webView, we have to come up with it ourselves |
There was a problem hiding this comment.
Maybe one could be created in ClientException for Webview crashes (WEBVIEW_CRASH?) which we can let oneauth know about.
3 participants
We do not gracefully handle webview process crashes, which can cause ANRs for calling activitties (LTW bug: https://dev.azure.com/microsoft/OS/_workitems/edit/59756442). In this PR we override the webview method onRenderProcessGone to handle what to do if webview no longer is able to render content, where we send the error to the callback rather than having an uncaught exception.
AB#3405900