Conversation
Comment out JDK 17 path for AGP compatibility.
…ad manager, UI init feedback, 4096 context window, generation settings, real-time commands, offline model screen elements
…ffline model fixes, 503 error handling, download notifications, Human Expert foreground service fix, HumanOperator icon, package name change
…nId, not namespace)
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR introduces a human operator feature for remote screen control via WebRTC. While the implementation is functional, there are 5 critical issues that must be fixed before merge:
Critical Issues Found:
- Memory Leak - Deprecated Handler constructor in MainActivity causing potential memory leaks
- Crash Risk - Null pointer exceptions in WebRTCClient when dataChannel is null during tap events
- Crash Risk - Multiple forced unwraps (
!!) in WebRTCSender that will crash if objects become null - Crash Risk - Unhandled JSON parsing exceptions when processing tap coordinates
- Crash Risk - Unsafe type casts and null handling in screen capture initialization
Security Concerns:
The AndroidManifest declares sensitive permissions (QUERY_ALL_PACKAGES, INJECT_EVENTS) that may require Play Store justification or could be rejected. Ensure these are absolutely necessary for core functionality.
Recommendation:
Do not merge until the crash risks are resolved. The memory leak and null safety issues will cause production crashes and poor user experience. Please address the code suggestions provided in the line-by-line comments.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
| private fun sendDataChannelMessage(message: String) { | ||
| val buffer = DataChannel.Buffer(java.nio.ByteBuffer.wrap(message.toByteArray()), false) | ||
| if (dataChannel?.send(buffer) != true) { | ||
| Log.w(TAG, "Failed to send DataChannel message: ${message.take(50)}") | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Crash Risk: Null pointer exception when dataChannel is null. The sendDataChannelMessage method attempts to send data when dataChannel may be null, but only logs a warning. This means tap events will silently fail when the data channel isn't open, causing the UI to be unresponsive. Add proper null checks or wait for the data channel to open before allowing user interaction.
| val json = com.google.gson.JsonParser.parseString(message).asJsonObject | ||
| if (json.has("type") && json.get("type").asString == "tap") { | ||
| val x = json.get("x").asFloat | ||
| val y = json.get("y").asFloat | ||
| listener.onTapReceived(x, y) | ||
| } |
There was a problem hiding this comment.
🛑 Crash Risk: Unhandled JSON parsing exception can crash the app. If the JSON is malformed or missing required fields (x, y), asFloat will throw an exception that's caught but leaves the connection in an inconsistent state. Validate JSON fields exist before accessing them.
| val json = com.google.gson.JsonParser.parseString(message).asJsonObject | |
| if (json.has("type") && json.get("type").asString == "tap") { | |
| val x = json.get("x").asFloat | |
| val y = json.get("y").asFloat | |
| listener.onTapReceived(x, y) | |
| } | |
| val json = com.google.gson.JsonParser.parseString(message).asJsonObject | |
| if (json.has("type") && json.get("type").asString == "tap") { | |
| if (json.has("x") && json.has("y")) { | |
| val x = json.get("x").asFloat | |
| val y = json.get("y").asFloat | |
| listener.onTapReceived(x, y) | |
| } else { | |
| Log.w(TAG, "Tap message missing x or y coordinates") | |
| } | |
| } |
| android.os.Handler(mainLooper).postDelayed({ | ||
| val answer = rtcClient.getLocalDescription() | ||
| if (answer != null) { | ||
| signalingClient?.sendAnswer(answer.description) | ||
| } | ||
| }, 1000) |
There was a problem hiding this comment.
🛑 Security Vulnerability: Deprecated Handler constructor causes memory leak and potential crashes. The android.os.Handler(mainLooper) constructor is deprecated and can lead to memory leaks when the activity is destroyed but the handler continues to exist. Use Handler(Looper.getMainLooper()) with proper lifecycle management or coroutines.
| android.os.Handler(mainLooper).postDelayed({ | |
| val answer = rtcClient.getLocalDescription() | |
| if (answer != null) { | |
| signalingClient?.sendAnswer(answer.description) | |
| } | |
| }, 1000) | |
| override fun onSDPOffer(sdp: String) { | |
| rtcClient.setRemoteOffer(sdp) | |
| // Send answer after a short delay | |
| Handler(Looper.getMainLooper()).postDelayed({ | |
| val answer = rtcClient.getLocalDescription() | |
| if (answer != null) { | |
| signalingClient?.sendAnswer(answer.description) | |
| } | |
| }, 1000) | |
| } |
| val factory = peerConnectionFactory ?: return | ||
| videoSource = factory.createVideoSource(videoCapturer!!.isScreencast) | ||
|
|
||
| // Initialize capturer | ||
| (videoCapturer as ScreenCapturerAndroid).initialize( | ||
| SurfaceTextureHelper.create("CaptureThread", eglBase.eglBaseContext), | ||
| context, | ||
| videoSource!!.capturerObserver | ||
| ) | ||
| (videoCapturer as ScreenCapturerAndroid).startCapture(720, 1280, 30) // Adjust resolution/fps as needed | ||
|
|
||
| videoTrack = factory.createVideoTrack("ARDAMSv0", videoSource) | ||
| videoTrack?.setEnabled(true) |
There was a problem hiding this comment.
🛑 Crash Risk: Multiple forced unwraps and unsafe casts can crash the app. Replace !! operators and unsafe casts with safe alternatives to prevent NullPointerException and ClassCastException. The current implementation will crash if videoCapturer is null or not a ScreenCapturerAndroid instance.
1 participant
No description provided.