FrontendScreen filter out messages not coming from the frontend external bus#6687
FrontendScreen filter out messages not coming from the frontend external bus#6687
Conversation
There was a problem hiding this comment.
Pull request overview
This PR extends the “only react to messages coming from the frontend external bus” work to FrontendScreen by introducing a revised JS bridge (V1 + V2) that parses/validates messages earlier, adds origin/iframe filtering for V2, and updates related plumbing + tests.
Changes:
- Introduces a new
frontend/js/FrontendJsBridgewith V1 (addJavascriptInterface) + V2 (WebMessageListener) support and shared parsing/dispatch logic - Switches external bus handling to pass
JsonElement(instead of raw JSON strings) and enriches unknown-polymorphic payloads with a discriminator for better diagnostics - Updates
FailFast.failWhento return the checked condition, enabling guard-style usage, and adjusts tests accordingly
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| common/src/test/kotlin/io/homeassistant/companion/android/common/util/FailFastTest.kt | Updates unit tests for new FailFast.failWhen boolean return value |
| common/src/main/kotlin/io/homeassistant/companion/android/common/util/JsonUtil.kt | Fixes serializer module naming and extends UnknownJsonContent with discriminator |
| common/src/main/kotlin/io/homeassistant/companion/android/common/util/FailFast.kt | Makes failWhen return the evaluated condition for inline-guard usage |
| common/src/main/kotlin/io/homeassistant/companion/android/common/data/websocket/impl/entities/SocketResponse.kt | Renames serializer module and propagates discriminator into unknown socket responses |
| app/src/test/kotlin/io/homeassistant/companion/android/util/HAWebViewClientTest.kt | Updates test after removing JS callback reattachment from HAWebViewClient |
| app/src/test/kotlin/io/homeassistant/companion/android/onboarding/connection/ConnectionViewModelTest.kt | Updates mocks/signatures after HAWebViewClientFactory.create API change |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsBridgeTest.kt | Adds comprehensive tests for V1/V2 bridge attach + dispatch + validation |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/handler/FrontendMessageHandlerTest.kt | Updates tests for typed payloads and JsonElement external bus input |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendViewModelTest.kt | Updates ViewModel construction to inject FrontendJsBridgeFactory |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendScreenTest.kt | Updates imports for new JS bridge location |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/FrontendJsBridgeTest.kt | Removes old bridge test that targeted the pre-refactor bridge API |
| app/src/test/kotlin/io/homeassistant/companion/android/frontend/externalbus/FrontendExternalBusRepositoryImplTest.kt | Updates external bus repository tests to use JsonElement inputs and adds new edge-case coverage |
| app/src/screenshotTest/kotlin/io/homeassistant/companion/android/frontend/FrontendScreenScreenshotTest.kt | Updates previews/tests to use FrontendJsBridge.noOp |
| app/src/main/kotlin/io/homeassistant/companion/android/webview/WebViewActivity.kt | Reuses bridge constants/logic and uses FailFast.failWhen guard pattern for callback validation |
| app/src/main/kotlin/io/homeassistant/companion/android/util/HAWebViewClient.kt | Removes frontendJsCallback parameter and reattachment behavior on render-process crash |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsHandler.kt | Adds a typed handler interface for bridge callbacks |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsCallback.kt | Adds an attach-only callback interface for WebView bridge registration |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/js/FrontendJsBridge.kt | Adds new V1/V2 bridge implementation with origin/iframe filtering and early JSON parsing |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/handler/FrontendMessageHandler.kt | Switches to typed auth payload + JsonElement bus messages; moves parsing responsibility to the bridge |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendViewModel.kt | Creates the bridge via assisted factory and passes bridge state (serverId/url) |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendScreen.kt | Attaches the JS bridge in WebViewEffects right before loading the URL |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/FrontendJsBridge.kt | Removes the old single-protocol bridge implementation |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/externalbus/incoming/IncomingExternalBusMessage.kt | Adds discriminator propagation for unknown incoming external bus message types |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/externalbus/FrontendExternalBusRepositoryImpl.kt | Changes repository input to JsonElement and decodes via decodeFromJsonElement |
| app/src/main/kotlin/io/homeassistant/companion/android/frontend/externalbus/FrontendExternalBusRepository.kt | Updates repository API to accept JsonElement instead of raw strings |
24669a5 to
07390e1
Compare
|
|
||
| override fun onRenderProcessGone(view: WebView?, detail: RenderProcessGoneDetail?): Boolean { | ||
| Timber.e("onRenderProcessGone: webView crashed") | ||
| view?.let { webView -> | ||
| frontendJsCallback?.attachToWebView(webView) | ||
| onCrash?.invoke() | ||
| } | ||
| onCrash?.invoke() | ||
| return true | ||
| } | ||
|
|
There was a problem hiding this comment.
onRenderProcessGone indicates the WebView renderer process has died and the existing WebView instance is no longer safe to keep using. Currently the code only invokes onCrash and returns true, but does not destroy the WebView (or otherwise ensure it gets recreated), which can lead to repeated failures if the UI continues calling loadUrl on the same instance.
Consider explicitly destroying the view here and/or changing the crash callback contract so the UI can drop/recreate the WebView instance after a renderer crash.
There was a problem hiding this comment.
@jpelgrom I don't want to change this here since I've replicated the existing behavior but maybe that's something to consider? I have no idea how to properly test this scenario.
There was a problem hiding this comment.
We previously had a discussion about this in #3787 and I'm still not sure what the best path is here. Reloading seems to work despite the documentation suggesting otherwise. You can test with chrome://crash.
07390e1 to
87be04c
Compare
|
I think I made a Git mistake by merging the other one first, and now it says there is a merge conflict even though it was clean when stacked on top? (Thankfully this is used in debug only) |
jpelgrom
left a comment
There was a problem hiding this comment.
Tested and reviewed. Good to merge after resolving the merge conflict.
It's fine it's just a rebase to do since it was based on the other branch. I'll handle it Monday |
87be04c to
c6279f3
Compare
Summary
Same as #6680 but on the
FrontendScreen, it also add proper testing. I had to rework the bridge pattern to make it more coherent with the switch between the two version.FailFast.failWhento avoid duplication of codediscriminatorWebViewActivityto avoid duplicationChecklist
Any other notes
Based on #6680
I found out a small bug in the
FrontendViewModel.urlFlowthat if we call.valueon it without any subscriber even if the state contains a URL it returnsnull, I'm going to address this into another PR.